-
Notifications
You must be signed in to change notification settings - Fork 32
Fix eager compilation with signature for dppy.kernel #291
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
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.
Please fix CI on Windows.
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 test AOT somehow?
@reazulhoque please merge with main. It will fix black. |
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 am continuing to 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.
Much better. I have made remarks with useful links describing how to make better.
@@ -31,11 +32,28 @@ def filter_str(request): | |||
return request.param | |||
|
|||
|
|||
def skip_if_win(): | |||
if sys.platform in ["win32", "cygwin"]: |
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.
https://stackoverflow.com/questions/1325581/how-do-i-check-if-im-running-on-windows-in-python
it shows that it is better to rely on os.name == 'nt'
.
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 question directs to an already answered question which suggests sys.platform
.
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.
platform.system() == "Windows"
This PR does not fix #75. Because the test uses |
@PokhodenkoSA, this PR solves #75. There are multiple tests that needed to be updated, which test are you talking about when you mention I have updated tests with signatures that will trigger eager compilation. I asked Numba team specifically if they have tests for eager compilation and they mentioned they do not. I decided to not write specific tests following that conversation. When I add signature to
|
I agree with test changes. Need CI fix and use |
Closes #75
This will fix the seg-fault issue when signature was provided with
dppy.kernel
.Eager compilation is still Just-In-Time compilation. Instead of the compilation happening at the call site it happens at the definition site. We have caching mechanism for already compiled kernels that takes the SYCL queue in account. This
mechanism can be potentially used to recompile a kernel that has been compiled for a different backend (Sycl queue).
Steps taken to fix this: