Allow pickling objects defined interactively. #384
Conversation
|
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? |
|
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? |
|
On Thu, Apr 14, 2011 at 3:30 PM, takluyver
Don't really know. Perhaps we'll just be back to FakeModule :) I If we can't do any better no worries. Cheers, f |
|
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. |
|
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! |
|
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. |
|
On Thu, Apr 14, 2011 at 4:45 PM, takluyver
No problem; I'll be in the same boat and mostly offline... No worries. Thanks for the great work! |
|
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. |
|
Looked it over, I think it's fine. Go for the merge, and thanks a lot! |
|
Fernando just reported #387, which is not in master, but actually introduced by this branch. Don't merge before fixing that. |
|
On Fri, Apr 15, 2011 at 4:04 PM, minrk
Yup, along with a test for it :) |
|
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 |
|
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. |
|
@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. |
|
@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! |
|
Superseded by #648, so closing here. |
… user_ns (#14754) ## What changes are proposed in this pull request? Some IPython apps, such as the one used in Databricks, need to seed the shell with a custom user namespace before it is actually created. This means the main module used by ipython can’t be a normal ModuleType (which doesn’t support overwriting __dict__) but instead a custom “DummyMod”. DummyMod isn’t considered an instance of ModuleType, which can mess up tools like ipytest + pytest -- see, for example, pytest-dev/pytest#12965. To fix, in this PR we create a helper function that generates a `ModuleType` subclass on-the-fly and that uses the user namespace provided a priori as its `__dict__`. Note that `ModuleType.__dict__` is read-only, which is probably why IPython originally created DummyMod. To get around that, we create a `__dict__` property and use custom getters / setters to delegate to the user_ns we pass in. Searching around, it seems like `DummyMod` / `FakeModule` may have caused some pain points in the past, e.g. around pickling (#384, #112) -- still trying to determine if this change can help with that. ## How is this tested? Manual + pre-merge
3 participants
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 makesys.modules[__name__].__dict__ is ip.user_nsTrue. 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.