-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Adopt multi-phase init (PEP 489) #2577
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
base: master
Are you sure you want to change the base?
Conversation
There's errors like:
|
@giampaolo sorry! copy and paste error. I've pushed a commit to fix, please could you approve CI? A |
Since this is all repeated code, I wonder if it makes sense to somehow define it in |
psutil/_psutil_posix.c
Outdated
#ifdef Py_mod_multiple_interpreters // Python 3.12+ | ||
{Py_mod_multiple_interpreters, Py_MOD_PER_INTERPRETER_GIL_SUPPORTED}, |
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.
This pattern doesn't work well with stable ABI extensions. For example, if you compile psutil for cp36+ abi3, it won't have this slot set even when loaded into Python 3.12+.
https://pypi.org/project/psutil/#files
If you want it to work with the stable ABI, I think you need to hard code the constant and dynamically set the slot at runtime (i.e., in PyInit__psutil_posix
).
The same would be true for Py_mod_gil
, but the free threaded build doesn't currently work with the stable ABI, so it's not currently relevant.
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.
@colesbury Indeed, I've thinking about this but haven't come up with any good solutions. The 'simplest' is for psutil to additionally distribute a wheel built with the limited API set to 3.12+, which would double the number of wheels in the medium term, but still be O(1).
One potential solution would be to define two slots arrays, slots_py36
and slots_py312
. We could then choose between them based on the runtime version (though Py_Version
was only added in 3.11, so this might have to go via sys.hexversion
). This would still need to hard-code the symbols that don't exist in Python 3.6, but avoids dynamic manipulation of the arrays.
cc @encukou for any advice you may have here!
A
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 suggest not adding new functionality in this PR. Keep the PyUnstable_Module_SetGIL
call, and remove the new slots for now.
Allowing per-interpreter GIL should be its own PR and discussion. (And preparing a good proposal for psutil devs might not be trivial.)
Alas, limited API does limit you to a minimum Python version. It is not designed to dynamically allow newer features.
We can change that, but this is not the place to do it.
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.
Thanks to Petr for the advice, I've removed the Py_mod_gil
and Py_mod_multiple_interpreters
slots from this PR and added back the PyUnstable_Module_SetGIL()
call. @giampaolo please could you run CI?
A
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.
@giampaolo please could you run CI?
It looks like there are failures 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.
@AA-Turner, the suggestions should fix those.
|
||
static int module_loaded = 0; |
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.
static int module_loaded = 0; |
.m_base = PyModuleDef_HEAD_INIT, | ||
.m_name = "_psutil_windows", | ||
.m_size = sizeof(struct module_state), | ||
.m_methods = mod_methods, |
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.
.m_methods = mod_methods, | |
.m_methods = PsutilMethods, |
cc @giampaolo
xref #2576
The mechanical changes for each module are:
PyMODINIT_FUNC PyInit_{name}(void)
tostatic int {name}_exec(PyObject *mod)
return NULL
to-1
andreturn mod
to0
PyModule_Create
andPyUnstable_Module_SetGIL
.m_slots
memberPyMODINIT_FUNC PyInit_{name}(void)
function, now just with thePyModuleDef_Init()
callA