-
Notifications
You must be signed in to change notification settings - Fork 55
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
Add CUDA 12.0 build matrix #967
Conversation
We're seeing the following error for CUDA 12:
I'm not actually sure what's missing or done wrong here, I've compared to cuDF's |
If this is an error in a conda build, check on meta.yaml. This line probably needs to be conditional on CUDA 11. ucx-py/conda/recipes/ucx-py/meta.yaml Line 42 in 869c3f6
|
@bdice I looked at that, but cuDF seems to do it the same way, no conditionals, it does have some conditionals but those are for the |
Could you please try dropping It's already pulled in by UCX when it is needed Edit: Filed an issue about cuDF ( rapidsai/cudf#13613 ) |
@pentschev That’s a bug in cudf’s recipe but we disable recipe tests so it isn’t seen. https://github.com/rapidsai/cudf/blob/0b4e3543fb35a6f8bbae3ad3e07b544c534901ac/ci/build_python.sh#L19 This package isn’t skipping the tests so you do see a failure here. |
Be sure that all the CUDA 11 test configurations pull in their target version. That’s why this specific test pinning exists — otherwise all the CI jobs for 11.2, 11.5, 11.8 would install any compatible version like |
We could use |
No,
Also not sure what this comment relates to, are we supposed to add it somewhere else besides https://github.com/rapidsai/ucx-py/pull/967/files#diff-5475a6d76de4c506ee92cf6f941bba30a6a07b5881cfea531d42a6ec035095a6R77 ? |
Try replacing this line in meta.yaml: ucx-py/conda/recipes/ucx-py/meta.yaml Line 42 in 869c3f6
with - cuda-version ={{ cuda_version }} |
Thanks @bdice and @jakirkham , with the latest suggestion tests are passing now. Do you think we're good here and should move ahead on merging this, or are there further changes required? |
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.
Thanks Peter! 🙏
Had a couple comments below
CUDA is not a hard-dependency, thus we should remove it.
Hi @pentschev could you please link this PR to #927 either by editing in closing keywords to the description, or through the UI? Thanks! |
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.
Seems fine, one minor comment on the open conversation about dependencies.yaml
but nothing blocking.
Woohoo! Thanks all for the reviews here! 😄 |
/merge |
Thanks Peter and thanks everyone for reviewing! 🙏 |
Closes #927