Repository navigation
PyImport_AddModuleRef is questionable #106915
Description
The function PyImport_AddModuleRef, which was recently added to the stable ABI, is questionable.
It returns an already loaded module, or a new empty module, but doesn't give any indication of which one of those it did. IMO, that means you can't really use it for anything except a module that's supposed to stay empty.
It is not clear to me if this function actually useful for anything except the __main__ module.
Maybe it would be more useful as a recipe in the docs that users could adapt to their needs, rather than API. Or it should be changed to make it easier to use safely.
Activity
vstinner commentedon Jul 20, 2023
I looked for usage of PyImport_AddModule() and I was surprised by the results:
#105922 (comment)
It's used for other modules than
__main__.I'm not sure of the purpose of this issue. What do you propose?
encukou commentedon Aug 21, 2023
OK, I've analyzed the usages of
PyImport_AddModuleyou posted in #105922 (comment)⚠ Creating a new module
The following are (as far as I can tell) meant to create a new module.
If the module already existed, it would be a bug -- usually some subsequent code would reload/reinitialize the module.
Presumably the code guards against that somehow, but the API could be much more helpful (at least signal that the module was already loaded).
CFFI (imports a module "from" literal C source using
PyRun_String):OpenCV (initializes submodules this way):
pybind11:
uwsgi:
others:
⚠ Importing the standard library
The module is assumed to already exist and be loaded.
Most of these cases should use
PyImport_ImportModuleinstead.⚠ Importing existing module
Here, the module is assumed to already exist and be loaded.
These should also use
PyImport_ImportModuleinstead.jpype.protocolis a.pyfile:Projects importing themselves:
✔ Namespace
These create a module that's initially empty, and is filled with attributes.
This is a valid use case, if somewhat niche.
Such a private namespace seems useful for code generators.
Cython:
SWIG runtime data:
✔ Importing
__main__Another legit but niche use case. The
__main__module is initially empty, soPyImport_AddModuledoes the right thing.□ Not relevant.
Comments:
Re-exports & definitions:
Generated code:
encukou commentedon Aug 21, 2023
Based on this, I believe it would be better to add the following,
instead of
PyImport_AddModuleRef:*resultand return 1.*resultand return 0.@vstinner would that make sense to you?
Usage examples:
Creating a new module
Importing
__main__or another “namespace” moduleImporting stdlib or a module that should be already loaded
Use
PyImport_ImportModuleinstead.encukou commentedon Sep 5, 2023
@vstinner would that make sense to you?
vstinner commentedon Sep 5, 2023
Yes, your proposed API makes sense, as soon as the caller can decide to treat 0 and 1 the same way, the migration should be smooth.
Would it be possible to keep PyImport_AddModuleRef() name, to have a smoother transition (the new name would be easier to discover)? I don't have a strong opinion on names, but your proposed name sounds too long and I'm surprised to have "Or" in its name. If you are fine with keeping the name, I can write a PR to maje the API change.
erlend-aasland commentedon Sep 6, 2023
@encukou, wouldn't it make for cleaner APIs if we provide two separate APIs for each use case (1. create module, 2. import
__main__or namespace module)? Making one API do two things is seldom a good idea IMO.vstinner commentedon Sep 6, 2023
My worry is the adoption of a new API if it's too different to the API that it replaces, or too complicated to use.
PyImport_AddModule() is weird, but it has its user base. For me, the most surprising is to see that it's used to get a module already loaded!
Maybe we should put a different name and fail if the module already exists. I don't know.
encukou commentedon Sep 7, 2023
I'd prefer not doing that. Few projects use it correctly, so if we're already asking the maintainers to update their code (add DECREF), I think it's reasonable to switch to the slightly more complex alternative.
Could the deprecation warning mention the intended replacement, to make it more discoverable?
For 1. (create new module, which needs to be initialized), you'd use the API I proposed. You need the 3-way return to tell whether the module needs initialization.
Once we have that API, 2. (
__main__or namespace module) becomes a special case of 1. -- you simply always skip the initialization.For 2.,
PyImport_AddModuleRefactually does the right thing, and is a bit easier to use -- but IMO this case is not common enough to get a shortcut. (Here I'm mostly worried about the docs: explaining when should you use one or the other. Skipping the(result == 0)initialization branch if it doesn't apply to you is straightforward.)Yeah, a better name describing the
getdefault- style functionality would be nice.IMO
PyImport_ImportOrAddModuleis more likely to make people read the docs, rather than assume behaviour from the name alone.encukou commentedon Oct 23, 2023
@vstinner do you still insist the function needs to be added to the stable ABI?
pythongh-106915: Add PyImport_CreateModule() function
vstinner commentedon Oct 31, 2023
I dislike
PyImport_ImportOrAddModule()name, it's just too long :-)I proposed PR gh-111559 to add
int PyImport_CreateModule(name, &module)function and removemodule = PyImport_AddModuleRef(name)function.encukou commentedon Nov 2, 2023
Thank you!
It's long but precise. IMO,
PyImport_CreateModuleis confusing: it doesn't always create the module -- sometimes it imports the existing one.What about
PyImport_ImportOrCreate?vstinner commentedon Nov 2, 2023
It's now harder to miss the fact that the module can be not created if it already exists with the new API, since the information is in the return value:
int PyImport_CreateModule(const char *name, PyObject **module)encukou commentedon Nov 2, 2023
Well, I guess this should be discussed in the c-api working group, once that's formed.
pythongh-106915: Add PyImport_ImportOrAddModule() function
vstinner commentedon Nov 7, 2023
Since no one proposed better name, I modified my PR to rename the function to
PyImport_ImportOrCreate(). I also updated the doc to explain that first the function tries to get an already imported module, and only create a new module if it doesn't exist (to better stick to the new function name).vstinner commentedon Jan 27, 2025
Python 3.13 was released with
PyImport_AddModuleRef()in October 2024. I close the issue.