Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
cuDF numba cuda 12 updates #13337
cuDF numba cuda 12 updates #13337
Changes from 8 commits
76109ce
de2b678
442fefc
f5f915d
19dd82c
d360008
9c76c61
950f98f
c8142ea
8c7bae8
c4edd0e
b8d290d
a50c642
6ba957c
96b6f01
47d8a2e
66226c6
b9634f9
7a594b3
cf642d0
e7b49e9
cb5a756
dcc73e1
053193a
c2285fa
b72eef0
bd27a2f
8c9c070
662b30b
93af613
2ff5c5d
5cb0ce6
fc69663
0f1079c
0797cde
e799992
41e92a9
8839f8c
c27a4b1
6925612
439a667
1bfb382
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Any magic in the import order is usually good to explicitly describe for readers.
Is it dangerous / invalid if a user imports
numba.cuda
in their own code before importing cudf?numba
import?)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.
My interpretation of @brandon-b-miller's response here was that in fact there is no longer an explicit ordering issue because we are using the numba config API rather than an environment variable. If there is still an ordering issue then I remain confused about my question in the other thread because it seems like we would be breaking the ordering there.
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.
There's only an ordering issue if we use the config option from numba 0.57. I believe the linker patching approach (which is taken in this PR at this point) is not perturbed by when
numba.cuda
is imported.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.
So this is still an item of concern because we've pivoted back to putting numba 0.57 into this PR. Currently the call to
_setup_numba
is placed very early on in cudf's__init__
even beforevalidate_setup()
is called, asvalidate_setup()
callsrmm._gpu.runtimeGetVersion()
which actually importsnumba.cuda
as a side effect.The order of imports does matter here. In addition there's @bdice 's observation that a user importing
numba.cuda
somewhere in their workflow beforecudf
is imported is likely to break cuDF. So certainly this is an issue we need to come to some consensus about before merging.One option I can think of that could work is detecting this case dynamically somehow and issuing a warning to the user that instructs them to set
CUDA_ENABLE_MINOR_VERSION_COMPATIBILITY=1
in their environment. This should set the variable globally before the user's import.