Use pynvjitlink for CUDA 12+ MVC - #13650
Conversation
brandon-b-miller
added
feature request
|
Added a "Fixes" note in the OP. @brandon-b-miller please double check if that is correct |
brandon-b-miller
changed the title
nvjitlink for CUDA 12+ MVCpynvjitlink for CUDA 12+ MVC
|
/ok to test |
|
/ok to test |
|
|
||
|
|
||
| _patch_numba_mvc() | ||
| patch_numba_linker_cuda_11() |
There was a problem hiding this comment.
This seems to be unconditionally using CUDA 11 patching logic in tests. We should use CUDA 12 patching logic when on CUDA 12, right?
There was a problem hiding this comment.
I think this whole test skips in a CUDA 12 environment based on the below
@pytest.mark.skipif(
not IS_CUDA_11, reason="Minor Version Compatibility test for CUDA 11"
)
There was a problem hiding this comment.
We should probably add a test for CUDA 12 MVC eventually but I don't think any of our CI jobs would cover that case yet sadly.
There was a problem hiding this comment.
The MVC tests should be enabled for CUDA 12, even if we don't have an MVC-triggering environment in CI with the drivers/runtimes we use. We want to be able to run pytest on a system requiring MVC and know that it's being covered by cudf.
There was a problem hiding this comment.
It's okay to have them be skipped if on CUDA 12 and MVC is needed and pynvjitlink isn't available.
There was a problem hiding this comment.
Ok, that makes sense. I'm thinking I'll rename this file test_mvc and have a separate test for cuda 11 and cuda 12. updates to follow.
Co-authored-by: Bradley Dice <bdice@bradleydice.com>
|
/ok to test |
|
/ok to test |
|
/ok to test |
bdice
left a comment
bdice
left a comment
There was a problem hiding this comment.
A few cleanups -- but this is looking close to right. Have you verified that the tests run locally as expected with pynvjitlink?
Co-authored-by: Bradley Dice <bdice@bradleydice.com>
|
/ok to test |
Co-authored-by: Bradley Dice <bdice@bradleydice.com>
|
/ok to test |
For me at least tests pass locally building from source with a 12.0 driver and 12.3 CTK. I'll also do a round of follow up testing on the tip of 23.12 after the merge. |
|
/ok to test |
| def patch_numba_linker_pynvjitlink(): | ||
| warnings.warn( | ||
| "Minor version compatibility requires pynvjitlink. " | ||
| "Please pip install pynvjitlink to proceed using cuDF." |
There was a problem hiding this comment.
Depending on the timing of pynvjitlink being shipped, we'll want to do one of the following:
- If pynvjitlink isn't shipped by the time we release, we should remove this note and say something like before (explain the MVC problem and direct users to match their driver and toolkit)
- If pynvjitlink is available in time, we should give the full install command, like
pip install pynvjitlink --extra-index-url=https://pypi.nvidia.com
We don't want to leave this in a halfway state where we tell users to install pynvjitlink but don't specify the index, especially if the package isn't yet available.
There was a problem hiding this comment.
How do you feel about keeping the original warning to be safe then. The changes will still allow pynvjitlink to work once it's out, but we won't document it or direct users to it in any official way until then.
There was a problem hiding this comment.
Yes, do that. We can change the warning during code freeze if we ship pynvjitlink in time.
There was a problem hiding this comment.
Approving with one request for a final change based on the status/timeline of pynvjitlink wheels being published: #13650 (comment)
|
/ok to test |
|
/ok to test |
brandon-b-miller
added
the
5 - Ready to Merge
|
/merge |
5 participants
Fixes #12822
This PR provides minor version compatibility in the CUDA 12.x range through
nvjitlinkvia the preliminary nvjiitlink python binding. Thus far this PR merely leverages a local installation of the library and should not be merged untilnvjitlinkis hosted onconda-forgeand cuDF's dependencies are adjusted accordingly, likely as part of this PR.