Skip to content

Toggle numba caching by environment variable #870

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

Merged
merged 1 commit into from
Aug 2, 2022

Conversation

timothymillar
Copy link
Collaborator

@timothymillar timothymillar commented Jul 3, 2022

Fixes #371 and #869

This is just a suggestion and I'm not sure of the best way to test this in CI unless we create an additional runner?

@codecov-commenter
Copy link

codecov-commenter commented Jul 3, 2022

Codecov Report

Merging #870 (01e9763) into main (c13acb1) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #870   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           37        38    +1     
  Lines         3163      3170    +7     
=========================================
+ Hits          3163      3170    +7     
Impacted Files Coverage Δ
sgkit/accelerate.py 100.00% <100.00%> (ø)
sgkit/distance/metrics.py 100.00% <100.00%> (ø)
sgkit/stats/aggregation.py 100.00% <100.00%> (ø)
sgkit/stats/conversion.py 100.00% <100.00%> (ø)
sgkit/stats/hwe.py 100.00% <100.00%> (ø)
sgkit/stats/ld.py 100.00% <100.00%> (ø)
sgkit/stats/pedigree.py 100.00% <100.00%> (ø)
sgkit/stats/popgen.py 100.00% <100.00%> (ø)
sgkit/stats/utils.py 100.00% <100.00%> (ø)
sgkit/utils.py 100.00% <100.00%> (ø)

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@jeromekelleher
Copy link
Collaborator

Totally agree we should do this, but I wonder if it'd be better if we just made our own local jit decorator? This would allow us to put in library-wide defaults for things like cache, as well as give us more fine-grained control of when jitting is done.

@tomwhite
Copy link
Collaborator

tomwhite commented Jul 4, 2022

Totally agree we should do this, but I wonder if it'd be better if we just made our own local jit decorator? This would allow us to put in library-wide defaults for things like cache, as well as give us more fine-grained control of when jitting is done.

Agree - see #371.

@timothymillar
Copy link
Collaborator Author

Ah, thanks I hadn't noticed that issue. I can make a start on custom jit, vectorise and guvectorise decorators.

@jeromekelleher
Copy link
Collaborator

jeromekelleher commented Jul 5, 2022

FWIW, here's what I did for an experimental project downstream of sgkit. You might find parts of it familar 😉

@tomwhite
Copy link
Collaborator

tomwhite commented Jul 5, 2022

Nice! I'm not sure how easy it would be to completely disable numba for vectorize and guvectorize since these wrappers change the type signature in a way that is non-trivial to do in pure python. We don't need that here though (but I'll note that it would be useful for #803) - just a way to enforce numba caching consistently.

@timothymillar
Copy link
Collaborator Author

I've started with some simple wrappers for jit and guvectorize. I ran into this issue numba/numba#8231 when wrapping vectorize, but then realized that the only use of it (mine) was completely unnecessary 🤷. As @tomwhite pointed out, we can't really disable numba when using guvectorize so I'm ignoring that for now. I'm also leaving out cuda for now as I don't have convenient means to test it.

Copy link
Collaborator

@jeromekelleher jeromekelleher left a comment

Choose a reason for hiding this comment

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

LGTM!

@timothymillar
Copy link
Collaborator Author

@tomwhite are you happy that this satisfies #371? Or would you prefer to also handle @cuda now?

@tomwhite
Copy link
Collaborator

tomwhite commented Aug 2, 2022

@timothymillar sorry I was away last week on vacation and have only just got back. I'm fine leaving cuda for now.

@tomwhite tomwhite merged commit 6b3d495 into sgkit-dev:main Aug 2, 2022
@timothymillar
Copy link
Collaborator Author

No worries, thanks @tomwhite

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.

Create a local version of guvectorize
4 participants