bpo-44717: improve AttributeError on circular imports of submodules #27299

FFY00 · 2021-07-23T00:52:50Z

Signed-off-by: Filipe Laíns [email protected]

https://bugs.python.org/issue44717

Signed-off-by: Filipe Laíns <[email protected]>

ambv · 2021-07-23T16:02:33Z

@FFY00, I refactored your change a little in _bootstrap.py. What do you think?

FFY00 · 2021-07-23T16:18:28Z

Looks great, thanks! I wasn't sure about always setting the attribute so I just mimicked _initializing.
Don't you think it would be better to still keep _uninitialized_submodules private? I thought of it as a simple implementation detail to achieve what this patch's goal, I am afraid if we make public like this people might start relying on it for some reason. I assume _initializing being private has the same reasoning. Other than that, everything looks great, it's just this maintenance nitpick 😁

FFY00 · 2021-07-23T16:27:41Z

Btw, the tests are failing because you renamed _uninitialized_submodules to uninitialized_submodules and didn't update the C code 😛
I can fix the code if you want, just let me know what you want to do about the naming, as I described in the reply above.

FFY00 · 2021-07-23T16:34:39Z

Lib/importlib/_bootstrap.py

-                                    [])
-            parent_spec._uninitialized_submodules = _uninitialized + [child]
+        if parent_spec:
+            # Temporarily add currently imported child to parent's


Hum, shouldn't it more correct to say "child we are going to import" instead of "currently imported child"? As the child isn't actually imported, we are going to try to import it.

The error occurs when the child (let's say a.b.c) tries to import the parent (import a.b), but the parent also imports the child (import a.b.c), which results in the child not importing the parent, just the parent's parent (a) and not setting the parent as an attribute (a.b will not be set). We want to be able to flag that error when we import the child (a.b.c) and provide a nicer message if it tries to access the parent (a.b, which is not set, only a). Very confusing, I know 😅

pablogsal · 2021-07-23T17:39:39Z

Lib/importlib/_bootstrap.py

@@ -361,6 +361,7 @@ def __init__(self, name, loader, *, origin=None, loader_state=None,
        self.origin = origin
        self.loader_state = loader_state
        self.submodule_search_locations = [] if is_package else None
+        self.uninitialized_submodules = []


Though: This should better be a set, no? That way you can use PySet_Contains in the C code, which is not linear.

On the other hand, we don't expect this to be performance-critical and someone could change it so PySet_Contains can fail without a pre-check, which complicates the code.

Sure, I don't really have any preference, as this is something that will not really matter much. Let me know if you want me to change it to set.

pablogsal · 2021-07-23T17:45:48Z

Objects/moduleobject.c

+_PyModuleSpec_IsUninitializedSubmodule(PyObject *spec, PyObject *name)
+{
+    if (spec != NULL) {
+        _Py_IDENTIFIER(_uninitialized_submodules);
+        PyObject *value = _PyObject_GetAttrId(spec,
+                                              &PyId__uninitialized_submodules);
+        if (value != NULL) {
+            int is_uninitialized = PySequence_Contains(value, name);
+            Py_DECREF(value);
+            if (is_uninitialized >= 0) {
+                return is_uninitialized;
+            }
+        }
+    }
+    PyErr_Clear();
+    return 0;
+}


I prefer to avoid nesting the conditionals so every case is more clear:

Suggested change

_PyModuleSpec_IsUninitializedSubmodule(PyObject *spec, PyObject *name)

{

if (spec != NULL) {

_Py_IDENTIFIER(_uninitialized_submodules);

PyObject *value = _PyObject_GetAttrId(spec,

&PyId__uninitialized_submodules);

if (value != NULL) {

int is_uninitialized = PySequence_Contains(value, name);

Py_DECREF(value);

if (is_uninitialized >= 0) {

return is_uninitialized;

}

}

}

PyErr_Clear();

return 0;

}

_PyModuleSpec_IsUninitializedSubmodule(PyObject *spec, PyObject *name)

{

if (spec == NULL) {

goto exit;

}

_Py_IDENTIFIER(_uninitialized_submodules);

PyObject *value = _PyObject_GetAttrId(spec, &PyId__uninitialized_submodules);

if (value == NULL) {

goto error;

}

int is_uninitialized = PySequence_Contains(value, name);

Py_DECREF(value);

if (is_uninitialized == -1) {

goto error

}

return is_uninitialized;

exit:

PyErr_Clear();

return 0;

}

Also, I don't get why we need to clear the error if spec == NULL. That sounds a bit weird as an API.

This code was almost a verbatim copy from _PyModuleSpec_IsInitializing, it was not clear to me why the error was cleared given that we actually return on spec == NULL && PyErr_Occurred() in module_getattro. Looking at the commit that introduced it (3e429dc), it makes sense, but this is no longer needed.
We could remove this from _PyModuleSpec_IsUninitializedSubmodule and then I could make other PR removing it from _PyModuleSpec_IsInitializing too.

I would prefer to remove both here to keep then consistent

…edSubmodule Signed-off-by: Filipe Laíns <[email protected]>

FFY00 · 2021-07-23T18:26:10Z

I have renamed uninitialized_submodules to _uninitialized_submodules, adjusted the comment as indicated above, and removed the error clearing from _PyModuleSpec_IsUninitializedSubmodule.
@ambv feel free to rollback the commit, or ask me to, if you actually want uninitialized_submodules to be public 😊

ambv · 2021-07-24T09:50:35Z

Thanks a lot, Filipe! ✨ 🍰 ✨

bedevere-bot · 2021-07-24T10:27:42Z

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot x86 Gentoo Installed with X 3.x has failed when building commit 8072a11.

What do you need to do:

Don't panic.
Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/464/builds/574) and take a look at the build logs.
Check if the failure is related to this commit (8072a11) or if it is a false positive.
If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/464/builds/574

Failed tests:

test_import

Failed subtests:

test_absolute_circular_submodule - test.test_import.CircularImportTests

Summary of the results of the build (if available):

== Tests result: FAILURE then FAILURE ==

412 tests OK.

1 test failed:
test_import

14 tests skipped:
test_asdl_parser test_check_c_globals test_clinic test_ctypes
test_devpoll test_gdb test_ioctl test_kqueue test_msilib
test_startfile test_winconsoleio test_winreg test_winsound
test_zipfile64

Total duration: 29 min 18 sec

Click to see traceback logs

TracebackTests) ... ok


Traceback (most recent call last):
  File "/buildbot/buildarea/cpython/3.x.ware-gentoo-x86.installed/build/target/lib/python3.11/test/test_import/__init__.py", line 1355, in test_absolute_circular_submodule
    import test.test_import.data.circular_imports.subpkg2.parent
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
ModuleNotFoundError: No module named 'test.test_import.data.circular_imports.subpkg2'

pablogsal · 2021-07-24T12:57:08Z

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot x86 Gentoo Installed with X 3.x has failed when building commit 8072a11.

What do you need to do:

Don't panic.

Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.

Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/464/builds/574) and take a look at the build logs.

Check if the failure is related to this commit (8072a11) or if it is a false positive.

If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/464/builds/574

Failed tests:

test_import

Failed subtests:

test_absolute_circular_submodule - test.test_import.CircularImportTests

Summary of the results of the build (if available):

== Tests result: FAILURE then FAILURE ==

412 tests OK.

1 test failed:
test_import

14 tests skipped:
test_asdl_parser test_check_c_globals test_clinic test_ctypes
test_devpoll test_gdb test_ioctl test_kqueue test_msilib
test_startfile test_winconsoleio test_winreg test_winsound
test_zipfile64

Total duration: 29 min 18 sec
Click to see traceback logs
TracebackTests) ... ok


Traceback (most recent call last):
  File "/buildbot/buildarea/cpython/3.x.ware-gentoo-x86.installed/build/target/lib/python3.11/test/test_import/__init__.py", line 1355, in test_absolute_circular_submodule
    import test.test_import.data.circular_imports.subpkg2.parent
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
ModuleNotFoundError: No module named 'test.test_import.data.circular_imports.subpkg2'

This is a genuine failure, we will need to revert if is not fixed in 24 h

…odules (GH-27299)" This reverts commit 8072a11.

…odules (GH-27299)" (GH-27331) This reverts commit 8072a11.

bedevere-bot added the awaiting review label Jul 23, 2021

the-knights-who-say-ni added the CLA signed label Jul 23, 2021

This comment has been minimized.

Sign in to view

FFY00 force-pushed the bpo-44717 branch 4 times, most recently from 230b3d3 to 5ab4606 Compare July 23, 2021 13:33

FFY00 marked this pull request as ready for review July 23, 2021 13:35

bpo-44717: improve AttributeError on circular imports of submodules

4ae2f5a

Signed-off-by: Filipe Laíns <[email protected]>

FFY00 force-pushed the bpo-44717 branch from 5ab4606 to 4ae2f5a Compare July 23, 2021 13:42

Simplify submodule tracking code

96b8cd9

ambv added 2 commits July 23, 2021 18:08

Always set uninitialized_submodules

8e95aab

Regen importlib.h

9855484

FFY00 commented Jul 23, 2021

View reviewed changes

pablogsal reviewed Jul 23, 2021

View reviewed changes

fix _uninitialized_submodules and improve _PyModuleSpec_IsUninitializ…

793198c

…edSubmodule Signed-off-by: Filipe Laíns <[email protected]>

Reword comment

4a3f002

ambv merged commit 8072a11 into python:main Jul 24, 2021

bedevere-bot removed the awaiting review label Jul 24, 2021

pablogsal added a commit that referenced this pull request Jul 24, 2021

Revert "bpo-44717: improve AttributeError on circular imports of subm…

dda9760

…odules (GH-27299)" This reverts commit 8072a11.

pablogsal mentioned this pull request Jul 24, 2021

bpo-44717: Revert "improve AttributeError on circular imports of submodules" #27331

Merged

pablogsal added a commit that referenced this pull request Jul 24, 2021

Revert "bpo-44717: improve AttributeError on circular imports of subm…

3eae8f2

…odules (GH-27299)" (GH-27331) This reverts commit 8072a11.

FFY00 mentioned this pull request Jul 24, 2021

bpo-44717: improve AttributeError on circular imports of submodules #27338

Merged

Uh oh!

bpo-44717: improve AttributeError on circular imports of submodules #27299

bpo-44717: improve AttributeError on circular imports of submodules #27299

Uh oh!

Conversation

FFY00 commented Jul 23, 2021 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

ambv commented Jul 23, 2021

Uh oh!

FFY00 commented Jul 23, 2021

Uh oh!

FFY00 commented Jul 23, 2021

Uh oh!

FFY00 Jul 23, 2021

Choose a reason for hiding this comment

Uh oh!

FFY00 Jul 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pablogsal Jul 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FFY00 Jul 23, 2021

Choose a reason for hiding this comment

Uh oh!

pablogsal Jul 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pablogsal Jul 23, 2021

Choose a reason for hiding this comment

Uh oh!

FFY00 Jul 23, 2021

Choose a reason for hiding this comment

Uh oh!

pablogsal Jul 23, 2021

Choose a reason for hiding this comment

Uh oh!

FFY00 commented Jul 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ambv commented Jul 24, 2021

Uh oh!

bedevere-bot commented Jul 24, 2021

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Uh oh!

pablogsal commented Jul 24, 2021

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Uh oh!

Uh oh!

FFY00 commented Jul 23, 2021 •

edited by bedevere-bot

Loading

FFY00 Jul 23, 2021 •

edited

Loading

pablogsal Jul 23, 2021 •

edited

Loading

pablogsal Jul 23, 2021 •

edited

Loading

FFY00 commented Jul 23, 2021 •

edited

Loading