-
Notifications
You must be signed in to change notification settings - Fork 93
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
CUDA 12 Support #1201
CUDA 12 Support #1201
Conversation
Thanks Ben! 🙏 Would suggest looking at the RMM CUDA 12 PR ( rapidsai/rmm#1223 ) and lining this up with that |
Think we may also want these GHA changes Not seeing any environment files here Also So that may be it |
I agree with all @jakirkham's suggestions above. Maybe also need updates for |
Thanks Bradley! 🙏 Good catch. Yeah this |
I pushed GHA and docs changes. @jakirkham @bdice could you check that the docs is correct, is that supposed to work for both CUDA 11.x and 12.x, or the correct way to document it is CUDA 11.x requires |
Also seems like cuCIM needs to be handled before Dask-CUDA. |
Ok could you please add cuCIM to the list in this issue ( #1115 )? |
I'm not quite sure what's going on in the CUDA 12 tests, it seems Numba is failing to find NVVM files:
@bdice @jakirkham do you know if we're missing something here? Also cc @gmarkall for Numba, in case you have suggestions. |
Yeah This is needed to get |
Shouldn't this be provided when Numba gets installed then? Note that it happens during a test, and I don't think Dask-CUDA should be requiring |
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.
Requesting a small change to use cuda-version
in dependencies.yaml
.
dependencies.yaml
Outdated
@@ -59,6 +59,10 @@ dependencies: | |||
cuda: "11.8" | |||
packages: | |||
- cudatoolkit=11.8 |
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.
Can we change all these entries like the example below? This is needed to constrain the CUDA version of the RAPIDS packages used for Python testing. (It may be possible to derive the correct constraint from a pin on cudatoolkit
but that's going to be less direct for the environment solver.)
- cudatoolkit=11.8 | |
- cuda-version=11.8 | |
- cudatoolkit |
Similar to Edit: Maybe we can handle this on the cuDF side |
Numba is an optional dependency, so the user will certainly need to satisfy that when using Numba+Dask-CUDA but without RAPIDS. I'm guessing we will want to update Numba's installation instructions for CUDA 12 too? |
Yeah that's a good idea. Could you please raise an issue on the Numba repo and cc me? |
@jakirkham raised the issue in numba/numba#9045 |
Adding
It seems that this can't be added as a runtime dependency, is that the case? |
Thanks Peter! 🙏 This could be worked around by adding the That said, it seems reasonable to harden this requirement in |
Thanks @jakirkham , after merging of conda-forge/cuda-nvcc-feedstock#21 all tests now pass! 😄 |
- cudatoolkit | ||
- matrix: | ||
cuda: "12.0" | ||
packages: |
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.
Do we need cuda-nvrtc for Numba?
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.
You can get away without it if you're not linking .cu files with Python kernels or using float16. But for full functionality it should be present.
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.
Added this suggestion below
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! 🙏
This is looking pretty good. Added a minor suggestion to include cuda-nvrtc
based on the thread below
- cudatoolkit | ||
- matrix: | ||
cuda: "12.0" | ||
packages: |
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.
Added this suggestion below
cuda: "12.0" | ||
packages: | ||
- cuda-version=12.0 | ||
- cuda-nvcc |
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.
- cuda-nvcc | |
- cuda-nvrtc |
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.
From what I understand of @gmarkall 's suggestion we don't need cuda-nvrtc
given we're not doing any linkage whatsoever in Dask-CUDA, thus I think we should have only cuda-nvcc
here.
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.
Ok will leave this here and we can revisit as needed
/merge |
Thanks all! 🙏 |
Thanks everyone for reviews! 😄 |
Adds CUDA 12 Support to build matrix
Fixes #1115