Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Simplify caching of modules with %run #3555

Merged
merged 6 commits into from

4 participants

@takluyver
Owner

Previously, we cleared and re-used a single FakeModule instance in which to run scripts, and cached copies of the modules' namespaces to prevent them from being cleared. Now, we cache one FakeModule instance per script file, clearing it and re-using it if the same script is re-run.

I've assumed that new_main_mod and cache_main_mod don't count as public APIs; they're certainly bizarre corners that I hope no-one else has needed to use directly. Fixing this would be much harder if we have to preserve their signatures.

This closes #3547, and also fixes another test that was marked as a known failure. (Entirely clean test run on IPython.core!)

Pinging @fperez, as I think you're the most familiar with this code.

takluyver added some commits
@takluyver takluyver Add failing test for gh-3547 601235a
@takluyver takluyver Simplify caching of modules from %run-ing scripts.
Previously, we cleared and re-used a single FakeModule instance in which to
run cells, and cached copies of the modules' namespaces to prevent them from
being cleared. Now, we cache one FakeModule instance per script file, clearing
it and re-using it if the same script is re-run. This seems more robust.

Closes gh-3547
279211b
@takluyver takluyver Another passing test :-) bc2b914
@takluyver takluyver Minor tweak to test for Python 3 453a739
@minrk
Owner

Thanks, this all looks good to me, and cleaner than before. I will defer to @fperez as someone more familiar with %run and FakeModule for merge confirmation.

@takluyver
Owner

Since this is a breaking API change, I've added a note to the What's New doc in case any poor soul is calling this.

@fperez fperez was assigned
@Carreau
Owner

Explicitly assigning @fperez for his feedback.

IPython/core/interactiveshell.py
@@ -819,16 +819,9 @@ def register_post_execute(self, func):
# Things related to the "main" module
#-------------------------------------------------------------------------
- def new_main_mod(self,ns=None):
+ def new_main_mod(self,filename):
"""Return a new 'main' module object for user code execution.
@fperez Owner
fperez added a note

We should document the filename parameter in the docstring properly.

@takluyver Owner

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@fperez fperez commented on the diff
IPython/core/interactiveshell.py
@@ -1174,8 +1148,8 @@ def all_ns_refs(self):
Note that this does not include the displayhook, which also caches
objects from the output."""
- return [self.user_ns, self.user_global_ns,
- self._user_main_module.__dict__] + self._main_ns_cache.values()
+ return [self.user_ns, self.user_global_ns] + \
+ [m.__dict__ for m in self._main_mod_cache.values()]
@fperez Owner
fperez added a note

Isn't there a logic issue here? This walks the entire cache that now contains the namespaces for all scripts that have been run, so there will be 'leakage' from variables from one script into another... Unless I'm misunderstanding something, this seems like a problem in the current logic.

@takluyver Owner

I don't think so. This is the all_ns_refs property, which gives us references to every namespace that IPython knows about. It's only used by del_var() and reset_selective() to hunt and kill all variables of a given name.

Also, it's doing the same thing that it was before - _main_ns_cache already stored a copy of the namespace for every script that had been run.

@fperez Owner
fperez added a note

Agreed, thanks for the extra details. You're right, and there's no real issue here.

Merging now. Good job!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@fperez
Owner

I believe there's a problem with the logic, pointed above. Let's not merge this until we can either clarify that I'm wrong, or fix the issue if I'm right.

@fperez fperez merged commit 6217a47 into from
@takluyver takluyver referenced this pull request
Merged

Drop fakemodule #3622

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jul 5, 2013
  1. @takluyver

    Add failing test for gh-3547

    takluyver authored
  2. @takluyver

    Simplify caching of modules from %run-ing scripts.

    takluyver authored
    Previously, we cleared and re-used a single FakeModule instance in which to
    run cells, and cached copies of the modules' namespaces to prevent them from
    being cleared. Now, we cache one FakeModule instance per script file, clearing
    it and re-using it if the same script is re-run. This seems more robust.
    
    Closes gh-3547
  3. @takluyver

    Another passing test :-)

    takluyver authored
  4. @takluyver
Commits on Jul 7, 2013
  1. @takluyver
Commits on Jul 9, 2013
  1. @takluyver
This page is out of date. Refresh to see the latest.
View
85 IPython/core/interactiveshell.py
@@ -819,53 +819,32 @@ def register_post_execute(self, func):
# Things related to the "main" module
#-------------------------------------------------------------------------
- def new_main_mod(self,ns=None):
+ def new_main_mod(self, filename):
"""Return a new 'main' module object for user code execution.
- """
- main_mod = self._user_main_module
- init_fakemod_dict(main_mod,ns)
- return main_mod
-
- def cache_main_mod(self,ns,fname):
- """Cache a main module's namespace.
-
- When scripts are executed via %run, we must keep a reference to the
- namespace of their __main__ module (a FakeModule instance) around so
- that Python doesn't clear it, rendering objects defined therein
- useless.
+
+ ``filename`` should be the path of the script which will be run in the
+ module. Requests with the same filename will get the same module, with
+ its namespace cleared.
+
+ When scripts are executed via %run, we must keep a reference to their
+ __main__ module (a FakeModule instance) around so that Python doesn't
+ clear it, rendering references to module globals useless.
This method keeps said reference in a private dict, keyed by the
- absolute path of the module object (which corresponds to the script
- path). This way, for multiple executions of the same script we only
- keep one copy of the namespace (the last one), thus preventing memory
- leaks from old references while allowing the objects from the last
- execution to be accessible.
-
- Note: we can not allow the actual FakeModule instances to be deleted,
- because of how Python tears down modules (it hard-sets all their
- references to None without regard for reference counts). This method
- must therefore make a *copy* of the given namespace, to allow the
- original module's __dict__ to be cleared and reused.
-
-
- Parameters
- ----------
- ns : a namespace (a dict, typically)
-
- fname : str
- Filename associated with the namespace.
-
- Examples
- --------
-
- In [10]: import IPython
-
- In [11]: _ip.cache_main_mod(IPython.__dict__,IPython.__file__)
-
- In [12]: IPython.__file__ in _ip._main_ns_cache
- Out[12]: True
+ absolute path of the script. This way, for multiple executions of the
+ same script we only keep one copy of the namespace (the last one),
+ thus preventing memory leaks from old references while allowing the
+ objects from the last execution to be accessible.
"""
- self._main_ns_cache[os.path.abspath(fname)] = ns.copy()
+ filename = os.path.abspath(filename)
+ try:
+ main_mod = self._main_mod_cache[filename]
+ except KeyError:
+ main_mod = self._main_mod_cache[filename] = FakeModule()
+ else:
+ init_fakemod_dict(main_mod)
+
+ return main_mod
def clear_main_mod_cache(self):
"""Clear the cache of main modules.
@@ -877,17 +856,17 @@ def clear_main_mod_cache(self):
In [15]: import IPython
- In [16]: _ip.cache_main_mod(IPython.__dict__,IPython.__file__)
+ In [16]: m = _ip.new_main_mod(IPython.__file__)
- In [17]: len(_ip._main_ns_cache) > 0
+ In [17]: len(_ip._main_mod_cache) > 0
Out[17]: True
In [18]: _ip.clear_main_mod_cache()
- In [19]: len(_ip._main_ns_cache) == 0
+ In [19]: len(_ip._main_mod_cache) == 0
Out[19]: True
"""
- self._main_ns_cache.clear()
+ self._main_mod_cache.clear()
#-------------------------------------------------------------------------
# Things related to debugging
@@ -1017,10 +996,7 @@ def init_create_namespaces(self, user_module=None, user_ns=None):
# and clear_main_mod_cache() methods for details on use.
# This is the cache used for 'main' namespaces
- self._main_ns_cache = {}
- # And this is the single instance of FakeModule whose __dict__ we keep
- # copying and clearing for reuse on each %run
- self._user_main_module = FakeModule()
+ self._main_mod_cache = {}
# A table holding all the namespaces IPython deals with, so that
# introspection facilities can search easily.
@@ -1174,8 +1150,8 @@ def all_ns_refs(self):
Note that this does not include the displayhook, which also caches
objects from the output."""
- return [self.user_ns, self.user_global_ns,
- self._user_main_module.__dict__] + self._main_ns_cache.values()
+ return [self.user_ns, self.user_global_ns] + \
+ [m.__dict__ for m in self._main_mod_cache.values()]
@fperez Owner
fperez added a note

Isn't there a logic issue here? This walks the entire cache that now contains the namespaces for all scripts that have been run, so there will be 'leakage' from variables from one script into another... Unless I'm misunderstanding something, this seems like a problem in the current logic.

@takluyver Owner

I don't think so. This is the all_ns_refs property, which gives us references to every namespace that IPython knows about. It's only used by del_var() and reset_selective() to hunt and kill all variables of a given name.

Also, it's doing the same thing that it was before - _main_ns_cache already stored a copy of the namespace for every script that had been run.

@fperez Owner
fperez added a note

Agreed, thanks for the extra details. You're right, and there's no real issue here.

Merging now. Good job!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
def reset(self, new_session=True):
"""Clear all internal namespaces, and attempt to release references to
@@ -1219,9 +1195,6 @@ def reset(self, new_session=True):
# execution protection
self.clear_main_mod_cache()
- # Clear out the namespace from the last %run
- self.new_main_mod()
-
def del_var(self, varname, by_name=False):
"""Delete a variable from the various namespaces, so that, as
far as possible, we're not keeping any hidden references to it.
View
11 IPython/core/magics/execution.py
@@ -508,7 +508,7 @@ def run(self, parameter_s='', runner=None,
prog_ns = self.shell.user_ns
__name__save = self.shell.user_ns['__name__']
prog_ns['__name__'] = '__main__'
- main_mod = self.shell.new_main_mod(prog_ns)
+ main_mod = self.shell.user_module
else:
# Run in a fresh, empty namespace
if 'n' in opts:
@@ -516,7 +516,10 @@ def run(self, parameter_s='', runner=None,
else:
name = '__main__'
- main_mod = self.shell.new_main_mod()
+ # The shell MUST hold a reference to prog_ns so after %run
+ # exits, the python deletion mechanism doesn't zero it out
+ # (leaving dangling references). See interactiveshell for details
+ main_mod = self.shell.new_main_mod(filename)
prog_ns = main_mod.__dict__
prog_ns['__name__'] = name
@@ -593,10 +596,6 @@ def run():
if 'i' in opts:
self.shell.user_ns['__name__'] = __name__save
else:
- # The shell MUST hold a reference to prog_ns so after %run
- # exits, the python deletion mechanism doesn't zero it out
- # (leaving dangling references).
- self.shell.cache_main_mod(prog_ns, filename)
# update IPython interactive namespace
# Some forms of read errors on the file may mean the
View
22 IPython/core/tests/test_run.py
@@ -244,7 +244,6 @@ def test_obj_del(self):
err = None
tt.ipexec_validate(self.fname, 'object A deleted', err)
- @dec.skip_known_failure
def test_aggressive_namespace_cleanup(self):
"""Test that namespace cleanup is not too aggressive GH-238
@@ -258,11 +257,26 @@ class secondtmp(tt.TempFileMixin): pass
" try:\n"
" ip.magic('run %s')\n"
" except NameError as e:\n"
- " print i;break\n" % empty.fname)
- self.mktmp(py3compat.doctest_refactor_print(src))
+ " print(i)\n"
+ " break\n" % empty.fname)
+ self.mktmp(src)
_ip.magic('run %s' % self.fname)
_ip.run_cell('ip == get_ipython()')
- nt.assert_equal(_ip.user_ns['i'], 5)
+ nt.assert_equal(_ip.user_ns['i'], 4)
+
+ def test_run_second(self):
+ """Test that running a second file doesn't clobber the first, gh-3547
+ """
+ self.mktmp("avar = 1\n"
+ "def afunc():\n"
+ " return avar\n")
+
+ empty = tt.TempFileMixin()
+ empty.mktmp("")
+
+ _ip.magic('run %s' % self.fname)
+ _ip.magic('run %s' % empty.fname)
+ nt.assert_equal(_ip.user_ns['afunc'](), 1)
@dec.skip_win32
def test_tclass(self):
View
4 docs/source/whatsnew/development.txt
@@ -48,3 +48,7 @@ Backwards incompatible changes
:func:`IPython.utils.doctestreload.doctest_reload` to make doctests run
correctly inside IPython. Python releases since those versions are unaffected.
For details, see :ghpull:`3068` and `Python issue 8048 <http://bugs.python.org/issue8048>`_.
+* The ``InteractiveShell.cache_main_mod()`` method has been removed, and
+ :meth:`~IPython.core.interactiveshell.InteractiveShell.new_main_mod` has a
+ different signature, expecting a filename where earlier versions expected
+ a namespace. See :ghpull:`3555` for details.
Something went wrong with that request. Please try again.