Usermod #648

Merged
merged 13 commits into from Nov 27, 2011

3 participants

@takluyver
IPython member

This is intended to supersede my PR #384, where I made it possible to pickle interactively defined objects. While I was working on that, I realised that our distinction between local and global namespaces is a bit unclear (because they're usually the same thing). This goes some way towards clearing that up.

A global namespace should always be tied to a module: pickle accesses classes via the module in which they're defined. So I've changed the arguments for instantiating an InteractiveShell to include user_module in place of user_global_ns. The global namespace simply becomes a reference to user_module.__dict__.

For instantiating InteractiveShell, there are four possibilities:

  • Neither user_ns nor user_module is given. A new (real) module is created named __main__, and its __dict__ becomes the global and local namespace. This is what happens when starting IPython normally.
  • Only user_module is given. Its __dict__ becomes the global and local namespace.
  • Both user_ns and user_module are given. user_module.__dict__ is the global namespace, and user_ns is the local namespace. Note that we can't interactively define closures over variables in the local namespace (this seems to be a limitation of Python).
  • Only user_ns is given. It is treated as the global and local namespace, and a DummyMod object is created to refer to it. This is intended as a convenience, especially for the test suite. The recommended way to pass in a single global namespace is as a reference to the module.

embed() digs out the locals and the module from the frame in which it's called.

While I was doing namespaces, I also re-included a reference to the InteractiveShell, as a weakref proxy, named _ipy.

@takluyver
IPython member

This now works with embedding in a terminal program, but there's an odd exception. You can modify mutable values and define new local variables, but it doesn't rebind existing names. In the example below, doing b=77 in the embedded shell appears to work, and b becomes 77 for the lifetime of the shell. But when you leave the shell, the script still sees b as 66.

from IPython import embed

a = 12
def f():
    b = 66
    c = []
    embed()

    print locals()
    print globals()

f()
@takluyver
IPython member

Also, if I define d in the interactive shell, it shows up when I do print locals(), but print d throws a NameError. I think this is because Python detects local variables when compiling the function, so it doesn't look for d in the locals.

@fperez
IPython member

Should we close #384 and focus only on this one then?

@takluyver
IPython member

If you're happy with this approach, yes.

@fperez
IPython member

OK, I'm closing #384 then, and we'll focus on this one instead. I do like the idea of being more careful about how we handle namespaces, so I vote for this. Unfortunately I can't finish a full review of this one right now, but I'll try to get to it in the evening. Thanks!

@takluyver
IPython member

OK, great. Actually, I'm just going to revert a couple of commits that I no longer like, so if you've checked this branch out, be ready to fetch it again.

@minrk
IPython member

We had wanted this one to sit in master for a while before release, given its potential implications. Do we want to get on this, as we want 0.12 by December?

@takluyver
IPython member

I'm not too concerned about it being deferred. I think the main visible benefit it gives is that you can pickle objects defined interactively (#29). It could potentially lead to some small API breakage, but I don't think most software that embeds it passes a user_global_ns, so it shouldn't be too drastic.

@fperez
IPython member

@takluyver, I see a merge conflict on this one now... If you have a chance to clean it up, I'd be happy to try and work on it so we can merge sooner rather than later...

@takluyver
IPython member

Rebased and run the test suite on 2.7 and 3.2.

@fperez fperez and 1 other commented on an outdated diff Nov 27, 2011
IPython/core/tests/test_interactiveshell.py
+ # We need to swap in our main module - this is only necessary
+ # inside the test framework, because IPython puts the interactive module
+ # in place (but the test framework undoes this).
+ _main = sys.modules['__main__']
+ sys.modules['__main__'] = ip.user_module
+ try:
+ res = dumps(ip.user_ns["w"])
+ finally:
+ sys.modules['__main__'] = _main
+ self.assertTrue(isinstance(res, bytes))
+
+ def test_global_ns(self):
+ "Code in functions must be able to access variables outside them."
+ ip = get_ipython()
+ ip.run_cell("a = 10")
+ ip.run_cell(("def f(x):"
@fperez
IPython member
fperez added a note Nov 27, 2011

though this code is correct as-is, you probably meant to have a \n at the end here, no? It would make it more natural and in case anyone ever adds for some reason one more line, it would continue to work where as without newlines one more would be a problem.

@takluyver
IPython member

You're right - thanks. Sorted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@fperez fperez merged commit a1e4911 into ipython:master Nov 27, 2011
@fperez fperez added a commit to fperez/ipython that referenced this pull request Nov 27, 2011
@fperez fperez Fix critical bug with pylab support inadvertently introduced in #648.
code used it as a dict.  Updated that code to handle a dict correctly,
and added tests to catch this issue in the future (also increases test
coverage of pylab code).
cd84e0e
@fperez fperez referenced this pull request Nov 27, 2011
Merged

Pylab fix #1052

@mattvonrocketstein mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014
@fperez fperez Fix critical bug with pylab support inadvertently introduced in #648.
code used it as a dict.  Updated that code to handle a dict correctly,
and added tests to catch this issue in the future (also increases test
coverage of pylab code).
66db0de
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment