-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
Do not always import * with autoreload3 #14872
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
base: main
Are you sure you want to change the base?
Conversation
09e8eb3 to
918f1b0
Compare
918f1b0 to
df9cb12
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @jaysarva, this looks good to me (unit tests included make it much easier to review!).
I will run copilot on this PR to test it out, let's see if this is useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes the behavior of %autoreload 3 so that only explicitly imported symbols are reloaded instead of always reloading all module contents. It also adds unit tests to verify the behavior and updates the autoreload machinery to track "from X import Y" imports.
- Fix autoreload 3 to update only explicitly imported symbols.
- Add comprehensive tests for various import scenarios.
- Update the autoreload code to support import-from tracking.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/test_zzz_autoreload.py | Added tests covering different import variants and overloading scenarios for autoreload 3 |
| IPython/extensions/autoreload.py | Extended the autoreload mechanism to track imported symbols using AST parsing, and updated superreload to apply the new behavior |
Comments suppressed due to low confidence (1)
tests/test_zzz_autoreload.py:54
- [nitpick] Since FakeShell already initializes user_ns["In"], verify whether this duplicate initialization is needed for these tests or if it can be removed to avoid redundancy.
self.user_ns["In"] = []
IPython/extensions/autoreload.py
Outdated
| if name in symbol_map[module.__name__]: | ||
| name = symbol_map[module.__name__][name] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To avoid potential KeyErrors if module.__name__ is not present in symbol_map, consider using symbol_map.get(module.__name__, {}) in the conditional check.
| if name in symbol_map[module.__name__]: | |
| name = symbol_map[module.__name__][name] | |
| if name in symbol_map.get(module.__name__, {}): | |
| name = symbol_map.get(module.__name__, {})[name] |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was a good catch, I was able to invoke an exception on runtime:
although this was not even a KeyError but the fact that symbol_map can be None. I guess mypy should have caught it? I guess the problem is that superreload is not typed so mypy did not run on it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for catching this! It seems I had some local changes that I didn't add to this pr so my testing was still passing :) I believe this should no longer be a problem, as we do a check on symbol_map to make sure it isn't null. @krassowski
The PR introduces a critical bug as of now.
1d26aad to
6b98baf
Compare
Fixes #14839
Previously,
%autoreload 3always had the effect offrom module import *on reloads. This PR fixes this functionality to the expected utility: if users specifically import some symbols from a module (for examplefrom X import Yorfrom X import Y as Z), only those symbols are reloaded.Unit tests are added to demonstrate the new behavior as well.