classification
Title: Reload semantics changed unexpectedly in Python 3.3
Type: behavior Stage: committed/rejected
Components: Library (Lib) Versions: Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: eric.snow Nosy List: Arfrever, brett.cannon, eric.snow, ncoghlan, pje, python-dev
Priority: normal Keywords: patch

Created on 2013-10-27 03:16 by eric.snow, last changed 2013-11-01 06:12 by eric.snow. This issue is now closed.

Files
File name Uploaded Description Edit
reload-semantics.diff eric.snow, 2013-10-29 02:06 review
broken_reload.py ncoghlan, 2013-10-31 11:53 Failing test case for the 3.3+ import system
Messages (15)
msg201407 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2013-10-27 03:16
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
msg201411 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2013-10-27 03:48
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).
msg201600 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2013-10-29 02:06
Here's an updated patch.
msg201619 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2013-10-29 10:02
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:

    assertIs(type(types.__loader__), expected_loader_type)
msg201690 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2013-10-29 22:41
Brett: any opinions on fixing this?  3.3?

Nick: I'll add another test when I get a chance.
msg201704 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2013-10-30 02:56
Just had a thought on a possible functional test case:
- write a module file
- load it
- check for expected attributes
- move it from name.py to name/__init__.py
- reload it
- check for new expected attributes
msg201740 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2013-10-30 14:32
Fine with fixing it, but in context of PEP 451, not 3.3.
msg201795 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2013-10-31 08:48
I'm fine with not fixing this for 3.3.  Does this need to wait on PEP 451 acceptance?
msg201805 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2013-10-31 11:53
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
======================================================================
ERROR: test_module_to_package (__main__.TestBadReload)
----------------------------------------------------------------------
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'
>>>
msg201817 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2013-10-31 14:32
No, the fix can go into Python 3.4 right now.
msg201876 - (view) Author: Roundup Robot (python-dev) Date: 2013-11-01 04:35
New changeset 88c3a1a3c2ff by Eric Snow in branch 'default':
Issue #19413: Restore pre-3.3 reload() semantics of re-finding modules.
http://hg.python.org/cpython/rev/88c3a1a3c2ff
msg201877 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2013-11-01 04:37
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.
msg201878 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2013-11-01 05:43
Looks like this broke on windows:

======================================================================
FAIL: test_reload_namespace_changed (test.test_importlib.test_api.Source_ReloadTests)
----------------------------------------------------------------------
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.
First extra element 1:
C:\DOCUME~1\db3l\LOCALS~1\Temp\tmpxhxk6rt9\spam

- ['C:\\DOCUME~1\\db3l\\LOCALS~1\\Temp\\tmpxhxk6rt9\\spam']
?                                                         ^

+ ['C:\\DOCUME~1\\db3l\\LOCALS~1\\Temp\\tmpxhxk6rt9\\spam',
?                                                         ^

+  'C:\\DOCUME~1\\db3l\\LOCALS~1\\Temp\\tmpxhxk6rt9\\spam']
msg201879 - (view) Author: Roundup Robot (python-dev) Date: 2013-11-01 05:50
New changeset 78d36d54391c by Eric Snow in branch 'default':
Issue #19413: Disregard duplicate namespace portions during reload tests.
http://hg.python.org/cpython/rev/78d36d54391c
msg201881 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2013-11-01 06:11
Windows looks happy now.  I'll look into the duplicate portions separately in issue19469.
History
Date User Action Args
2013-11-01 06:12:22eric.snowsetstatus: pending -> closed
2013-11-01 06:11:57eric.snowsetstatus: open -> pending

messages: + msg201881
2013-11-01 05:50:02python-devsetmessages: + msg201879
2013-11-01 05:43:54eric.snowsetmessages: + msg201878
2013-11-01 05:42:58eric.snowsetstatus: pending -> open
2013-11-01 04:37:52eric.snowsetstatus: open -> pending
messages: + msg201877

assignee: eric.snow
resolution: fixed
stage: patch review -> committed/rejected
2013-11-01 04:35:08python-devsetnosy: + python-dev
messages: + msg201876
2013-10-31 14:32:31brett.cannonsetmessages: + msg201817
2013-10-31 11:53:39ncoghlansetfiles: + broken_reload.py

messages: + msg201805
2013-10-31 08:48:31eric.snowsetmessages: + msg201795
versions: - Python 3.3
2013-10-30 14:32:31brett.cannonsetmessages: + msg201740
2013-10-30 02:56:37ncoghlansetmessages: + msg201704
2013-10-29 22:41:00eric.snowsetmessages: + msg201690
2013-10-29 10:02:10ncoghlansetmessages: + msg201619
2013-10-29 02:06:53eric.snowsetfiles: - reload-semantics.diff
2013-10-29 02:06:42eric.snowsetfiles: + reload-semantics.diff

messages: + msg201600
2013-10-28 23:22:31Arfreversetnosy: + Arfrever
2013-10-27 03:48:14ncoghlansetmessages: + msg201411
2013-10-27 03:16:31eric.snowcreate