Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Allow pickling objects defined interactively. #384

Closed
wants to merge 6 commits into from

3 participants

@takluyver
Owner

This whole area is rather complex, so please be blunt if there's a reason the approach I've taken is completely wrong.

Per issue #29, we'd like to be able to pickle objects we define interactively. The reason we can't is that it looks for locally defined variables in the __main__ module, which doesn't get updated. So we need to make sys.modules[__name__].__dict__ is ip.user_ns True. But we can't assign to the module's __dict__ directly, because it's a read-only attribute.

In the normal case (running a shell), where user_ns is a dict, this creates a FakeModule, and then replaces user_ns with its __dict__ attribute, so that we're working directly in the module namespace. But that doesn't work with the test suite, where user_ns is a dict subclass (ipnsdict from IPython.core.testing.globalipapp). In that case, we keep user_ns as a separate object, and copy its contents across into the module after executing each cell.

The test suite still passes, at least on my machine, and copying and pasting the example code from #29, I can get the object pickled. I've not really gone looking for corner cases.

@fperez
Owner

One thing I keep wondering is whether we could get rid of the whole FakeModule idea in the first place. Do you think it's possible to approach this so we don't build that hack?

@takluyver
Owner

We need to have something module-like if we're going to pickle things, although I don't know how module-like it needs to be. What would we do instead of FakeModule? Just instantiate ModuleType and manipulate it?

@fperez
Owner
@takluyver
Owner

We might be able to simplify the code a bit, but I don't think there's an alternative to creating a fake module of some sort and binding it to the user namespace. Although this is a fairly esoteric corner of Python, so there could well be something that hasn't occurred to any of us.

@fperez
Owner

Code looks good, tests pass for me, interactive fuzzing seems to work fine too.

My only suggestion would be if you could add a few, even relatively naive, pickle tests. We've had so many problems with pickling in the past that it would be nice to have at least a minimal set of smoke tests for: define a class interactively, pickle it, load it in the same session, load it in a different session with plain python (can be done with a simple python -c "import pickle;..." statement run in a subshell).

Even if we don't catch the obscure corner cases, at least it will increase our chances of not causing a dumb regression in the future.

Other than that, good to merge from my side!

@takluyver
Owner

OK, great. I'm going to be real-life busy over the weekend, but I'll aim to add some tests in a few days, unless someone beats me to it.

@fperez
Owner
@takluyver
Owner

Found a bit of time to make a quick test. I don't know if that's the best way to test it, but it seems to work. I didn't try the test against master, but it failed in the expected manner when I commented out a couple of critical lines from these changes.

@fperez
Owner

Looked it over, I think it's fine. Go for the merge, and thanks a lot!

@minrk
Owner

Fernando just reported #387, which is not in master, but actually introduced by this branch.

Don't merge before fixing that.

@fperez
Owner
@takluyver
Owner

Yeah, that's the sort of corner case I'm glad someone found before we pushed it. I've changed the code so that that can't happen, and added a test. But the changes required led to the pickle test failing again (although it still worked interactively), so there were a couple more changes to our test framework (doctests were interfering with the namespaces). All the tests are passing again, and I've checked both pickling and issue #387 interactively.

I've also removed the make_user_namespaces method on InteractiveShell. I think our namespace production is too tied to initialising the shell to easily separate out that part. It's not just a case of making some dictionaries and attaching them as attributes (although that should work if you do, but performance will be worse, because it has to copy the user_ns after each execution).

@takluyver
Owner

I think this is working again (at least, I fixed the bug Fernando found, and checked that pickling still works). I don't know if we want to land it before 0.11 or wait until that's out, since as we found, it's a fairly delicate part of the code.

@takluyver
Owner

@fperez: I recommend leaving this now until after we release 0.11. It's an annoying issue, but the fix might lead to more unforeseen problems. Hopefully 0.11-0.12 will be a shorter cycle than 0.10-0.11.

@fperez
Owner

@takluyver, I agree with your last statement. We're pretty close to 0.11 now that we made progress on the config effort, so let's not drop this potential landmine in there now... We can keep the PR open and tidy (rebasing it as needed so it continues to merge cleanly) and tackle it later. Thanks for all the work on this one!

@fperez
Owner

Superseded by #648, so closing here.

@fperez fperez closed this
@takluyver takluyver referenced this pull request
Merged

Usermod #648

@damianavila damianavila referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
125 IPython/core/interactiveshell.py
@@ -813,17 +813,44 @@ def init_create_namespaces(self, user_ns=None, user_global_ns=None):
# that if you need to access the built-in namespace directly, you
# should start with "import __builtin__" (note, no 's') which will
# definitely give you a module. Yeah, it's somewhat confusing:-(.
-
- # These routines return properly built dicts as needed by the rest of
- # the code, and can also be used by extension writers to generate
- # properly initialized namespaces.
- user_ns, user_global_ns = self.make_user_namespaces(user_ns,
- user_global_ns)
+
+ # We must ensure that __builtin__ (without the final 's') is always
+ # available and pointing to the __builtin__ *module*. For more details:
+ # http://mail.python.org/pipermail/python-dev/2001-April/014068.html
+
+ separate_user_ns = True
+ if user_ns is None:
+ user_ns = {}
+ separate_user_ns = False
+
+ # Set __name__ to __main__ to better match the behavior of the
+ # normal interpreter.
+ user_ns.setdefault('__name__','__main__')
+ user_ns.setdefault('__builtin__',__builtin__)
+ user_ns.setdefault('__builtins__',__builtin__)
+
+ if (user_global_ns is not None) and \
+ not isinstance(user_global_ns, dict):
+ raise TypeError("user_global_ns must be a true dict; got %r"
+ % type(user_global_ns))
# Assign namespaces
- # This is the namespace where all normal user variables live
- self.user_ns = user_ns
- self.user_global_ns = user_global_ns
+ # user_ns the namespace where all normal user variables live
+ self.user_ns_mod = FakeModule(user_ns)
+ if not separate_user_ns:
+ # In the normal case, we replace user_ns with a module dict, so that
+ # user-defined objects are in the __main__ module, and can be pickled.
+ self.user_ns = self.user_ns_mod.__dict__
+ else:
+ # Otherwise, we will need to manually update it after execution.
+ self.user_ns = user_ns
+
+ # The global namespace will normally be identical to the user namespace.
+ # The exceptions are certain embedding cases.
+ if user_global_ns is None:
+ self.user_global_ns = self.user_ns
+ else:
+ self.user_global_ns = user_global_ns
# An auxiliary namespace that checks what parts of the user_ns were
# loaded at startup, so we can list later only variables defined in
@@ -866,8 +893,8 @@ def init_create_namespaces(self, user_ns=None, user_global_ns=None):
# A table holding all the namespaces IPython deals with, so that
# introspection facilities can search easily.
- self.ns_table = {'user':user_ns,
- 'user_global':user_global_ns,
+ self.ns_table = {'user':self.user_ns,
+ 'user_global':self.user_global_ns,
'internal':self.internal_ns,
'builtin':__builtin__.__dict__
}
@@ -880,65 +907,7 @@ def init_create_namespaces(self, user_ns=None, user_global_ns=None):
# clears them manually and carefully.
self.ns_refs_table = [ self.user_ns_hidden,
self.internal_ns, self._main_ns_cache ]
-
- def make_user_namespaces(self, user_ns=None, user_global_ns=None):
- """Return a valid local and global user interactive namespaces.
-
- This builds a dict with the minimal information needed to operate as a
- valid IPython user namespace, which you can pass to the various
- embedding classes in ipython. The default implementation returns the
- same dict for both the locals and the globals to allow functions to
- refer to variables in the namespace. Customized implementations can
- return different dicts. The locals dictionary can actually be anything
- following the basic mapping protocol of a dict, but the globals dict
- must be a true dict, not even a subclass. It is recommended that any
- custom object for the locals namespace synchronize with the globals
- dict somehow.
-
- Raises TypeError if the provided globals namespace is not a true dict.
-
- Parameters
- ----------
- user_ns : dict-like, optional
- The current user namespace. The items in this namespace should
- be included in the output. If None, an appropriate blank
- namespace should be created.
- user_global_ns : dict, optional
- The current user global namespace. The items in this namespace
- should be included in the output. If None, an appropriate
- blank namespace should be created.
-
- Returns
- -------
- A pair of dictionary-like object to be used as the local namespace
- of the interpreter and a dict to be used as the global namespace.
- """
-
-
- # We must ensure that __builtin__ (without the final 's') is always
- # available and pointing to the __builtin__ *module*. For more details:
- # http://mail.python.org/pipermail/python-dev/2001-April/014068.html
-
- if user_ns is None:
- # Set __name__ to __main__ to better match the behavior of the
- # normal interpreter.
- user_ns = {'__name__' :'__main__',
- '__builtin__' : __builtin__,
- '__builtins__' : __builtin__,
- }
- else:
- user_ns.setdefault('__name__','__main__')
- user_ns.setdefault('__builtin__',__builtin__)
- user_ns.setdefault('__builtins__',__builtin__)
-
- if user_global_ns is None:
- user_global_ns = user_ns
- if type(user_global_ns) is not dict:
- raise TypeError("user_global_ns must be a true dict; got %r"
- % type(user_global_ns))
-
- return user_ns, user_global_ns
-
+
def init_sys_modules(self):
# We need to insert into sys.modules something that looks like a
# module but which accesses the IPython namespace, for shelve and
@@ -962,7 +931,7 @@ def init_sys_modules(self):
except KeyError:
raise KeyError('user_ns dictionary MUST have a "__name__" key')
else:
- sys.modules[main_name] = FakeModule(self.user_ns)
+ sys.modules[main_name] = self.user_ns_mod
def init_user_ns(self):
"""Initialize all user-visible namespaces to their minimum defaults.
@@ -1057,6 +1026,7 @@ def reset(self, new_session=True):
# would cause errors in many object's __del__ methods.
for ns in [self.user_ns, self.user_global_ns]:
drop_keys = set(ns.keys())
+ drop_keys.discard('__name__')
drop_keys.discard('__builtin__')
drop_keys.discard('__builtins__')
for k in drop_keys:
@@ -1064,6 +1034,11 @@ def reset(self, new_session=True):
# Restore the user namespaces to minimal usability
self.init_user_ns()
+
+ # In some situations, e.g. testing, our fake main module has a __dict__
+ # separate from the user_ns: if so, we need to clear it manually.
+ if self.user_ns is not self.user_ns_mod.__dict__:
+ init_fakemod_dict(self.user_ns_mod, self.user_ns)
# Restore the default and user aliases
self.alias_manager.clear_aliases()
@@ -2145,7 +2120,13 @@ def run_cell(self, raw_cell, store_history=True):
self.run_ast_nodes(code_ast.body, cell_name,
interactivity="last_expr")
-
+
+ # In the normal case, user_ns is the __dict__ of the
+ # corresponding module. But e.g. in tests, it isn't, so we need
+ # to copy the contents across.
+ if self.user_ns is not self.user_ns_mod.__dict__:
+ init_fakemod_dict(self.user_ns_mod, self.user_ns)
+
# Execute any registered post-execution functions.
for func, status in self._post_execute.iteritems():
if not status:
View
22 IPython/core/tests/test_interactiveshell.py
@@ -91,3 +91,25 @@ def test_magic_names_in_string(self):
ip = get_ipython()
ip.run_cell('a = """\n%exit\n"""')
self.assertEquals(ip.user_ns['a'], '\n%exit\n')
+
+ def test_can_pickle(self):
+ ip = get_ipython()
+ ip.run_cell(("class Mylist(list):\n"
+ " def __init__(self,x=[]):\n"
+ " list.__init__(self,x)"))
+ ip.run_cell("w=Mylist([1,2,3])")
+
+ from cPickle import dumps
+ res = dumps(ip.user_ns["w"])
+ self.assertTrue(isinstance(res, bytes))
+
+ def test_global_ns(self):
+ ip = get_ipython()
+ ip.run_cell("a = 10")
+ ip.run_cell(("def f(x):"
+ " return x + a"))
+ ip.run_cell("b = f(12)")
+ self.assertEqual(ip.user_ns["b"], 22)
+
+
+
View
5 IPython/testing/globalipapp.py
@@ -82,6 +82,9 @@ def __init__(self,*a):
# namespace in doctests that call '_'.
self.protect_underscore = False
+ # We set this so that the tests don't clash with __main__
+ self["__name__"] = "__test_main__"
+
def clear(self):
dict.clear(self)
self.update(self._savedict)
@@ -158,7 +161,7 @@ def start_ipython():
# Create and initialize our test-friendly IPython instance.
shell = TerminalInteractiveShell.instance(config=config,
user_ns=ipnsdict(),
- user_global_ns={}
+ user_global_ns=None
)
# A few more tweaks needed for playing nicely with doctests...
View
4 IPython/testing/plugin/ipdoctest.py
@@ -271,6 +271,8 @@ def setUp(self):
# for IPython examples *only*, we swap the globals with the ipython
# namespace, after updating it with the globals (which doctest
# fills with the necessary info from the module being tested).
+ self.user_ns_orig = {}
+ self.user_ns_orig.update(_ip.user_ns)
_ip.user_ns.update(self._dt_test.globs)
self._dt_test.globs = _ip.user_ns
# IPython must protect the _ key in the namespace (it can't exist)
@@ -286,6 +288,8 @@ def tearDown(self):
# teardown doesn't destroy the ipython namespace
if isinstance(self._dt_test.examples[0],IPExample):
self._dt_test.globs = self._dt_test_globs_ori
+ _ip.user_ns.clear()
+ _ip.user_ns.update(self.user_ns_orig)
# Restore the behavior of the '_' key in the user namespace to
# normal after each doctest, so that unittests behave normally
_ip.user_ns.protect_underscore = False
Something went wrong with that request. Please try again.