-
Notifications
You must be signed in to change notification settings - Fork 47
Add missing numba-cuda dependency to the conda recipe #481
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 missing numba-cuda dependency to the conda recipe #481
Conversation
jameslamb
left a comment
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'm unsure that this is the right fix, left some clarifying questions.
conda/recipes/ucxx/recipe.yaml
Outdated
| run_constraints: | ||
| - cupy >=9.5.0 | ||
| - numba >=0.59.1,<0.62.0a0 | ||
| - numba-cuda >=0.14.0,<0.15.0a00 |
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.
cudf is updating its pinning to >=0.18.0,<0.19.0 in rapidsai/cudf#19604, so this will need to be updated again when that merges (to ensure all of RAPIDS can solve together).
Would it be acceptible to do something like >=0.14.0,<0.19.0 here, so environments that end up with both cudf and ucxx aren't broken? And then tighten it to >=0.18.0,<0.19.0 once that cudf PR is merged? Or is that range too large?
Also, should it be a run: (required) dependency not run_constraints: (optional)? I see that for wheels we only have this as an optional dependency in the [test] extra:
ucxx/python/ucxx/pyproject.toml
Line 44 in d2d9468
| "numba-cuda[cu12]>=0.14.0,<0.15.0a0", |
But it seems looks like it's imported unconditionally:
ucxx/python/ucxx/examples/basic.py
Line 15 in d2d9468
| import numba.cuda |
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.
As a recap, we had to rollback numba-cuda version dependency twice before, immediately this is NOT acceptable IMO. I don’t know if you recall, here's how events unfolded:
Since this is a critical issue, that we’ve been seeing segmentation faults in both RAFT and RMM, I think we first need to go the safe route and make sure everything is back to normal, then we can update the pin to a newer version.
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.
Moved to run section in a57320f .
jameslamb
left a comment
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.
This is looking good, thank you! Merge any time.
|
Thanks @jameslamb and @KyleFromNVIDIA , triggering auto-merge. |
|
/merge |
dependencies.yaml
Outdated
| - pynvml>=12.0.0,<13.0.0a0 | ||
| - output_types: [conda] | ||
| packages: | ||
| - &numba-cuda numba-cuda>=0.14.0,<0.15.0a0 |
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.
Trying to spell this consistently across RAPIDS to make automated updates easier.
| - &numba-cuda numba-cuda>=0.14.0,<0.15.0a0 | |
| - &numba-cuda-dep numba-cuda>=0.14.0,<0.15.0a0 |
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.
@KyleFromNVIDIA @bdice you should agree here. See rapidsai/dask-cuda#1531 (comment) for the identical discussion in the opposite direction.
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.
@bdice Most YAML anchors in dependencies.yaml use underscores and don't have -dep at the end. The existing numba-cuda ones are not consistent with everything else. We should fix the existing ones, not make even more inconsistent ones.
|
I applied my changes (minor renames / typos) since I just saw that |
|
Just for the sake of completeness here about #481 (comment), Kyle agreed we should go forward with the current naming and fix with rapids-reviser later if needed. |
The
numba-cudapackage was made a dependency in #462, but the conda recipe was missed.