ENH: Implement per-module state for lapack_lite to avoid global static states#31928
Open
prathamhole14 wants to merge 2 commits into
Open
ENH: Implement per-module state for lapack_lite to avoid global static states#31928prathamhole14 wants to merge 2 commits into
prathamhole14 wants to merge 2 commits into
Conversation
Member
|
Please ensure everything passes locally before making a PR. Running CI isn't free; and the maintainers get a notification for each commit regardless of whether it's a draft. Also; AI disclosure |
Contributor
Author
|
Sorry I should have provided context from the start, I did compile and ran tests locally and they passed but since this PR tries to adopt to limited api I was afraid that it might fail on other systems. This PR is related to #31913. |
Member
|
Ah actually the CI failure isn't your fault; see #31932 |
prathamhole14
commented
Jul 9, 2026
| lapack_lite_exec(PyObject *m) | ||
| lapack_lite_traverse(PyObject *m, visitproc visit, void *arg) | ||
| { | ||
| lapack_lite_state *state = (lapack_lite_state *)PyModule_GetState(m); |
Contributor
Author
There was a problem hiding this comment.
I did not use get_lapack_lite_state since state here can actually be NULL unlike in check_object
Member
|
Out of curiosity, why is this needed? I don't think per-module state is needed for limited API support. Or are you also working on subinterpreter support? |
Member
|
That would be my "fault".Yes, the goal of the internship is both tasks, as a modernization of the internal NumPy code base,and to see if these types of changes affect performance. |
Contributor
Author
Yes it's not "requirement" for limited api but since CPython is moving towards subinterpreters, @mattip and I discussed to start this effort with moving to per-module state. |
Contributor
Author
I ran the benchmarks ( |
da-woods
reviewed
Jul 10, 2026
Contributor
|
My basic (untested) intuition about performance is that looking things up in the module state will almost certainly be slower than looking things up from a static global. But in this case it's only an exception that will almost never be used so it shouldn't make a huge difference |
902a8e8 to
070942a
Compare
prathamhole14
commented
Jul 12, 2026
|
|
||
| #ifdef HAVE_BLAS_ILP64 | ||
| PyDict_SetItemString(d, "_ilp64", Py_True); | ||
| if (PyModule_AddObjectRef(m, "_ilp64", Py_True) < 0) { |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR summary
Related to: #31930
AI Disclosure
Only for some research and improving comments.