Use cuda-core for context initialization.#1537
Use cuda-core for context initialization.#1537rapids-bot[bot] merged 4 commits intorapidsai:branch-25.10from
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
/ok to test a65c7fa |
|
/ok to test 2da2e4d |
This uses `cuda.core` in place of the non-optional `numba.cuda` usage, enabling `numba.cuda` to become an optional dependency.
2da2e4d to
0445dd5
Compare
|
/ok to test 0445dd5 |
|
|
||
| import click | ||
| import numba.cuda | ||
| import cuda.core.experimental |
There was a problem hiding this comment.
I'm a little out of the loop here but depending on something with experimental in the name makes me nervous.
There was a problem hiding this comment.
I think it's more of an "experimental API" (you know, like all of Dask's API) rather than experimental functionality, those are essentially CUDA bindings.
There was a problem hiding this comment.
Also, at this point numba-cuda can also be considered experimental in my book, rapidsai/ucxx#462 then posterior need to downgrade it in rapidsai/ucxx#466 and immediately downgrade it again in rapidsai/ucxx#468, as well as NVIDIA/cuda-python#852 are a statement of that.
There was a problem hiding this comment.
From https://nvidia.github.io/cuda-python/cuda-core/latest/api.html#cuda-core-experimental-api-reference
All of the APIs listed (or cross-referenced from) below are considered experimental and subject to future changes without deprecation notice. Once stablized they will be moved out of the experimental namespace.
I would hope we'll get a bit of time to move to cuda.core.Device before .experimental is removed completely, but I can try to do the equivalent operation with the (experimental) cuda.bindings.
There was a problem hiding this comment.
@pentschev I take your point, but this is clearly labelled as experimental and subject to change.
Given that .experimental is in the namespace that's certainly subject to change as it don't be experimental forever.
Perhaps wrapping the calls via a utility would help centralise future changes? It could also be a good place to catch future import errors and raise something more helpful like a link to an issue that describes that this needs changing.
There was a problem hiding this comment.
Right, but if the argument is being experimental, then I would consider all of Dask experimental because everything is subject to change, and has proven to be the case in countless occasions. Dask only avoids the trouble of even labelling anything appropriately, so arguable worse.
I wouldn't mind centralizing it to simplify for future changes, that would be fine. I would still prefer that we use cuda.core.experimental than roll our own re-implementation on top of cuda.bindings, it's way more likely it will be better tested there than what we would do on our own, even though it's experimental. The move out of experimental will still break us at some point but that will be way less maintenance burden than the alternatives.
There was a problem hiding this comment.
We are also likely to catch issues from cuda.core.experimental now, which is good for everyone, it helps us finding problems that will eventually come to bite us early, and in the process we help making cuda.core better earlier.
There was a problem hiding this comment.
I got confirmation from the cuda-python team that pinning to minor versions (e.g., cuda-core=0.3.*) we are safe against breaking changes, and therefore I still think we should go ahead with the changes as they, plus adding a proper cuda-core=0.3.* explicit dependency with the pin.
There was a problem hiding this comment.
c184166 adjusted the pin, so I think we're good. We'll just need to be prepared to update this in the future once things are available in cuda.core rather than cuda.core.experimental.
pentschev
left a comment
There was a problem hiding this comment.
I think we can/should also remove numba-cuda/numba dependencies now, can't we @TomAugspurger ?
|
caed70f removes numba as a required dependency. This would be a behavior change for users who
Those users will now need to add |
Thanks Tom. Given the alternative of potentially having to pursuit the path of switching from |
|
I think this is good now, thanks all! |
|
/merge |
This uses
cuda.corein place of the non-optionalnumba.cudausage, enablingnumba.cudato become an optional dependency.