Update deepreload to use a rewritten knee.py. Fixes dreload(numpy). #1457

Merged
merged 7 commits into from Apr 18, 2012

Projects

None yet

2 participants

@bfroehle

knee.py, a Python re-implementation of hierarchical module import was
removed from the standard library because it no longer functioned properly.

deepreload.py is little more than a hacked version of knee.py which overrides
__builtin__.__import__ to ensure that each module is re-imported once (before
just referring to sys.modules as usual).

In addition, os.path was added to the default excluded modules, since
somehow it has an entry in sys.modules without `os' being a package.

I've posted my original re-written version of knee.py at https://gist.github.com/1944819. This would close gh-32.

@fperez
IPython member

@bfroehle, I'm really sorry for the long delay in reviewing this, but March was insane with travel and conferences. This is absolutely fantastic, thanks!!!

I'd only suggest that you also add a simple test that does import numpy; dreload(numpy), which would fail with current master and pass with your changes. That can be protected with a decorator so it only runs if people have numpy installed, we have such decorators already available, so you just have to do

from IPython.testing import decorators as dec
@dec.skipif_not_numpy
def test_deepreload():
  import numpy
  dreload(numpy)
bfroehle added some commits Feb 29, 2012
@bfroehle bfroehle Update deepreload to use a rewritten knee.py. Fixes dreload(numpy).
knee.py, a Python re-implementation of hierarchical module import was
removed from the standard library because it no longer functioned properly.

deepreload.py is little more than a hacked version of knee.py which overrides
__builtin__.__import__ to ensure that each module is re-imported once (before
just referring to sys.modules as usual).

In addition, `os.path` was added to the default excluded modules, since
somehow it has an entry in sys.modules without `os' being a package.
c0ad013
@bfroehle bfroehle Keep original function names. 47c768c
@bfroehle bfroehle Add deepreload unit test.
Thanks to Fernando for the suggestion and starting code.
b0f53e4
@bfroehle

Thanks for the unit test suggestion. The code provided worked, except that I needed to add 'unittest' to the list of excluded modules to prevent an error.

I suppose there should be a way to test that the deep reloading works, e.g.

def test_deepreload():
    import something_which_imports_string as m # What to choose here?
    x = string.Template('')

    dreload(m, exclude=['string'])
    nt.assert_is_instance(x, string.Template)

    dreload(m)
    nt.assert_not_is_instance(x, string.Template)
@fperez
IPython member

Yes, that would be possible, but I consider that to be above and beyond the expected call of duty here :) You'd have to create dynamically two modules in temporary files, modify the 2nd by rewriting the file, calling dreload() and then checking that the new values are correct. It's not really difficult, just some extra work. If you want to do that let me know and I'll wait, otherwise I think it's OK to proceed with merging.

@bfroehle

The unit test need not be that complicated. As I alluded to above, we could check verify that the module was reloaded by seeing if an instance of a class of the original module is no longer an instance of the class of the reloaded module.

--- A.py ---
class Object(object):
    pass

--- B.py ---
import A

--- test_deepreload.py ---
import nose.tools as nt
def test_deepreload():
    import A
    import B # B imports A
    obj = A.Object()

    dreload(B, exclude=['A'])
    nt.assert_is_instance(obj, A.Object)

    dreload(B)
    nt.assert_not_is_instance(obj, A.Object)

However I'm not convinced that it's worth creating this elaborate framework (of several modules) for this one test. So I think we can just go ahead with the merge.

@fperez
IPython member

good point. Since you have it basically written already (and making those two files just requires this code:

import tempfile
tmp = tempfile.mkdtemp()
import sys
from os.path import join
sys.path.insert(0, tmp)
with open(join(tmp, 'A.py'), 'w') as f:
    f.write('class Object(object):\n    pass\n')
with open(join(tmp, 'B.py'), 'w') as f:
    f.write('import A')
try:
    # do rest of test here
    import A # etc...
finally:
    import shutil
    shutil.rmtree(tmp)

I guess we might as well put it in :) Do you want to go ahead and finish it? It will mean that this code at least has one real test for it, which does help.

@fperez
IPython member

ps - the above is a quick draft, the stdlib imports should go atop the main file, as usual

@bfroehle

Okay, I think this is pretty much done. Do I need to cleanup the addition to sys.path? Other IPython tests are pretty inconsistent in this manner, some doing it, and others not.

@fperez
IPython member

Sorry, forgot to do that. Yes, it's probably a good idea to do undo the sys.path change. We should probably just have a context manager for that:

with temp_sys_path():
  sys.path.whatever()
  ...
# back to normal sys.path

but don't worry about it, just undo it with a sys.path.pop(0) in the finally clause.

bfroehle added some commits Apr 18, 2012
@bfroehle bfroehle Fix deepreload tests in Python 2.6.
Apparently assert_is_instance and assert_not_is_instance are not available
in Python 2.6.
807f609
@bfroehle bfroehle Clean up sys.path entry. 2e6a444
@bfroehle

Apparently such a context manager exists: IPython.utils.syspathcontext.prepended_to_syspath. Unfortunately we cannot use a more compact syntax since we need Python 2.6 compatibility:

with TemporaryDirectory() as tmpdir, prepended_to_syspath(tmpdir):
    ...
@fperez
IPython member

Looks great, thanks! Merging now. Thanks a ton for your patience despite my delayed reply...

@fperez fperez merged commit a206ee2 into ipython:master Apr 18, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment