Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork10.9k
GH-29021: Convert umath_linalg to multi-phase init (PEP 489)#29030
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Conversation
ebfb8a7
to7121db6
Compare7121db6
toeae5c53
Compareeae5c53
to6876e24
CompareThere 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 work on a single PR to reduce maintainer review burden. Once we are settled that we wish to make this change, we can revisit the other ones. Does this impact performance, either import time or runtime?
PyModuleDef_HEAD_INIT, /* m_base */ | ||
"_umath_linalg", /* m_name */ | ||
NULL, /* m_doc */ | ||
0, /* m_size */ |
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.
For now, in a future PR this can be changed.
0,/* m_size*/ | |
-1,/* m_size*/ |
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.
See also my note in#29025 (comment). Briefly, I was mistaken in my understanding of the documentation,-1
is only valid for single-phase initialisation, and is forbidden for multi-phase.
The safest thing reinitialisation-wise would be to use themultiple interpreter opt-out suggested by CPython. This would enforce that each extension module is only imported & initialised once per process.
However, this causestest_reloading.py::test_full_reimport()
to fail. Is that an acceptable trade-off? Multi-phase init (and at some point, isolated modules) opens the door to proper support for reloading/reimportingnumpy
.
cc@encukou re the best way to do a gradual migration to multi-phase
In the case ofumath_linalg
, we might be fine globals-wise, butmultiarray
will definitely need a solution here.
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'vejust updated the HOWTO, seehere before it gets to main docs.
Basically, to allow re-import, clear the "loaded" flag inPyModuleDef
'sm_clear
. Note that thiswill do an actual reload of the module (sans C globals), that is,exec will run. (With single-phase, the module dict contents are stashed away and reused. You could emulatethat, but )
The PyMutex stuff in the updated HOWTO is 3.13+ non-limited API, but isn't necessary in the medium term (on CPython you're protected by the GIL; even free-threading builds use an import lock which isn't likely to go away). For the long-term (or alternate CAPI implementations) you could add no-op shims.
ThePy_MOD_MULTIPLE_INTERPRETERS_NOT_SUPPORTED
flag will only allow loading in the main interpreter; that's redundant if you're limited to one module per process.
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.
However, this causes test_reloading.py::test_full_reimport() to fail. Is that an acceptable trade-off?
I am not opposed to breaking the test/changing the behavior. The ask would be to summarize how things change for:
- Potential
subinterpreter
users (Ithink that just doesn't work at all anymore, but not sure). - Users for
importlib.reload()
, currently they get a warning that this is a bad idea. A full-blown error may not be so bad, though.
59ec898
to6876e24
Compare
Will do. For context, there isa proposal to deprecate support for single-phase initialisation in favour of multi-phase. We're working toimprove the documentation here too. A |
Local timings using
No significant impact of multi-phase on import time. I will investigate running the benchmarks (my laptop is ill-suited to it, though), but there should be no real impact -- this only changes the import process, the end result (initialised module) should be identical performance-wise. A |
Uh oh!
There was an error while loading.Please reload this page.