Skip to content
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

Avoid deprecated generated_jit #784

Merged
merged 1 commit into from Dec 5, 2023

Conversation

JelleAalbers
Copy link
Member

What is the problem / what does the code in this PR do
Avoid a NumbaDeprecationWarning and a future crash from using the deprecated function numba.generated_jit.

Can you briefly describe how it works?
numba.generated_jit is deprecated: https://numba.readthedocs.io/en/stable/reference/deprecation.html#deprecation-of-generated-jit. I think most people don't see the warning message for this yet because our environments use numba versions that are more than a year old.

We use generated_jit in strax.endtime to condition its behavior on the dtype of its input. This PR replaces generated_jit with numba.extending.overload. An additional benefit is that we don't need to condition on the environment variable NUMBA_DISABLE_JIT anymore, this was one of the main reasons generated_jit was deprecated.

@coveralls
Copy link

coveralls commented Dec 5, 2023

Coverage Status

coverage: 91.261% (-0.3%) from 91.539%
when pulling cbec3a8 on JelleAalbers:fix_generated_jit
into 21cc96e on AxFoundation:master.

@JelleAalbers JelleAalbers marked this pull request as ready for review December 5, 2023 14:15
@dachengx
Copy link
Collaborator

dachengx commented Dec 5, 2023

Thanks @JelleAalbers . Just to check my understanding. Now outside numba decorated functions, like barely call strax.endtime will not trigger the usage of _overload_endtime. This is different from before but is exactly the functionality of numba.extending.overload. Am I correct? I test with some toy functions and it seems yes.

@dachengx dachengx self-requested a review December 5, 2023 15:17
@JelleAalbers
Copy link
Member Author

Hi Dacheng -- thanks for the quick look! Yes, I didn't consider it, but if you call endtime from a non-jitted function you will get the regular non-jitted function after this PR, whereas previously you always got a jitted function (unless you set NUMBA_DISABLE_JIT).

Here that should be fine, the regular endtime is actually faster that its jitted cousin when called on its own. The overhead of switching into numba is more than that of the little bit of python attribute access endtime does. We just jit endtime because we need to call it from jitted functions, not because it speeds things up.

But if we had some complicated performance-critical function (that just has different implementations for different input types), the generated_jit behavior would have been nicer. There is a numba discussion on this here: numba/numba#8897.

Copy link
Collaborator

@dachengx dachengx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should be fine with the performance if we always put complex calculations into numba jit. Thanks again!

@dachengx dachengx merged commit 8c54d5b into AxFoundation:master Dec 5, 2023
11 checks passed
@JelleAalbers JelleAalbers deleted the fix_generated_jit branch December 5, 2023 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants