search

python / cpython

The Python programming language
https://www.python.org
Other
62.69k stars 30.06k forks source link

C API: Add PyImport_AddModuleRef() function #105922

Closed vstinner closed 1 year ago

vstinner commented 1 year ago

The C API PyImport_AddModule() returns a borrowed reference using a special dance added by commit https://github.com/python/cpython/commit/4db8988420e0a122d617df741381b0c385af032c of issue #86160:

        PyObject *ref = PyWeakref_NewRef(mod, NULL);
        Py_DECREF(mod);
        if (ref == NULL) {
            return NULL;
        }
        mod = PyWeakref_GetObject(ref);
        Py_DECREF(ref);

Borrowed references are bad:

I proposed to:

Linked PRs

erlend-aasland commented 1 year ago

The PRs are merged; looks like this can be closed.

vstinner commented 1 year ago

The PRs are merged; looks like this can be closed.

Well, this part is not done yet:

Deprecate PyImport_AddModule() and PyImport_AddModuleObject()

vstinner commented 1 year ago

I added PyImport_AddModuleRef() to pythoncapi-compat: commit https://github.com/python/pythoncapi-compat/commit/8ba8db366f51b3d8883c4e4bf4709394b418e688

encukou commented 1 year ago

Is this function actually useful for anything except the __main__ module? 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.

Would it be better to deprecate the old functions without direct replacement, and add PyImport_AddModuleRef as a recipe in the docs that users could adapt to their needs?

Or, should we change the function to indicate whether the returned module is new or not?

vstinner commented 1 year ago

@encukou:

Is this function actually useful for anything except the main module?

I'm not sure. Here is a code search on PyPI top 5,000 projcts: 31 projects use it.

cffi-1.15.1.tar.gz: cffi-1.15.1/cffi/_cffi_errors.h: m = PyImport_AddModule("_cffi_error_capture");
matplotlib-3.7.1.tar.gz: matplotlib-3.7.1/src/_macosx.m: if (!(builtins = PyImport_AddModule("builtins"))  // borrowed.
Cython-0.29.34.tar.gz: Cython-0.29.34/Cython/Compiler/ModuleNode.py: '%s = PyImport_AddModule(__Pyx_BUILTIN_MODULE_NAME); %s' % (
Cython-0.29.34.tar.gz: Cython-0.29.34/Cython/Compiler/ModuleNode.py: '%s = PyImport_AddModule((char *) "cython_runtime"); %s' % (
Cython-0.29.34.tar.gz: Cython-0.29.34/Cython/Compiler/ModuleNode.py: '%s = PyImport_AddModule("%s"); %s' % (
Cython-0.29.34.tar.gz: Cython-0.29.34/Cython/Includes/cpython/module.pxd: PyObject* PyImport_AddModule(const char *name) except NULL
Cython-0.29.34.tar.gz: Cython-0.29.34/Cython/Utility/CommonStructures.c: fake_module = PyImport_AddModule((char*) "_cython_" CYTHON_ABI);
Cython-0.29.34.tar.gz: Cython-0.29.34/Cython/Utility/CommonStructures.c: fake_module = PyImport_AddModule((char*) "_cython_" CYTHON_ABI);
Cython-0.29.34.tar.gz: Cython-0.29.34/Cython/Utility/ModuleSetupCode.c: abi_module = PyImport_AddModule(__Pyx_FastGIL_ABI_module);
cloudpickle-2.2.1.tar.gz: cloudpickle-2.2.1/tests/cloudpickle_testpkg/_cloudpickle_testpkg/mod.py: # it will use the Python/C API ``PyImport_AddModule(submodule_qualified_name)``
cloudpickle-2.2.1.tar.gz: cloudpickle-2.2.1/tests/cloudpickle_testpkg/build/lib/_cloudpickle_testpkg/mod.py: # it will use the Python/C API ``PyImport_AddModule(submodule_qualified_name)``
orjson-3.8.10.tar.gz: orjson-3.8.10/local_dependencies/pyo3-ffi/src/import.rs: pub fn PyImport_AddModuleObject(name: *mut PyObject) -> *mut PyObject;
orjson-3.8.10.tar.gz: orjson-3.8.10/local_dependencies/pyo3-ffi/src/import.rs: #[cfg_attr(PyPy, link_name = "PyPyImport_AddModule")]
orjson-3.8.10.tar.gz: orjson-3.8.10/local_dependencies/pyo3-ffi/src/import.rs: pub fn PyImport_AddModule(name: *const c_char) -> *mut PyObject;
orjson-3.8.10.tar.gz: orjson-3.8.10/include/pyo3/pyo3-ffi/src/import.rs: pub fn PyImport_AddModuleObject(name: *mut PyObject) -> *mut PyObject;
orjson-3.8.10.tar.gz: orjson-3.8.10/include/pyo3/pyo3-ffi/src/import.rs: #[cfg_attr(PyPy, link_name = "PyPyImport_AddModule")]
orjson-3.8.10.tar.gz: orjson-3.8.10/include/pyo3/pyo3-ffi/src/import.rs: pub fn PyImport_AddModule(name: *const c_char) -> *mut PyObject;
sentencepiece-0.1.98.tar.gz: sentencepiece-0.1.98/src/sentencepiece/sentencepiece_wrap.cxx: PyObject *module = PyImport_AddModule("swig_runtime_data" SWIG_RUNTIME_VERSION);
opencv-python-4.7.0.72.tar.gz: opencv-python-4.7.0.72/opencv/modules/python/src2/cv2.cpp: submodule = PyImport_AddModule(full_submodule_name.c_str());
opencv-python-4.7.0.72.tar.gz: opencv-python-4.7.0.72/opencv/modules/python/src2/cv2.cpp: /// Return `PyImport_AddModule` NULL with an exception set on failure.
JPype1-1.4.1.tar.gz: JPype1-1.4.1/native/common/include/jp_primitive_accessor.h: PyObject *typing = PyImport_AddModule("jpype.protocol");
JPype1-1.4.1.tar.gz: JPype1-1.4.1/native/common/jp_booleantype.cpp: PyObject *typing = PyImport_AddModule("jpype.protocol");
JPype1-1.4.1.tar.gz: JPype1-1.4.1/native/common/jp_booleantype.cpp: PyObject *typing = PyImport_AddModule("jpype.protocol");
JPype1-1.4.1.tar.gz: JPype1-1.4.1/native/common/jp_classhints.cpp: PyObject *typing = PyImport_AddModule("jpype.protocol");
JPype1-1.4.1.tar.gz: JPype1-1.4.1/native/common/jp_classhints.cpp: PyObject *typing = PyImport_AddModule("jpype.protocol");
JPype1-1.4.1.tar.gz: JPype1-1.4.1/native/common/jp_classhints.cpp: PyObject *typing = PyImport_AddModule("jpype.protocol");
JPype1-1.4.1.tar.gz: JPype1-1.4.1/native/common/jp_classhints.cpp: PyObject *typing = PyImport_AddModule("jpype.protocol");
JPype1-1.4.1.tar.gz: JPype1-1.4.1/native/common/jp_classhints.cpp: PyObject *typing = PyImport_AddModule("jpype.protocol");
JPype1-1.4.1.tar.gz: JPype1-1.4.1/native/common/jp_context.cpp: JPPyObject import = JPPyObject::use(PyImport_AddModule("importlib.util"));
JPype1-1.4.1.tar.gz: JPype1-1.4.1/native/common/jp_functional.cpp: PyObject *typing = PyImport_AddModule("jpype.protocol");
pybind11-2.10.4.tar.gz: pybind11-2.10.4/pybind11/include/pybind11/pybind11.h: handle submodule = PyImport_AddModule(full_name.c_str());
opencv-python-headless-4.7.0.72.tar.gz: opencv-python-headless-4.7.0.72/opencv/modules/python/src2/cv2.cpp: submodule = PyImport_AddModule(full_submodule_name.c_str());
opencv-python-headless-4.7.0.72.tar.gz: opencv-python-headless-4.7.0.72/opencv/modules/python/src2/cv2.cpp: /// Return `PyImport_AddModule` NULL with an exception set on failure.
onnx-1.13.1.tar.gz: onnx-1.13.1/third_party/pybind11/include/pybind11/pybind11.h: auto result = reinterpret_borrow<module_>(PyImport_AddModule(full_name.c_str()));
uwsgi-2.0.21.tar.gz: uwsgi-2.0.21/plugins/python/python_plugin.c: if (!PyImport_AddModule("dummy_threading"))
uwsgi-2.0.21.tar.gz: uwsgi-2.0.21/plugins/python/python_plugin.c: py_file_module = PyImport_AddModule(name);
uwsgi-2.0.21.tar.gz: uwsgi-2.0.21/plugins/python/python_plugin.c: new_uwsgi_module = PyImport_AddModule("uwsgi");
uwsgi-2.0.21.tar.gz: uwsgi-2.0.21/plugins/python/symimporter.c: PyObject *mod = PyImport_AddModule(fullname);
uwsgi-2.0.21.tar.gz: uwsgi-2.0.21/plugins/python/symimporter.c: PyObject *mod = PyImport_AddModule(fullname);
uwsgi-2.0.21.tar.gz: uwsgi-2.0.21/plugins/python/symimporter.c: PyObject *mod = PyImport_AddModule(fullname);
uwsgi-2.0.21.tar.gz: uwsgi-2.0.21/plugins/python/symimporter.c: PyObject *mod = PyImport_AddModule(fullname);
python-crfsuite-0.9.9.tar.gz: python-crfsuite-0.9.9/crfsuite/swig/python/export_wrap.cpp: PyObject *module = PyImport_AddModule((char*)"swig_runtime_data" SWIG_RUNTIME_VERSION);
pyinstaller-5.10.0.tar.gz: pyinstaller-5.10.0/bootloader/src/pyi_launch.c: __main__ = PI_PyImport_AddModule("__main__");
pyinstaller-5.10.0.tar.gz: pyinstaller-5.10.0/bootloader/src/pyi_python.c: DECLPROC(PyImport_AddModule);
pyinstaller-5.10.0.tar.gz: pyinstaller-5.10.0/bootloader/src/pyi_python.c: GETPROC(dll, PyImport_AddModule);
pyinstaller-5.10.0.tar.gz: pyinstaller-5.10.0/bootloader/src/pyi_python.h: EXTDECLPROC(PyObject *, PyImport_AddModule, (char *));
opencv-contrib-python-4.7.0.72.tar.gz: opencv-contrib-python-4.7.0.72/opencv/modules/python/src2/cv2.cpp: submodule = PyImport_AddModule(full_submodule_name.c_str());
opencv-contrib-python-4.7.0.72.tar.gz: opencv-contrib-python-4.7.0.72/opencv/modules/python/src2/cv2.cpp: /// Return `PyImport_AddModule` NULL with an exception set on failure.
PyMuPDF-1.21.1.tar.gz: PyMuPDF-1.21.1/fitz/helper-other.i: PyObject *this_module = PyImport_AddModule("fitz");  // get our module
cvxpy-1.3.1.tar.gz: cvxpy-1.3.1/cvxpy/cvxcore/python/cvxcore_wrap.cxx: PyObject *module = PyImport_AddModule("swig_runtime_data" SWIG_RUNTIME_VERSION);
mecab-python3-1.0.6.tar.gz: mecab-python3-1.0.6/src/MeCab/MeCab_wrap.cpp: PyObject *module = PyImport_AddModule("swig_runtime_data" SWIG_RUNTIME_VERSION);
M2Crypto-0.38.0.tar.gz: M2Crypto-0.38.0/src/SWIG/_m2crypto_wrap.c: PyObject *module = PyImport_AddModule("swig_runtime_data" SWIG_RUNTIME_VERSION);
pygraphviz-1.10.zip: pygraphviz-1.10/pygraphviz/graphviz_wrap.c: PyObject *module = PyImport_AddModule("swig_runtime_data" SWIG_RUNTIME_VERSION);
opencv-contrib-python-headless-4.7.0.72.tar.gz: opencv-contrib-python-headless-4.7.0.72/opencv/modules/python/src2/cv2.cpp: submodule = PyImport_AddModule(full_submodule_name.c_str());
opencv-contrib-python-headless-4.7.0.72.tar.gz: opencv-contrib-python-headless-4.7.0.72/opencv/modules/python/src2/cv2.cpp: /// Return `PyImport_AddModule` NULL with an exception set on failure.
GDAL-3.6.3.tar.gz: GDAL-3.6.3/extensions/gdal_array_wrap.cpp: PyObject *module = PyImport_AddModule("swig_runtime_data" SWIG_RUNTIME_VERSION);
GDAL-3.6.3.tar.gz: GDAL-3.6.3/extensions/gdal_wrap.cpp: PyObject *module = PyImport_AddModule("swig_runtime_data" SWIG_RUNTIME_VERSION);
GDAL-3.6.3.tar.gz: GDAL-3.6.3/extensions/gdalconst_wrap.c: PyObject *module = PyImport_AddModule("swig_runtime_data" SWIG_RUNTIME_VERSION);
GDAL-3.6.3.tar.gz: GDAL-3.6.3/extensions/gnm_wrap.cpp: PyObject *module = PyImport_AddModule("swig_runtime_data" SWIG_RUNTIME_VERSION);
GDAL-3.6.3.tar.gz: GDAL-3.6.3/extensions/ogr_wrap.cpp: PyObject *module = PyImport_AddModule("swig_runtime_data" SWIG_RUNTIME_VERSION);
GDAL-3.6.3.tar.gz: GDAL-3.6.3/extensions/osr_wrap.cpp: PyObject *module = PyImport_AddModule("swig_runtime_data" SWIG_RUNTIME_VERSION);
pythonnet-3.0.1.tar.gz: pythonnet-3.0.1/src/runtime/PythonEngine.cs: var module = Runtime.PyImport_AddModule(name);
pythonnet-3.0.1.tar.gz: pythonnet-3.0.1/src/runtime/Runtime.Delegates.cs: PyImport_AddModule = (delegate* unmanaged[Cdecl]<StrPtr, BorrowedReference>)GetFunctionByName(nameof(PyImport_AddModule), GetUnmanagedDll(_PythonDll));
pythonnet-3.0.1.tar.gz: pythonnet-3.0.1/src/runtime/Runtime.Delegates.cs: internal static delegate* unmanaged[Cdecl]<StrPtr, BorrowedReference> PyImport_AddModule { get; }
pythonnet-3.0.1.tar.gz: pythonnet-3.0.1/src/runtime/Runtime.cs: internal static BorrowedReference PyImport_AddModule(string name)
pythonnet-3.0.1.tar.gz: pythonnet-3.0.1/src/runtime/Runtime.cs: return Delegates.PyImport_AddModule(namePtr);
pyarmor-8.1.3.zip: pyarmor-8.1.3/src/helper/buildext.py: m = PyImport_AddModule("__main__");
dlib-19.24.1.tar.gz: dlib-19.24.1/dlib/external/pybind11/include/pybind11/pybind11.h: handle submodule = PyImport_AddModule(full_name.c_str());
osmium-3.6.0.tar.gz: osmium-3.6.0/contrib/pybind11/include/pybind11/pybind11.h: handle submodule = PyImport_AddModule(full_name.c_str());
osmium-3.6.0.tar.gz: osmium-3.6.0/contrib/pybind11/tests/test_modules.py: # Meant to trigger PyImport_AddModule() failure:
onnxoptimizer-0.3.10.tar.gz: onnxoptimizer-0.3.10/third_party/onnx/third_party/pybind11/include/pybind11/pybind11.h: auto result = reinterpret_borrow<module_>(PyImport_AddModule(full_name.c_str()));
casadi-3.6.0.tar.gz: casadi-3.6.0/swig/casadi.i: PyObject* module = PyImport_AddModule("casadi");
casadi-3.6.0.tar.gz: casadi-3.6.0/swig/python/target/source/casadiPYTHON_wrap.cxx: PyObject *module = PyImport_AddModule((char*)"swig_runtime_data" SWIG_RUNTIME_VERSION);
casadi-3.6.0.tar.gz: casadi-3.6.0/swig/python/target/source/casadiPYTHON_wrap.cxx: PyObject* module = PyImport_AddModule(SWIG_module_name);
casadi-3.6.0.tar.gz: casadi-3.6.0/swig/python/target/source/casadiPYTHON_wrap.cxx: PyObject* module = PyImport_AddModule("casadi");
casadi-3.6.0.tar.gz: casadi-3.6.0/swig/python/target3/source/casadiPYTHON_wrap.cxx: PyObject *module = PyImport_AddModule((char*)"swig_runtime_data" SWIG_RUNTIME_VERSION);
casadi-3.6.0.tar.gz: casadi-3.6.0/swig/python/target3/source/casadiPYTHON_wrap.cxx: PyObject* module = PyImport_AddModule(SWIG_module_name);
casadi-3.6.0.tar.gz: casadi-3.6.0/swig/python/target3/source/casadiPYTHON_wrap.cxx: PyObject* module = PyImport_AddModule("casadi");
onnxsim-0.4.19.tar.gz: onnxsim-0.4.19/third_party/onnx-optimizer/third_party/onnx/third_party/pybind11/include/pybind11/pybind11.h: auto result = reinterpret_borrow<module_>(PyImport_AddModule(full_name.c_str()));
onnxsim-0.4.19.tar.gz: onnxsim-0.4.19/third_party/pybind11/include/pybind11/pybind11.h: auto result = reinterpret_borrow<module_>(PyImport_AddModule(full_name.c_str()));
pygdal-3.5.1.11.tar.gz: pygdal-3.5.1.11/extensions/gdal_array_wrap.cpp: PyObject *module = PyImport_AddModule("swig_runtime_data" SWIG_RUNTIME_VERSION);
pygdal-3.5.1.11.tar.gz: pygdal-3.5.1.11/extensions/gdal_wrap.cpp: PyObject *module = PyImport_AddModule("swig_runtime_data" SWIG_RUNTIME_VERSION);
pygdal-3.5.1.11.tar.gz: pygdal-3.5.1.11/extensions/gdalconst_wrap.c: PyObject *module = PyImport_AddModule("swig_runtime_data" SWIG_RUNTIME_VERSION);
pygdal-3.5.1.11.tar.gz: pygdal-3.5.1.11/extensions/ogr_wrap.cpp: PyObject *module = PyImport_AddModule("swig_runtime_data" SWIG_RUNTIME_VERSION);
pygdal-3.5.1.11.tar.gz: pygdal-3.5.1.11/extensions/osr_wrap.cpp: PyObject *module = PyImport_AddModule("swig_runtime_data" SWIG_RUNTIME_VERSION);
mercurial-6.4.1.tar.gz: mercurial-6.4.1/contrib/fuzz/pyutil.cc: mainmod = PyImport_AddModule("__main__");
vstinner commented 1 year ago

The initial issue, adding PyImport_AddModuleRef() function, was done, so I close the issue.

Deprecate PyImport_AddModule() and PyImport_AddModuleObject()

IMO PyWeakref_GetObject() API is unsafe since it returns a borrowed reference from a weak reference, so I chose to deprecate it as soon as I added PyWeakref_GetRef().

For PyImport_AddModuleObject(), it's less obvious since the module must be stored in sys.modules which holds a strong reference. Moreover, I checked and it looks impossible to use a type other than dict for sys.modules in practice, since PyImport_AddModuleObject() access directly to PyInterpreterState.imports.modules which cannot be overriden.

See PR #105998 and PR #106001 for technical details.

Moreover, the Steering Council is still discussing the topic: https://github.com/python/steering-council/issues/201

For now, I prefer to not deprecate the old API. We can still deprecate it later once the situation will be more clear.

encukou commented 1 year ago

Let me bring the discussion from https://github.com/python/steering-council/issues/201#issuecomment-1644017177 here:

We will want to yet another function to fix that wart, and if no one gets to it in time, the *Ref iteration will be hard to get rid of.

Is there any user request for such feature? Can't you get the behavior you want with existing functions, like checking if the name is in sys.modules?

No, there is no such request. Neither is there a user request for a PyImport_AddModuleRef.

One minor change would be to provide the information if the module was imported or created (add an optional parameter for that). You can open an issue if you want that.

I opened an issue here: https://github.com/python/cpython/issues/106915