-
Notifications
You must be signed in to change notification settings - Fork 217
test_nvvm.py simplification / use llvmlite
in toolshed/
only
#1047
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
Conversation
Auto-sync is disabled for ready for review pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
Holding off running tests until after reviews. |
from cuda.bindings import nvvm | ||
|
||
|
||
def get_minimal_nvvmir_txt_template(): |
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't you just check in a JSON file that contains the bitcode? Why do we need the sys.path
munging and print
-generated dictionary?
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.
Where would you put the JSON file?
I had on my mind: Avoid creating extra artifacts.
But that's something I could do if you prefer.
Re printing out the generated dictionary: I don't want to over-engineer a helper script that's used only rarely (possibly only every couple years). The main goal is to archive "how it worked last time", so that future maintainers don't have to start from scratch. (Back in February it took me more than a couple hours to figure out the approach.)
"Static bitcode for NVVM IR version " | ||
f"{major}.{debug_major} is not available in this test.\n" | ||
"Maintainers: Please run the helper script to generate it and add the " | ||
"output to the MINIMAL_NVVMIR_BITCODE_STATIC dict:\n" |
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 don't follow why we can't make llvmlite
a required testing dependency and always generate the bitcode and eliminate the static testing fixture altogether.
Then we can avoid a lot of this complexity.
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 mutually exclusive with my JSON file suggestion.
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 don't follow why we can't make
llvmlite
a required testing dependency and always generate the bitcode and eliminate the static testing fixture altogether.
Ah, that's whole point of this PR: remove llvmlite
even as an optional test dependency.
@leofang please correct me if I'm wrong, but I got the impression you were skeptical of the llvmlite
dependency all the while.
Originally I wanted to keep things simple and make llvmlite
a required dependency.
Before we had the recent llvmlite
v0.45
related breakage (see #988 and numba/llvmlite#1297), I had it on my list to simplify the test_nvvm.py
code, with llvmlite
as a hard dependency.
But after the breakage, and seeing how @rparolin stumbled even over the optional dependency last week (see team chat), I got to think it's more trouble than it's worth to even have it as an optional dependency.
With this PR, the cuda_bindings
unit tests will be fully isolated from llvmlite
, so we won't stumble that much in the future.
Only every couple years probably someone has to dust off the helper script to add a new entry in test_nvvm.py
. Estimated effort: 30-60 minutes, depending on prior background of the person who takes this on.
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.
Won't we still need llvmlite
eventually to regenerate the bitcode?
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.
Yes, but only "offline", in the sense that the routine cuda_bindings
unit tests (CI) don't need it.
Based on the few months of related experience that I have:
- I expect that
test_nvvm.py
will only break if we're testing new CTK releases. - Fixing up
test_nvvm.py
will be just one of potentially several adjustments we have to make for new CTK versions. - Chances of
test_nvvm.py
breakages due to minor-version CTK releases are probably small. — Every couple of months roughly. - Chances of
test_nvvm.py
breakages due to major-version CTK releases are significant. — Every couple years.
Making llvmlite
a required dependency means that we'll be living at the bleeding edge: random breakages at random times (from NVIDIA's viewpoint) that need immediate attention.
With the static bitcode inputs, we'll only need to tend to the test when there are CTK changes (release schedule controlled by NVIDIA).
For completeness: There are other ways to convert the txt
version to bitcode
. I don't remember exactly, I believe there are llvm commands that could be used instead. What's most suitable might change in the future, but I'm guessing as long as numba-cuda
uses llvmlite
, it'll be a good choice.
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 really dislike the manual copy paste aspect of this, but it seems like we're content to leave it as is.
/ok to test |
This comment has been minimized.
This comment has been minimized.
Thanks @cpcloud! |
|
* Remove bitcode_dynamic code from test_nvvm.py * New toolshed/build_static_bitcode_input.py * Import test_nvvm to get access to MINIMAL_NVVMIR_TXT (to avoid duplicating it). * Rename MINIMAL_NVVMIR_TXT → MINIMAL_NVVMIR_TXT_TEMPLATE for clarity. * Minor simplifications of helper script. (cherry picked from commit 5383cb5)
Successfully created backport PR for |
…) (#1053) * Remove bitcode_dynamic code from test_nvvm.py * New toolshed/build_static_bitcode_input.py * Import test_nvvm to get access to MINIMAL_NVVMIR_TXT (to avoid duplicating it). * Rename MINIMAL_NVVMIR_TXT → MINIMAL_NVVMIR_TXT_TEMPLATE for clarity. * Minor simplifications of helper script. (cherry picked from commit 5383cb5) Co-authored-by: Ralf W. Grosse-Kunstleve <rwgkio@gmail.com>
This fully isolates us from
llvmlite
for regularcuda_bindings
unit testing.llvmlite
needs to be installed locally only when the newtoolshed/build_static_bitcode_input.py
helper script is needed, which is expected to be rare.See also:
llvmlite
fromtest
dependencies incuda_bindings/pyproject.toml
#988"-arch=compute_100"
tonvvm.compile_program
options and restore testing with llvmlite-generated IRs #990 (obsolete)