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

fix: proper error messages if autosummary fails to import module #9555

Closed
wants to merge 8 commits into from

Conversation

Pro
Copy link

@Pro Pro commented Aug 17, 2021

Subject: Show proper error messages if autosummary fails to import module

Fixes #7989

Feature or Bugfix

  • Bugfix

Purpose

As described in #7989, autosummary currently does not provide helpful error messages.

E.g., let's say I have a module called fancy_bob, which in its init.py uses import filelock (or any other external module).
Then in my docs I write:

.. autosummary::
   :toctree: _autosummary
   :recursive:

   fancy_bob

If the filelock module is not installed, autosummary will just fail with:

[autosummary] failed to import 'fancy_bob': no module named fancy_bob.

This error message is completely misleading, since it actually finds the module fancy_bob but then fails to import its dependencies.

This MR changes the error output in a way that you at least get some hints what went wrong, e.g., now you get:

[autosummary] failed to import 'fancy_bob': no module named fancy_bob. Possible hints: ["No module named 'filelock'"]

Or if there is a syntax error in the module's init.py, it will show:

[autosummary] failed to import 'fancy_bob': no module named fancy_bob. Possible hints: ['invalid syntax (__init__.py, line 4)']

Reproduction

@astrojuanlu provided a nice example project to reproduce the issue in https://github.com/astrojuanlu/sphinx-autosummary-tracebacks

Not setting the pythonpath properly results in:

[autosummary] failed to import 'test_pkg.A': no module named test_pkg.A. Possible hints: ["No module named 'test_pkg'", 'no module named test_pkg']

Setting the pythonpath gives the correct error:

[autosummary] failed to import 'test_pkg.A': no module named test_pkg.A. Possible hints: ['no module named test_pkg', "No module named 'attr'"]

I.e., "No module named 'attr'

Detail

Relates

@AndiEcker
Copy link

+1 thank you very much @Pro

Copy link
Contributor

@flying-sheep flying-sheep left a comment

Choose a reason for hiding this comment

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

Your stringification is a bit wonky, but generally good idea!

sphinx/ext/autosummary/__init__.py Outdated Show resolved Hide resolved
sphinx/ext/autosummary/__init__.py Outdated Show resolved Hide resolved
sphinx/ext/autosummary/__init__.py Outdated Show resolved Hide resolved
Pro and others added 2 commits August 18, 2021 14:28
Co-authored-by: Philipp A. <flying-sheep@web.de>
Co-authored-by: Philipp A. <flying-sheep@web.de>
@tk0miya tk0miya added extensions:autosummary type:enhancement enhance or introduce a new feature labels Aug 29, 2021
@tk0miya tk0miya added this to the 4.2.0 milestone Aug 29, 2021
@@ -614,7 +614,7 @@ def get_import_prefixes_from_env(env: BuildEnvironment) -> List[str]:
return prefixes


def import_by_name(name: str, prefixes: List[str] = [None]) -> Tuple[str, Any, Any, str]:
def import_by_name(name: str, prefixes: List[str] = [None], last_errors = None) -> Tuple[str, Any, Any, str]:
Copy link
Member

Choose a reason for hiding this comment

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

I feel this change is strange because last_errors should be a part of the return value. I understand using argument gives us compatibility. But...

BTW, it should be annotated.

Copy link
Author

Choose a reason for hiding this comment

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

Also thought the same, but that would probably break backwards compatibility? Or should I change last_errors to be a retun value?

Copy link
Member

Choose a reason for hiding this comment

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

I have two ideas:

  1. Add another version of import_by_name that also returns list of errors. It will be named as different name.
  2. Add a boolean argument like raise_error to import_by_name. And it raises a custom exception that contains list of errors.

Copy link
Author

Choose a reason for hiding this comment

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

Hmm, raise_error would not work that well since we need to collect all errors during the various import methods there are.

Having another version of the import_by_name is IMHO also not such an ideal solution, since then we would need to duplicate code, not only for the import_by_name but also for the sub-methods such as _import_by_name which is called inside of import_by_name

sphinx/ext/autosummary/__init__.py Outdated Show resolved Hide resolved
sphinx/ext/autosummary/__init__.py Outdated Show resolved Hide resolved
sphinx/ext/autosummary/__init__.py Outdated Show resolved Hide resolved
sphinx/ext/autosummary/generate.py Outdated Show resolved Hide resolved
@tk0miya tk0miya modified the milestones: 4.2.0, 4.3.0 Sep 12, 2021
sphinx/ext/autosummary/generate.py Outdated Show resolved Hide resolved
sphinx/ext/autosummary/generate.py Outdated Show resolved Hide resolved
@@ -614,7 +614,7 @@ def get_import_prefixes_from_env(env: BuildEnvironment) -> List[str]:
return prefixes


def import_by_name(name: str, prefixes: List[str] = [None]) -> Tuple[str, Any, Any, str]:
def import_by_name(name: str, prefixes: List[str] = [None], last_errors = None) -> Tuple[str, Any, Any, str]:
Copy link
Member

Choose a reason for hiding this comment

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

I have two ideas:

  1. Add another version of import_by_name that also returns list of errors. It will be named as different name.
  2. Add a boolean argument like raise_error to import_by_name. And it raises a custom exception that contains list of errors.

Pro and others added 2 commits September 15, 2021 17:49
Co-authored-by: Takeshi KOMIYA <i.tkomiya@gmail.com>
Co-authored-by: Takeshi KOMIYA <i.tkomiya@gmail.com>
@tk0miya tk0miya modified the milestones: 4.3.0, 4.4.0 Nov 9, 2021
@Pro
Copy link
Author

Pro commented Dec 20, 2021

@tk0miya is there still something missing to merge this MR?

@tk0miya
Copy link
Member

tk0miya commented Dec 30, 2021

Add a boolean argument like raise_error to import_by_name. And it raises a custom exception that contains list of errors.

I just post #10031 to implement my idea.

@Pro
Copy link
Author

Pro commented Dec 30, 2021

Add a boolean argument like raise_error to import_by_name. And it raises a custom exception that contains list of errors.

I just post #10031 to implement my idea.

Cool! Also nice solution!
I will close this MR in favor of yours.

@Pro Pro closed this Dec 30, 2021
tk0miya added a commit that referenced this pull request Jan 1, 2022
…autosummary

Close #9555: autosummary: Improve error messages on failure to load target object
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
extensions:autosummary type:enhancement enhance or introduce a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

autosummary doesn’t report correct module on ImportError
4 participants