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.

Sign up for GitHub

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

Use a more robust method to improve our ModuleNotFound errors #3263

Merged
merged 5 commits into from
Dec 13, 2022
Merged

Use a more robust method to improve our ModuleNotFound errors #3263

merged 5 commits into from
Dec 13, 2022

Conversation

hoodmane
Copy link
Member

@hoodmane hoodmane commented Nov 16, 2022 •
edited
Loading

@ryanking13 added these very nice error messages to the ModuleNotFound errors. However they introduce a few problems:

  1. find_spec is supposed to return None or a spec and not to raise an error. If it raises errors, it can cause trouble in code that tries to check if a module is installed or not.
  2. Other code that tries to add new import hooks has to know to insert them before these error-raising import hooks.

See the discussion in #3262.

This instead patches importlib._bootstrap to create a function called _get_module_not_found_error. We then can monkey patch this to modify the error messages that importlib raises.

Checklists

@hoodmane
Copy link
Member Author

I also made a thread about this on discuss.python.org:
https://discuss.python.org/t/a-hook-to-allow-adding-notes-to-modulenotfounderror/21151/2

@hoodmane hoodmane mentioned this pull request Nov 17, 2022
Merged
@ryanking13
Copy link
Member

ryanking13 commented Nov 17, 2022 •
edited
Loading

Thanks for the fix @hoodmane!

What do think of modifying sys.excepthook, something like this? (I have never tried, just an idea).

def pyodide_except_hook(type, value, traceback):
    orig_excepthook = sys.excepthook
    if isinstance(type, ModuleNotFoundError):
        ...
    else:
        return orig_excepthook(...)

sys.excepthook = pyodide_except_hook

I think this won't require patching CPython.

@hoodmane
Copy link
Member Author

hoodmane commented Nov 17, 2022 •
edited
Loading

We currently never call sys.excepthook. Instead of that, we turn errors into JavaScript PythonError exceptions when they reach the bottom of the stack.

@ryanking13
Copy link
Member

Oh, okay. You are right. I was able to check that sys.excepthook is never called.

@rth
Copy link
Member

rth commented Nov 17, 2022 •
edited
Loading

No objection to this change, but it would be worth opening a feature request at Cpython to ask to make it possible change this message without patching. Or if they don't want to update the public API of importlib for this, maybe at least they would be OK with adding this patch upstream so that it can be modified via monkeypatching (instead of real patching). As the patch to Python/importlib.h in particular looks a bit annoying to maintain.

@hoodmane
Copy link
Member Author

the patch to Python/importlib.h in particular looks a bit annoying to maintain.

Well it's autogenerated, you just have to run make regen-importlib.

@hoodmane
Copy link
Member Author

at least they would be OK with adding this patch upstream so that it can be modified via monkeypatching (instead of real patching).

I bet they would accept that, I'll try.

@ryanking13 ryanking13 added this to the 0.22.0 milestone Nov 30, 2022
@ryanking13 ryanking13 mentioned this pull request Dec 12, 2022
Closed
10 tasks
ryanking13
ryanking13 approved these changes Dec 12, 2022
View reviewed changes
Copy link
Member

@ryanking13 ryanking13 left a comment

Choose a reason for hiding this comment

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

I am okay with merging this, and would prefer to include this in 0.22.

@hoodmane hoodmane mentioned this pull request Dec 13, 2022
Open
@hoodmane hoodmane merged commit 33d1794 into pyodide:main Dec 13, 2022
@hoodmane hoodmane deleted the module-not-found-hook branch December 13, 2022 02:35
dcherian added a commit to dcherian/pyodide that referenced this pull request Dec 13, 2022
@dcherian
Merge branch 'main' into patch-1
d345712 
* main: (45 commits)
  Remove pre-built docker image support (pyodide#3342)
  Remove "Python initialization complete" log line (pyodide#3247)
  Use a more robust method to improve our ModuleNotFound errors (pyodide#3263)
  Distinguish between sync and async JavaScript iterators when possible (pyodide#3339)
  [pre-commit.ci] pre-commit autoupdate (pyodide#3345)
  NFC Place js_flags in separate dict (pyodide#3338)
  NFC Use initialization function to load _pyodide_core (pyodide#3333)
  Make fs timestamps have millisecond resolution rather than second resolution (pyodide#3313)
  Add gensim package pyodide#2545 (pyodide#3326)
  [pre-commit.ci] pre-commit autoupdate (pyodide#3325)
  Update scikit-learn to 1.1.3 (pyodide#3324)
  Fix markdown in doc (pyodide#3323)
  Emscripten 3.1.27 (pyodide#3314)
  [pre-commit.ci] pre-commit autoupdate (pyodide#3254)
  Add a typeshed for the js module (pyodide#3298)
  Unpin host Python patch versions in GHA (pyodide#3309)
  Package pyinstrument (pyodide#3258)
  Add athrow and aclose to JsProxy of an AsyncGenerator (pyodide#3299)
  Add test for MutableMapping methods on object_maps and fix bug (pyodide#3297)
  Bump minimatch from 3.0.4 to 3.1.2 in /src/js (pyodide#3306)
  ...
@JeffersGlass JeffersGlass mentioned this pull request Jan 9, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ryanking13 ryanking13 ryanking13 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.22.0
Development

Successfully merging this pull request may close these issues.
3 participants
@hoodmane @ryanking13 @rth