Simplify caching of modules with %run #3555

Merged
merged 6 commits into from Jul 11, 2013

Projects

None yet

4 participants

@takluyver
IPython member

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 Jul 5, 2013
@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
IPython member

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
IPython member

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 Jul 9, 2013
@Carreau
IPython member

Explicitly assigning @fperez for his feedback.

@fperez fperez and 1 other commented on an outdated diff Jul 9, 2013
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
fperez Jul 9, 2013

We should document the filename parameter in the docstring properly.

@fperez fperez commented on the diff Jul 9, 2013
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
fperez Jul 9, 2013

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
takluyver Jul 9, 2013

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
fperez Jul 11, 2013

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

Merging now. Good job!

@fperez
IPython member

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 ipython:master Jul 11, 2013

1 check passed

Details default The Travis CI build passed
@takluyver takluyver referenced this pull request Jul 12, 2013
Merged

Drop fakemodule #3622

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