Leverage importlib.reload for reloading modules
#11679
Conversation
Rather than directly manipulating `sys.modules` ourselves, rely on `importlib.reload` to do it, as it works in more cases (particularly for Rust extension modules built with PyO3). Also, import modules with `typing.TYPE_CHECKING` set to `False` first, and reload them with it set to `True`. This way, if the reload fails, things are likely to still be in a valid state from the first, successful import.
sphinx/ext/autodoc/importer.py
Outdated
| typing.TYPE_CHECKING = False | ||
| module = import_module(modname, warningiserror=warningiserror) | ||
| # Ignore failures; we've already successfully loaded these modules. | ||
| with contextlib.suppress(Exception): |
There was a problem hiding this comment.
Can we have a narrower exception type than Exception?
There was a problem hiding this comment.
I don't think so -- at least, not without sacrificing correctness. If I change this to just catch ImportError, then doing:
.. automodule:: pydanticfails with::
> _PartialClsOrStaticMethod: TypeAlias = Union[classmethod[Any, Any, Any], staticmethod[Any, Any], partialmethod[Any]]
E TypeError: type 'classmethod' is not subscriptable
Let me flip the question around on you: given that the module was already imported successfully with typing.TYPE_CHECKING == False, which sorts of exceptions encountered while reloading the module with typing.TYPE_CHECKING == True should cause import_object to fail instead of using the already successfully imported module?
There was a problem hiding this comment.
Good point. I take it from your testing that e.g. if pydantic is successfully imported, and then fails on the reload, the initial imported module is still usable, coherent, etc?
There was a problem hiding this comment.
That's my expectation, and it seems to be the case for pydantic, at least up to the point where a basic .. automodule:: pydantic succeeds. I'm not sure how to test it more thoroughly, though. I do suspect that this is still going to be fragile in ways that we can't predict, the hope is just that it works for more cases than both what's in #11511 and what's in #11645
|
I'm gonna mark this as draft for now, and investigate the report in #11662 (comment) that this still isn't enough for some modules... |
|
Matt -- I've added a basic escape hatch through |
|
I'd like to merge this as it is an improvement over the status quo, even if not perfect -- @godlygeek did you make any progress in your investigations, or any other observations that would halt merging this? A |
|
Nothing beyond what I noted in #11662 (comment) I still think this is probably more correct than both what shipped in 7.2.0 and what shipped in 7.2.5, but I also think that this entire approach of setting |
Feature or Bugfix
Purpose
Rather than directly manipulating
sys.modulesourselves, rely onimportlib.reloadto do it, as it works in more cases (particularly for Rust extension modules built with PyO3).Also, import modules with
typing.TYPE_CHECKINGset toFalsefirst, and reload them with it set to true. This way, if the reload fails, things are likely to still be in a valid state from the first, successful import.Relates