Merge pull request #3555 from takluyver/i3547

Simplify caching of modules with %run.

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.

Closes  #3547, and fixes another test that was marked as a known failure.

IPython/core/interactiveshell.py

@@ -819,53 +819,32 b' class InteractiveShell(SingletonConfigurable):'
     # 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 b' class InteractiveShell(SingletonConfigurable):'
 
         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 b' class InteractiveShell(SingletonConfigurable):'
         # 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 b' class InteractiveShell(SingletonConfigurable):'
         
         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()]
 
     def reset(self, new_session=True):
         """Clear all internal namespaces, and attempt to release references to
@@ -1219,9 +1195,6 b' class InteractiveShell(SingletonConfigurable):'
         # 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.

IPython/core/magics/execution.py

@@ -508,7 +508,7 b' python-profiler package from non-free.""")'
             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 b' python-profiler package from non-free.""")'
             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 b' python-profiler package from non-free.""")'
                 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

IPython/core/tests/test_run.py

@@ -244,7 +244,6 b' class TestMagicRunSimple(tt.TempFileMixin):'
             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 b' class TestMagicRunSimple(tt.TempFileMixin):'
                "   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):

docs/source/whatsnew/development.txt

@@ -48,3 +48,7 b' 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.