Skip to content
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

[BUG] Partially loaded circular imports don't import #4390

Closed
da-woods opened this issue Sep 30, 2021 · 4 comments
Closed

[BUG] Partially loaded circular imports don't import #4390

da-woods opened this issue Sep 30, 2021 · 4 comments

Comments

@da-woods
Copy link
Contributor

Copied from the users mailing list (https://groups.google.com/g/cython-users/c/VNuQKATGuss)

I'm running cython (cloned from github mid-May 2021) and Python 3.6/3.7.9. I've encountered an issue with circular imports, one that Python runs without a problem. At its simplest, it's the following sequence of operations:

import A.B         
  from . import C      # <-- in A/B.py
    from . import D    # <-- in A/C.py
      from . import C  # <-- in A/D.py

If you run this scenario in CPython, it runs without error, but if I compile all the .py files into C extensions using Cython, the last import fails with the error:

ImportError: cannot import name C

Each of the above statements seems to be implemented by Cython with a pair of __Pyx_Import and __Pyx_ImportFrom calls. The __Pyx_Import call attempts to do the import, and returns the parent package (in all cases above, it returns A). The __Pyx_ImportFrom call just looks inside the returned module for an attribute with the requested name (C, or D in the above examples). If the requested name ('C') in __Pyx_ImportFrom is actually a module that has not finished initializing yet, then it is not yet set as an attribute in its parent ('A') so this fails.

In CPython, I think, if the above attribute access (i.e. not hasattr(A, 'C')) fails, then it checks if it is a module (i.e. __import__('A.C')) probably to address this exact case. Any chance __Pyx_ImportFrom could do this too to mimic CPython behavior? Obviously the above scenario is silly, but the pattern shows up in an existing library that I'm trying to embed into a static Python interpreter and it seems like it is supported by the language.

Below is a list of commands that will generate the source files and test the above scenario on CentOS/RedHat 7, presumably minimal changes for other Linux variants. Thanks for your help!

mkdir circular_import
cd circular_import
mkdir -p src/A
mkdir -p install/A
echo "from . import C" > src/A/B.py
echo "from . import D" > src/A/C.py
echo "from . import C" > src/A/D.py
for i in A/{B,C,D} ; do cython src/"${i}".py ; gcc -shared -pthread -fPIC -fwrapv -O2 -Wall -fno-strict-aliasing -I/usr/include/python3.6m -o install/"${i}".so src/"${i}".c ; done

(cd src && echo "import A.B" | python3 )  # succeeds
(cd install && echo "import A.B" | python3 )  # fails
@SyamGadde
Copy link
Contributor

I'm the original poster, thanks for bringing this over. I've prototyped the following and it avoids the error above, but don't know if it's the most appropriate solution (or whether it follows code guidelines!):

index a7e18c73c..22ea3ec15 100644
--- a/Cython/Utility/ImportExport.c
+++ b/Cython/Utility/ImportExport.c
@@ -238,6 +238,23 @@ static PyObject* __Pyx_ImportFrom(PyObject* module, PyObject* name); /*proto*/
 static PyObject* __Pyx_ImportFrom(PyObject* module, PyObject* name) {
     PyObject* value = __Pyx_PyObject_GetAttrStr(module, name);
     if (unlikely(!value) && PyErr_ExceptionMatches(PyExc_AttributeError)) {
+        /* in case name refers to a (sub-)module, try importing it by full name */
+        PyErr_Clear();
+        PyObject* module_name = PyModule_GetNameObject(module);
+        if (module_name) {
+            PyObject* dot = PyUnicode_FromString(".");
+            PyObject* module_dot = PyUnicode_Concat(module_name, dot);
+            PyObject* full_name = PyUnicode_Concat(module_dot, name);
+            if (full_name) {
+                value = __Pyx_Import(full_name, (PyObject *)NULL, 0);
+            }
+            Py_XDECREF(full_name);
+            Py_XDECREF(module_dot);
+            Py_XDECREF(dot);
+            Py_XDECREF(module_name);
+        }
+    }
+    if (unlikely(!value)) {
         PyErr_Format(PyExc_ImportError,
         #if PY_MAJOR_VERSION < 3
             "cannot import name %.230s", PyString_AS_STRING(name));

@da-woods
Copy link
Contributor Author

I can see a tiny C89 change that we'd ask for:

PyErr_Clear();
PyObject* module_name = ...

We're trying to keep things C89 compatible for now, so declarations come at the start of the block (for module_name).

I think a PR with that would be welcome. We'd want a test case, but the one in your post would be fine. Add it in a srctree file in tests/run.

(I'm not an expert in the fine details of the import mechanism so there may be some details that I'm missing of course!)

@da-woods
Copy link
Contributor Author

Also some NULL checks on the results of the unicode concats (as unlikely as it would be to trigger those)

@SyamGadde
Copy link
Contributor

I'll work on a PR, thanks for the advice!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants