PyImport_AddModuleRef is questionable #106915
Comments
I looked for usage of PyImport_AddModule() and I was surprised by the results: It's used for other modules than
I'm not sure of the purpose of this issue. What do you propose? |
|
OK, I've analyzed the usages of ⚠ Creating a new moduleThe following are (as far as I can tell) meant to create a new module. CFFI (imports a module "from" literal C source using OpenCV (initializes submodules this way): pybind11: uwsgi: others: ⚠ Importing the standard libraryThe module is assumed to already exist and be loaded. ⚠ Importing existing moduleHere, the module is assumed to already exist and be loaded.
Projects importing themselves: ✔ NamespaceThese create a module that's initially empty, and is filled with attributes. This is a valid use case, if somewhat niche. Cython: SWIG runtime data: ✔ Importing
|
|
Based on this, I believe it would be better to add the following, int PyImport_ImportOrAddModule(char* name, PyObject **result);
@vstinner would that make sense to you? Usage examples: Creating a new modulePyObject *mod;
int result = PyImport_ImportOrAddModule("__main__", &mod);
if (result < 0) {
goto error;
}
if (result == 0) {
... initialize `mod` ...
// (on error, the module should be removed from sys.modules!)
}Importing
|
|
@vstinner would that make sense to you? |
|
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. |
|
@encukou, wouldn't it make for cleaner APIs if we provide two separate APIs for each use case (1. create module, 2. import |
|
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. |
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.
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.
Yeah, a better name describing the |
|
@vstinner do you still insist the function needs to be added to the stable ABI? |
Remove PyImport_AddModuleRef() function.
|
I dislike I proposed PR gh-111559 to add |
|
Thank you!
It's long but precise. IMO, What about |
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:
|
|
Well, I guess this should be discussed in the c-api working group, once that's formed. |
Remove PyImport_AddModuleRef() function.
|
Since no one proposed better name, I modified my PR to rename the function to |
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.
Linked PRs
The text was updated successfully, but these errors were encountered: