Clean `sys.modules` if `TYPE_CHECKING=True` import fails #11645

godlygeek · 2023-08-25T23:22:31Z

autodoc now attempts to import with typing.TYPE_CHECKING = True first, and then falls back to typing.TYPE_CHECKING = False if that fails. Unfortunately, the first import can leave behind some partially-imported modules in sys.modules such that the retry fails.

Attempt to work around this by detecting what modules were added to sys.modules by the first attempt and removing them before retrying.

Feature or Bugfix

Bugfix

Purpose

autodoc now attempts to importing with typing.TYPE_CHECKING = True first, and then falls back to typing.TYPE_CHECKING = False if that fails. Unfortunately, the first import can leave behind some partially-imported modules in sys.modules such that the retry fails.

Attempt to work around this by detecting what modules were added to sys.modules by the first attempt and removing them before retrying.

Detail / Relates

See #11642

godlygeek · 2023-08-26T00:57:56Z

The 1 failing test run seems unrelated to my changes, AFAICS

picnixz

Can you add an explicit test checking the state of sys.modules please?

picnixz · 2023-08-26T11:19:27Z

sphinx/ext/autodoc/importer.py

@@ -82,13 +83,18 @@ def import_object(modname: str, objpath: list[str], objtype: str = '',
        objpath = list(objpath)
        while module is None:
            try:
+                orig_modules = set(sys.modules)


Use a frozenset instead. Same in the except when you are running over the modules to remove them. At least it shows that the variable is meant to be readonly.

godlygeek · 2023-08-26T18:15:39Z

Can you add an explicit test checking the state of sys.modules please?

Can you elaborate on what you'd like this test to look like? I'm not sure I follow. At what point do you want sys.modules to be checked? Do you want it to be checked by the test body, or by the code inside the test root?

Oh, maybe I see: are you just suggesting that tests/roots/test-ext-autodoc/circular_import/c.py should have something like assert hasattr(circular_import, "a") to make it more explicit exactly what bug is being fixed? I've done that.

picnixz · 2023-08-27T13:17:29Z

Actually the test root is only some fixture directory so it should be tested in the test body. For instance, you check what sys.modules is before and after building (I think?).

AA-Turner · 2023-08-28T04:23:57Z

Tentatively targeting 7.2.5 for this, as it is a regression from 7.1.

a

autodoc now attempts to import with `typing.TYPE_CHECKING = True` first, and then falls back to `typing.TYPE_CHECKING = False` if that fails. Unfortunately, the first import can leave behind some partially-imported modules in `sys.modules` such that the retry fails. Attempt to work around this by detecting what modules were added to `sys.modules` by the first attempt and removing them before retrying. Signed-off-by: Matt Wozniski <mwozniski@bloomberg.net>

godlygeek · 2023-08-28T20:10:30Z

Actually the test root is only some fixture directory so it should be tested in the test body. For instance, you check what sys.modules is before and after building (I think?).

OK, I've added a test that explicitly looks at sys.modules. I'm not sure if it's what you were picturing or not, so please take a look.

picnixz · 2023-08-29T00:20:48Z

Thank you. I was thinking about a more explicit check where you check that some module does not exist in sys.modules.

AA-Turner

Implementation looks good, please ping when the test suggested by @picnixz has been added for merging. Please also add an entry to CHANGES describing the bug fixed as I intend this to be included in 7.2.5.

A

godlygeek · 2023-08-29T02:26:55Z

Thank you. I was thinking about a more explicit check where you check that some module does not exist in sys.modules.

I don't understand...

The regression that this PR is fixing is that autodoc fails to import some modules. The buggy behavior that the new test would exhibit without my fix is that autodoc fails to import circular_import, and the corrected behavior, as well as the 7.1 behavior, is that it imports it successfully. I don't understand what sort of test you're picturing for this behavior that's based on a module being absent from sys.modules, when the whole goal of the change is to cause more modules to be imported than would be without the change.

AA-Turner · 2023-08-29T02:41:09Z

I'd understood it to mean checking that a half-imported module isn't left over, even if the entire import fails, but that might be difficult to set up a test-case for.

A

godlygeek · 2023-08-29T02:47:59Z

I'd understood it to mean checking that a half-imported module isn't left over, even if the entire import fails

This PR won't do that, though. It prevents a failed import done with typing.TYPE_CHECKING = True from polluting sys.modules in a way that causes a future import of the same module with typing.TYPE_CHECKING = False to fail. It stops failures in the first pass from bleeding into the second and causing failures, but doesn't prevent failures in the second pass from affecting sys.modules - if the module really can't be imported, even with typing.TYPE_CHECKING = False, then half-imported modules will still be present in sys.modules (just like they would have been in 7.1)

AA-Turner · 2023-08-29T03:03:41Z

Ahh! Then let's leave things as they are, just the CHANGES entry needed please if possible!

A

picnixz · 2023-08-29T04:46:34Z

Ah, my bad then, I understood wrongly.

godlygeek · 2023-08-29T16:13:46Z

@AA-Turner I've added that CHANGES entry

AA-Turner · 2023-08-29T16:18:54Z

Thanks -- I've credited you as Matt, just checking on Matt vs Matthew?

godlygeek · 2023-08-29T18:54:08Z

I prefer "Matt" - thanks for checking!

AA-Turner · 2023-08-29T19:05:49Z

Thanks @godlygeek!

A

godlygeek force-pushed the fix-typing.TYPE_CHECKING-handling branch from cba1453 to b1977c5 Compare August 25, 2023 23:23

picnixz reviewed Aug 26, 2023

View reviewed changes

godlygeek force-pushed the fix-typing.TYPE_CHECKING-handling branch 3 times, most recently from d5bdf5e to 2077f50 Compare August 26, 2023 18:23

godlygeek force-pushed the fix-typing.TYPE_CHECKING-handling branch from 2077f50 to 8528c31 Compare August 28, 2023 20:07

AA-Turner added 2 commits August 29, 2023 03:19

Merge branch 'master' into fix-typing.TYPE_CHECKING-handling

9220b7f

style

186618a

AA-Turner approved these changes Aug 29, 2023

View reviewed changes

picnixz approved these changes Aug 29, 2023

View reviewed changes

Add CHANGES entry

4023ecd

Add credit

ae36324

AA-Turner merged commit 8248be3 into sphinx-doc:master Aug 29, 2023

AA-Turner mentioned this pull request Aug 29, 2023

Sphinx 7.2 autodoc fails to import some modules that use typing.TYPE_CHECKING #11642

Closed

AA-Turner mentioned this pull request Sep 1, 2023

Sphinx 7.2.5 breaks with PyO3: "modules may only be initialized once per interpreter process" #11662

Closed

matthiasdiener mentioned this pull request Sep 7, 2023

limit sphinx version inducer/grudge#317

Merged

godlygeek mentioned this pull request Sep 8, 2023

Leverage importlib.reload for reloading modules #11679

Merged

github-actions bot locked as resolved and limited conversation to collaborators Sep 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean `sys.modules` if `TYPE_CHECKING=True` import fails #11645

Clean `sys.modules` if `TYPE_CHECKING=True` import fails #11645

godlygeek commented Aug 25, 2023 •

edited

Loading

godlygeek commented Aug 26, 2023

picnixz left a comment

picnixz Aug 26, 2023

godlygeek commented Aug 26, 2023 •

edited

Loading

picnixz commented Aug 27, 2023

AA-Turner commented Aug 28, 2023

godlygeek commented Aug 28, 2023

picnixz commented Aug 29, 2023

AA-Turner left a comment

godlygeek commented Aug 29, 2023

AA-Turner commented Aug 29, 2023

godlygeek commented Aug 29, 2023 •

edited

Loading

AA-Turner commented Aug 29, 2023

picnixz commented Aug 29, 2023

godlygeek commented Aug 29, 2023

AA-Turner commented Aug 29, 2023

godlygeek commented Aug 29, 2023

AA-Turner commented Aug 29, 2023

Clean sys.modules if TYPE_CHECKING=True import fails #11645

Clean sys.modules if TYPE_CHECKING=True import fails #11645

Conversation

godlygeek commented Aug 25, 2023 • edited Loading

Feature or Bugfix

Purpose

Detail / Relates

godlygeek commented Aug 26, 2023

picnixz left a comment

Choose a reason for hiding this comment

picnixz Aug 26, 2023

Choose a reason for hiding this comment

godlygeek commented Aug 26, 2023 • edited Loading

picnixz commented Aug 27, 2023

AA-Turner commented Aug 28, 2023

godlygeek commented Aug 28, 2023

picnixz commented Aug 29, 2023

AA-Turner left a comment

Choose a reason for hiding this comment

godlygeek commented Aug 29, 2023

AA-Turner commented Aug 29, 2023

godlygeek commented Aug 29, 2023 • edited Loading

AA-Turner commented Aug 29, 2023

picnixz commented Aug 29, 2023

godlygeek commented Aug 29, 2023

AA-Turner commented Aug 29, 2023

godlygeek commented Aug 29, 2023

AA-Turner commented Aug 29, 2023

Clean `sys.modules` if `TYPE_CHECKING=True` import fails #11645

Clean `sys.modules` if `TYPE_CHECKING=True` import fails #11645

godlygeek commented Aug 25, 2023 •

edited

Loading

godlygeek commented Aug 26, 2023 •

edited

Loading

godlygeek commented Aug 29, 2023 •

edited

Loading