-
Notifications
You must be signed in to change notification settings - Fork 10
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
Use pynvjitlink
for MVC
#23
Use pynvjitlink
for MVC
#23
Conversation
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.
Just had a quick skim and left some initial comments / thoughts.
This would also need the Numba tests from pynvjitlink porting in.
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 for the updates - I just had another pass over this and added some comments on the diff.
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 for the updates - I added a couple more comments on the diff.
Thank you for the review :) I've made some changes that hopefully address your comments. |
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 for the changes - I've gone through another round and have some more thoughts on the diff. CI is failing because the logic for setting config.ENABLE_PYNVJITLINK
is still not quite right.
We also need to port in the tests from pynvjitlink - I suspect there's at least one logic error (to do with adding LTOIR from a file) which the added tests might catch.
f".{ext}") | ||
self.add_file(path, kind) | ||
if isinstance(path_or_code, str): | ||
ext = pathlib.Path(path_or_code).suffix |
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.
Are we missing a bit of logic for handling .ltoir
here? Or has it gone somewhere else? The original I'm looking at is https://github.com/rapidsai/pynvjitlink/blob/a2f23b7c3c237f2cdde3093c845e0453572503eb/pynvjitlink/patch.py#L170-L171
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 realise now the handling for this is taken care of by having LTOIR in the file extension map. See comments below (I'll have to link after posting the review because the links don't exist before I post the review.
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.
The tests now pass, modulo what appears to be an intermittent error on the conda pynvjitlink test job. I do not seem to have permissions to rerun this job. |
Running the testsuite on my local system seems to be deadlocking at:
when pynvjitlink is enabled:
|
Threads:
|
Connected offline with @gmarkall , we concluded the above is probably an issue in |
It was a combination of mismatched versions (cudadevrt from 12.6 with nvJitLink from 12.5) and nvJitLink not handling this situation gracefully. |
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 think this is mergeable. There are some small fixups that I'd like, but would rather push them into a new PR rather than fiddling any more with this, because it's been a heroic effort over a long period of time.
It should also me merged into main
rather than develop
, so I've rebased it and it's in #56 - over there I'm just waiting for CI, then will merge and follow up with the fixes.
Closing as #56 has just been merged. |
- Update the codegen class docstring for LTO. - Simplify / correct some logic in `_readenv()` (`value.lower()` could never be `"True"`, only `"true"`. - Simplify additional flags and linker checks. - Setting `self._linker.complete` in `complete()` is unnecessary, as calling `get_linked_cubin()` sets the link as complete already.
- Update the codegen class docstring for LTO. - Simplify / correct some logic in `_readenv()` (`value.lower()` could never be `"True"`, only `"true"`. - Simplify additional flags and linker checks. - Setting `self._linker.complete` in `complete()` is unnecessary, as calling `get_linked_cubin()` sets the link as complete already.
- Update the codegen class docstring for LTO. - Simplify / correct some logic in `_readenv()` (`value.lower()` could never be `"True"`, only `"true"`. - Simplify additional flags and linker checks. - Setting `self._linker.complete` in `complete()` is unnecessary, as calling `get_linked_cubin()` sets the link as complete already.
This PR attempts to move some of the logic inside the
pynvjitlink
patch.py to work behindconfig.CUDA_ENABLE_MINOR_VERSION_COMPATIBILITY
such that numba may perform the patch if necessary rather thanpynvjitlink
itself.