fix: allow multiphase modules to be re-imported#5782
Conversation
- When a module is successfully imported and exec'd, save its handle in a dict in the interpreter state - Use a special Py_mod_create slot to look in the cache and return the cached handle if it is in the cache - Don't re-run the user exec function if the module is in the interpreter's cache (implying it was already successfully imported)
rwgk
left a comment
rwgk
left a comment
There was a problem hiding this comment.
Here is my related ChatGPT conversation:
https://chatgpt.com/share/688c74a5-2e08-8008-9c30-daf546a2f9a8
I'm still pretty uncertain about the static inline question, even though ChatGPT strongly suggests keeping the static. We have many helper functions that are not static.
include/pybind11/pybind11.h
Outdated
| A Py_mod_create slot function which will return the previously created module from the cache if one | ||
| exists, and otherwise will create a new module object. | ||
| */ | ||
| static inline PyObject *cached_create_module(PyObject *spec, PyModuleDef *) { |
There was a problem hiding this comment.
I believe this needs to be extern "C", because the function pointer needs to have C linkage.
There was a problem hiding this comment.
I don't think it does, CPython doesn't care what the name of the symbol is only what the pointer is as supplied by the slot.
tests/test_modules.py
Outdated
| assert x is y | ||
|
|
||
|
|
||
| @pytest.mark.xfail("env.GRAALPY", reason="TODO should be fixed on GraalPy side") |
There was a problem hiding this comment.
In case this TODO survives for some time, it'll be useful to have a pointer back here:
reason="TODO should be fixed on GraalPy side (see PR #5782)"
…in the first place.
…esn't have the right properties to check the cache then just allow exec to run.
tests/test_modules.py
Outdated
| assert x is y | ||
|
|
||
|
|
||
| @pytest.mark.xfail("env.GRAALPY", reason="TODO should be fixed on GraalPy side") |
There was a problem hiding this comment.
In case this TODO survives for some time, it'll be useful to have a pointer back here:
reason="TODO should be fixed on GraalPy side (see PR #5782)"
henryiii
changed the title
3 participants
Description
This fixes the exception reported in #5780 and gets things close to the behavior of single-phase module initialization. When import is done on a single-phase module after a
del sys.modules[...], the single-phase module is completely reloaded and reinitialized.I could not get CPython will not do this to multi-phase modules (I tried a few different things).. Instead, this PR keeps a cache of fully created and initialized modules (in the interpreter state dict). When creating a module, we look in the cache first and use the already created and initialized one if it is in there. Otherwise we create a new one. And then also before running the user's exec function we check in the cache.
So, with this PR, the difference is that single-phase modules are reinitialized after a
del, and multi-phase modules silently return a cached handle to the previously initialized instance. I think this difference is probably OK because doing adel sys.modules[...]is probably not very common, and even in this case it was done as part of a test setup.Suggested changelog entry:
PYBIND11_MODULEcode to run again if the module was re-imported after being deleted fromsys.modules.