-
Notifications
You must be signed in to change notification settings - Fork 171
Always build and run Cython tests + other CI improvements #640
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
/ok to test f0d7333 |
/ok to test 18be719 |
/ok to test c41a39a |
/ok to test 227e315 |
/ok to test |
@leofang, there was an error processing your request: See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/1/ |
/ok to test 9fc20e1 |
/ok to test 512e613 |
/ok to test de4db55 |
/ok to test be78f1b |
/ok to test a0f964d |
/ok to test 19e5709 |
/ok to test 78c8afd |
/ok to test 8f7a054 |
/ok to test 9ad27ac |
/ok to test 08c5579 |
/ok to test 452fced |
/ok to test d6ada33 |
/ok to test f5ffde9 |
/ok to test a676eb3 |
/ok to test f409d19 |
run: | | ||
pip install $(ls ${{ env.CUDA_CORE_ARTIFACTS_DIR }}/*.whl)[test] | ||
pushd ${{ env.CUDA_CORE_CYTHON_TESTS_DIR }} | ||
bash build_tests.sh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to have these built by a more standard mechanism and avoid needing to run a separate bash script. Any reason we wouldn't want these built when invoking pip install
?
Doesn't need to be solved in this PR regardless.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had the same thought, I have some local changes (started on build) I will rebase once this is merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because I don't want to include the built tests in the wheel. If there is a way to achieve this with the build frontend/backend, it'd certainly simplify build-wheel.yml
here, but I couldn't find any.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
Description
cuda.bindings
build time on Linux becomes ~2x slower #642)Previously, the Cython tests are run on
With this PR, the Cython tests are run on
We can no longer run against the previous major version because of the way of building the Cython tests is changed. It is now built against the build-time CTK, which is always the latest major.minor. But I think this is OK, because the same Cython tests are already run in each major version's branch (example from 11.x).
There is still one open task relevant to Cython tests (#274 (comment)), but the requirement there is likely incompatible with either the old or new approach here and always needs a separate job/workflow, so I consider it out of scope.
Checklist