clawpack / visclaw

Clawpack visualization tools
http://www.clawpack.org
BSD 3-Clause "New" or "Revised" License
29 stars 48 forks source link

need to import reload also in Python 2 now #204

Closed rjleveque closed 7 years ago

rjleveque commented 7 years ago

202 breaks Python 2 version, as the test below shows. The solution is to always import reload from somewhere. I guess imp is the right place, but @ketch or @quantheory might want to check this.

def test():
    import sys
    if sys.version_info[0] >= 3:
        if sys.version_info[1] >= 4:
            from importlib import reload
        else:
            from imp import reload
    reload(sys)

gives:

Traceback (most recent call last):
  File "test1.py", line 12, in <module>
    test()
  File "test1.py", line 10, in test
    reload(sys)
UnboundLocalError: local variable 'reload' referenced before assignment
ketch commented 7 years ago

This is bizarre! It definitely appears to be a bug (in Python): statements inside an 'if' that don't get executed turn out to have an effect on the rest of the program. I'm seeing the same behavior.

ketch commented 7 years ago

Here's something even spookier. The code below fails with the same error!

def test():
    import sys
    reload(sys)
    if sys.version_info[0] >= 3:
        if sys.version_info[1] >= 4:
            from importlib import reload
        else:
            from imp import reload

But of course this runs fine:

def test():
    import sys
    reload(sys)
    if sys.version_info[0] >= 3:
        pass
mandli commented 7 years ago

This must be some sort of weird scope problem as taking this out of the function test the original code works.

mandli commented 7 years ago

Is there a different interpretation of the from stuff import thing in 2 vs. 3? Maybe this behavior was corrected in Python 3?

mandli commented 7 years ago

This works:

#!/usr/bin/env python

import sys

def test():
    import sys
    if sys.version_info[0] == 2:
        pass
    elif sys.version_info[0] >= 3:
        if sys.version_info[1] >= 4:
            import importlib.reload
            importlib.reload(sys)
            reload = importlib.reload
        else:
            import imp.reload
            imp.reload(sys)
            reload = imp.reload

test()
reload(sys)

Perhaps something with sys is going awry?

ketch commented 7 years ago

I don't think it's about sys. Here is something even crazier. This fails with the same error:

def test():
    import numpy
    reload(numpy)
    if sys.version_info[0] == 2:
        pass
    elif sys.version_info[0] >= 3:
        if sys.version_info[1] >= 4:
            from importlib import reload
        else:
            from imp import reload

It never gets to complain about sys being undefined because it chokes on the call to reload!

ketch commented 7 years ago

In any case, @mandli's last version seems like a good way to go.

ketch commented 7 years ago

So, this also fails with the same error:

def test():
    range(5)
    if True:
        pass
    elif False:
        from six.moves import range

It looks like any "from ... import" statement inside a function can cause a built-in to become undefined, even if it never gets executed.

rjleveque commented 7 years ago

It was reload that was undefined in my example, not sys, and the last example from @ketch shows this clearly. The example from @mandli avoids this since the reload(sys) is outside of the function. If you move it inside, the same problem arises.

ketch commented 7 years ago

Ah, so then it's not about the "from .. import" syntax either. It's a side effect of having any code that could redefine a built-in. This also gives the same error:

def test():
    range(5)
    if True:
        pass
    elif False:
        range = 0
ketch commented 7 years ago

I think that

from imp import reload

may break for older 2.x versions of Python. I think we can solve this by just moving the reload imports to the top of the file (outside any function), which in any case follows the PEP8 convention.

ketch commented 7 years ago

Also, I learned from Jed Brown that Python determines scope at bytecode-compile time. So after you define test(), if you look at test.__code__.co_varnames you'll see listed anything that is/could be defined inside test(). Apparently those things are forced to be undefined when you enter the function.

quantheory commented 7 years ago

Python determines scope at bytecode-compile time.

That would seem to be why the order of statements and location in if lines is irrelevant. It seems like the "real" problem here is that Python is trying too hard to identify local variables, so I like @ketch's solution of just moving the import outside of the function.

Yet another fix:

    global reload
    if sys.version_info[0] >= 3:
        if sys.version_info[1] >= 4:
            from importlib import reload
        else:
            from imp import reload

However, I believe that this is equivalent to just moving the import lines outside of the function entirely.

quantheory commented 7 years ago

Oh, and @rjleveque, this is Sean Santos. I just realized that you probably don't recognize me from this username.

donnaaboise commented 7 years ago

I just ran into the same problem - what is the status of this bug?

xinshengqin commented 7 years ago

I ran into the same problem and Kyle directed me here. For now I am using python 2.7 so I just commented out those imports. I will wait for David's solution getting merged to origin/master.

donnaaboise commented 7 years ago

I am also using 2.7, and just rolled back visclaw to the tag v5.4.0 and things seem to work.

mandli commented 7 years ago

@ketch has a fix in #205 but is has yet to be tested. Doing that now.

mandli commented 7 years ago

205 has now been merged in. I am going to close this PR.