-
-
Notifications
You must be signed in to change notification settings - Fork 31.1k
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
Reload semantics changed unexpectedly in Python 3.3 #63612
Comments
PJE brought up concerns on python-dev regarding PEP-451 and module reloading. [1] However, the issue isn't with the PEP changing reload semantics (mostly). Those actually changed with the switch to importlib (and a pure Python reload function) in the 3.3 release. Nick sounded positive on fixing it, while Brett did not sound convinced it is worth it. I'm +1 as long as it isn't too complicated to fix. While we hash that out, here's a patch that hopefully demonstrates it isn't too complicated. :) [1] https://mail.python.org/pipermail/python-dev/2013-October/129863.html |
It's actually even simpler than that - we can just go back to ignoring the __loader__ attribute entirely and always searching for a new one, since we want to pick up changes to the import hooks, even for modules with a __loader__ already set (which is almost all of them in 3.3+) I'm not sure it's worth fixing in 3.3 though, as opposed to just formally specifying the semantics in PEP-451 (as noted on python-dev). |
Here's an updated patch. |
Patch mostly looks good to me, but there should be a second test ensuring that the loader attribute gets *replaced*, even if it is already set to something else. A similar test structure to the existing one should work, but replacing the "del types.__loader__" with: expected_loader_type = type(types.__loader__)
types.__loader__ = "this will be replaced by the reload" and then later:
|
Brett: any opinions on fixing this? 3.3? Nick: I'll add another test when I get a chance. |
Just had a thought on a possible functional test case:
|
Fine with fixing it, but in context of PEP-451, not 3.3. |
I'm fine with not fixing this for 3.3. Does this need to wait on PEP-451 acceptance? |
Failing test case showing that Python 3.3 can't reload a module that is converted to a package behind importlib's back (I like this better than the purely introspection based tests, since it shows a real, albeit obscure, regression due to this behavioural change): $ ./broken_reload.py
E ====================================================================== Traceback (most recent call last):
File "./broken_reload.py", line 28, in test_module_to_package
imp.reload(mod)
File "/usr/lib64/python3.3/imp.py", line 271, in reload
return module.__loader__.load_module(name)
File "<frozen importlib._bootstrap>", line 586, in _check_name_wrapper
File "<frozen importlib._bootstrap>", line 1024, in load_module
File "<frozen importlib._bootstrap>", line 1005, in load_module
File "<frozen importlib._bootstrap>", line 562, in module_for_loader_wrapper
File "<frozen importlib._bootstrap>", line 855, in _load_module
File "<frozen importlib._bootstrap>", line 950, in get_code
File "<frozen importlib._bootstrap>", line 1043, in path_stats
FileNotFoundError: [Errno 2] No such file or directory: '/tmp/tmp_n48mm/to_be_reloaded.py' Ran 1 test in 0.002s FAILED (errors=1) Interactive session showing that import.c didn't have this problem, since it reran the whole search (foo is just a toy module I had lying around in my play directory): $ python
Python 2.7.5 (default, Oct 8 2013, 12:19:40)
[GCC 4.8.1 20130603 (Red Hat 4.8.1-1)] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import foo
Hello
>>> foo.__file__
'foo.py'
>>> import os
>>> os.mkdir("foo")
>>> os.rename('foo.py', 'foo/__init__.py')
>>> reload(foo)
Hello
<module 'foo' from 'foo/__init__.py'>
>>> foo.__file__
'foo/__init__.py'
>>> |
No, the fix can go into Python 3.4 right now. |
New changeset 88c3a1a3c2ff by Eric Snow in branch 'default': |
As you can see, Nick, I came up with a test that did just about the same thing (which you had suggested earlier :-) ). For good measure I also added a test that replaces a namespace package with a normal one. |
Looks like this broke on windows: ====================================================================== Traceback (most recent call last):
File "D:\cygwin\home\db3l\buildarea\3.x.bolen-windows\build\lib\test\test_importlib\test_api.py", line 283, in test_reload_namespace_changed
[os.path.dirname(bad_path)] * 2)
AssertionError: Lists differ: ['C:\[46 chars]spam'] != ['C:\[46 chars]spam', 'C:\\DOCUME~1\\db3l\\LOCALS~1\\Temp\\tmpxhxk6rt9\\spam'] Second list contains 1 additional elements.
+ ['C:\\DOCUME~1\\db3l\\LOCALS~1\\Temp\\tmpxhxk6rt9\\spam', + 'C:\\DOCUME~1\\db3l\\LOCALS~1\\Temp\\tmpxhxk6rt9\\spam'] |
New changeset 78d36d54391c by Eric Snow in branch 'default': |
Windows looks happy now. I'll look into the duplicate portions separately in bpo-19469. |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: