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

Jump to bottom

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

Merged
merged 6 commits into from Jul 24, 2021
Merged

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

merged 6 commits into from Jul 24, 2021

Conversation

@FFY00
Copy link
Member

@FFY00 FFY00 commented Jul 23, 2021 •
edited by bedevere-bot

Signed-off-by: Filipe Laíns lains@riseup.net

https://bugs.python.org/issue44717
@FFY00

This comment has been hidden.

Sign in to view
@FFY00 FFY00 force-pushed the bpo-44717 branch 4 times, most recently from 230b3d3 to 5ab4606 Jul 23, 2021
@FFY00 FFY00 marked this pull request as ready for review Jul 23, 2021
@FFY00
bpo-44717: improve AttributeError on circular imports of submodules
4ae2f5a 
Signed-off-by: Filipe Laíns <lains@riseup.net>
@ambv
Copy link
Contributor

@ambv ambv commented Jul 23, 2021

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

@FFY00
Copy link
Member Author

@FFY00 FFY00 commented Jul 23, 2021

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
Copy link
Member Author

@FFY00 FFY00 commented Jul 23, 2021

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
FFY00 reviewed Jul 23, 2021
View changes
Lib/importlib/_bootstrap.py Outdated
[])
parent_spec._uninitialized_submodules = _uninitialized + [child]
if parent_spec:
# Temporarily add currently imported child to parent's
Copy link
Member Author

@FFY00 FFY00 Jul 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@FFY00 FFY00 Jul 23, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
pablogsal reviewed Jul 23, 2021
View changes
Lib/importlib/_bootstrap.py Outdated
@@ -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 = []
Copy link
Member

@pablogsal pablogsal Jul 23, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@FFY00 FFY00 Jul 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
pablogsal reviewed Jul 23, 2021
View changes
Objects/moduleobject.c Outdated
_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;
}
Copy link
Member

@pablogsal pablogsal Jul 23, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
}

Copy link
Member

@pablogsal pablogsal Jul 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@FFY00 FFY00 Jul 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@pablogsal pablogsal Jul 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to remove both here to keep then consistent

@FFY00
fix _uninitialized_submodules and improve _PyModuleSpec_IsUninitializ…
793198c 
…edSubmodule

Signed-off-by: Filipe Laíns <lains@riseup.net>
@FFY00
Copy link
Member Author

@FFY00 FFY00 commented Jul 23, 2021 •
edited

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 ambv merged commit 8072a11 into python:main Jul 24, 2021
13 checks passed
@ambv
Copy link
Contributor

@ambv ambv commented Jul 24, 2021

Thanks a lot, Filipe! ✨ 🍰 ✨

@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Jul 24, 2021

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

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

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. 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.
  4. Check if the failure is related to this commit (8072a11) or if it is a false positive.
  5. 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
Copy link
Member

@pablogsal pablogsal commented Jul 24, 2021

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

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

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. 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.
  4. Check if the failure is related to this commit (8072a11) or if it is a false positive.
  5. 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

pablogsal pushed a commit that referenced this issue Jul 24, 2021
Revert "bpo-44717: improve AttributeError on circular imports of subm…
dda9760 
…odules (GH-27299)"

This reverts commit 8072a11.
@pablogsal pablogsal mentioned this pull request Jul 24, 2021
Merged
pablogsal pushed a commit that referenced this issue Jul 24, 2021
Revert "bpo-44717: improve AttributeError on circular imports of subm…
3eae8f2 
…odules (GH-27299)" (GH-27331)

This reverts commit 8072a11.
@FFY00 FFY00 mentioned this pull request Jul 24, 2021
Merged
JuniorJPDJ added a commit to JuniorJPDJ/cpython that referenced this issue Aug 12, 2021
@FFY00 @ambv @JuniorJPDJ
bpo-44717: improve AttributeError on circular imports of submodules (p…
e02454e 
…ythonGH-27299)

Signed-off-by: Filipe Laíns <lains@riseup.net>

Co-authored-by: Łukasz Langa <lukasz@langa.pl>
JuniorJPDJ added a commit to JuniorJPDJ/cpython that referenced this issue Aug 12, 2021
@JuniorJPDJ
Revert "bpo-44717: improve AttributeError on circular imports of subm…
003fed0 
…odules (pythonGH-27299)" (pythonGH-27331)

This reverts commit 8072a11.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pablogsal pablogsal

Assignees
No one assigned
Labels
CLA signed
Projects
None yet
Milestone
No milestone
5 participants
@FFY00 @ambv @bedevere-bot @pablogsal @the-knights-who-say-ni