classification
Title: Pickling of classes with a metaclass and copy_reg
Type: behavior Stage: resolved
Components: Extension Modules, Library (Lib) Versions: Python 3.3, Python 3.2, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Brent.Payne, alexandre.vassalotti, bsilverthorn, craigcitro, dalleyg, daniel.urban, eric.araujo, fbissey, jackdied, ncoghlan, nlhepler, pitrou, python-dev
Priority: normal Keywords: needs review, patch

Created on 2010-01-13 09:32 by craigcitro, last changed 2011-10-04 17:15 by ncoghlan. This issue is now closed.

Files
File name Uploaded Description Edit
dynamic_class_copyreg.patch craigcitro, 2010-01-13 09:32 patch against trunk r77421 review
issue7689.patch daniel.urban, 2011-03-13 10:01 Updated patch for py3k review
Messages (18)
msg97702 - (view) Author: Craig Citro (craigcitro) Date: 2010-01-13 09:32
Currently, it's impossible to use the usual pickle mechanisms to pickle a dynamically created class, even if the user requests a different pickling mechanism via copy_reg. The attached patch makes this customization possible by simply transposing two blocks of code. 

Longer explanation: Classes are pickled by name, which is of course problematic when trying to pickle a dynamically created class. The natural solution would be to create a __reduce__ method on the metaclass. However, as mentioned in issue 494904, this won't work -- the issue is that for any class C with a metaclass M, C.__reduce__ is the unbound method for instances of class C, as opposed to the bound method M.__reduce__ for C (i.e. viewing C as an instance of M). Guido's patch on that ticket does the sensible thing, which is force the class to be pickled by name -- this is fine, until you want to pickle a class that's created at runtime. 

The copy_reg module exists to handle custom pickling of objects, which is exactly what's needed here. However, the code from #494904 that checks for instances of a metaclass does this *just before* it looks at the copy_reg dispatch table. The patches just reorder these tests in pickle.py and cPickle.c, so that one can register a custom pickler for instances of a metaclass. 

Comments: First, let me preemptively say that I do indeed have a use case for this fix. We ran into this bug working on Sage (http://www.sagemath.org), where we create lots of dynamic classes at runtime (to model mathematical relationships between different kinds of objects). There's a healthy mix of dynamic and non-dynamic classes floating around, and we want the user to be able to pickle them without having to know anything about how the class was created. We could create our own pickling function, but we'd much rather just use the default Python mechanisms, especially since it's such a small fix.

Second, this patch is fairly "safe," in the sense that it's pretty unlikely it can break any existing code. The only way it could break is if someone created a dynamic class, registered a pickler for it with copy_reg, and was depending on calls to pickle to fail. 

Third, there's a few extra lines of code in the chunk the patch moves around coming from ticket #502085. These are there to deal with versions of Boost circa 2002. Is it worth removing these?

The attached patch is against trunk (r77421). At least as I write this, the same patch should apply against the py3k branch (up to renaming copy_reg to copyreg, anyway), but I'm happy to do the legwork of rebasing it as needed. 

Also, I'd happily review something else in exchange for a review of this. We'll be keeping this patch around in Sage until it gets merged, so the sooner the better.

Authors: Nicolas Thiery (nthiery@users.sf.net) first hunted this down and wrote a patch for cPickle.c. I did the (fairly trivial) work of mirroring the fix in pickle.py, and added the tests in pickletester.
msg107954 - (view) Author: Gerald Dalley (dalleyg) Date: 2010-06-16 20:49
Another use case: for distributed processing, it's handy to be able to pickle interactive functions and functions that are part of a script.  The user can then remotely execute a broader set of functions than can be pickled by default.  This is especially helpful for interactive exploration of data.

Pickling most types of functions is possible by serializing a function's bytecode, etc. and registering a handler with copy_reg.  Unfortunately, this handler is ignored under some conditions (e.g. copy_reg is ignored when attempting to serialize top-level functions in a script) but the copy_reg handler does get used for lambdas and inner functions.  This patch looks like it will allow FunctionType's pickle handler to be overridden in all cases.
msg107956 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2010-06-16 21:08
Thanks for your report and patch. I’m editing the versions field: New features and bug fixes go to the active branch, py3k (future 3.2), while only security and documentation fixes are allowed to go in stable branches (2.6 and 3.1). Current trunk (2.7) is frozen, since it’s in release candidate stage; if your bug is fixed in py3k really soon, we’ll ask the release manager Benjamin Peterson if he agrees to let it into 2.7. Thanks again!
msg112245 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2010-08-01 01:10
I was mistaken: bug fixes go in stable releases too.
msg112247 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2010-08-01 01:12
Not an expert, but the Python parts of your patch look good to me.
msg130733 - (view) Author: Daniel Urban (daniel.urban) * Date: 2011-03-13 10:01
Attaching an updated patch for py3k.

> Not an expert, but the Python parts of your patch look good to me.
Me neither, but the C parts also look good to me. The tests fail without the patch, succeed with it.

Note, that it is possible, that the copy module also should be fixed similarly (but currently that can't even copy non-dynamic classes with a metaclass, see issue11480).
msg144856 - (view) Author: Lance Hepler (nlhepler) Date: 2011-10-04 05:42
Hello all, sorry to be a bother, but what's the progress on this issue? I have a codebase that requires resolution of this issue to enable multiprocessing. What are the remaining outstanding problems herein preventing the attached patches from being merged?
msg144857 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-10-04 06:04
+    def __eq__(self, other):
+        r = (type(self) == type(other))
+        if r:
+            return r

I think this should be "if not r".
msg144858 - (view) Author: Craig Citro (craigcitro) Date: 2011-10-04 06:57
Antoine -- why do you want to switch "if r" for "if not r"? 

If we did, the test would just confirm that the unpicked object was of the same type as the original; if we were going to do that, we might as well just replace the whole `__cmp__` function with just `return cmp(type(self), type(other))`. On the flipside, I could see an argument for adding *more* content to the test, but that seemed like overkill.
msg144859 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-10-04 07:03
Craig: I'm talking about the __eq__ version (durban's patch). The __cmp__ version is probably fine.
msg144860 - (view) Author: Craig Citro (craigcitro) Date: 2011-10-04 07:13
Antoine -- ah, that makes sense. Is that the only blocker? I've let this patch rot on the vine a long time; if so, I'll happily switch `__eq__` back to `__cmp__` and re-post if it'll get submitted.
msg144862 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-10-04 07:25
No need, I'll do it myself.
msg144863 - (view) Author: Roundup Robot (python-dev) Date: 2011-10-04 07:34
New changeset 760ac320fa3d by Antoine Pitrou in branch '3.2':
Issue #7689: Allow pickling of dynamically created classes when their
http://hg.python.org/cpython/rev/760ac320fa3d

New changeset 46c026a5ccb9 by Antoine Pitrou in branch 'default':
Issue #7689: Allow pickling of dynamically created classes when their
http://hg.python.org/cpython/rev/46c026a5ccb9
msg144864 - (view) Author: Roundup Robot (python-dev) Date: 2011-10-04 07:39
New changeset 64053bd79590 by Antoine Pitrou in branch '2.7':
Issue #7689: Allow pickling of dynamically created classes when their
http://hg.python.org/cpython/rev/64053bd79590
msg144865 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-10-04 07:40
This is fixed now, thank you!
msg144900 - (view) Author: Brent Payne (Brent.Payne) Date: 2011-10-04 16:53
will the 2.7 patch also be incorporated into a 2.7 release?
msg144903 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-10-04 17:05
Yes, it will.
msg144906 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2011-10-04 17:15
Specifically, 2.7.3. A date for that has not yet been set, but somewhere in the December/January time frame is likely.
History
Date User Action Args
2011-10-04 17:15:31ncoghlansetmessages: + msg144906
2011-10-04 17:05:49pitrousetmessages: + msg144903
2011-10-04 16:53:08Brent.Paynesetmessages: + msg144900
2011-10-04 07:40:54pitrousetstatus: open -> closed
versions: - Python 3.1
messages: + msg144865

resolution: fixed
stage: patch review -> resolved
2011-10-04 07:39:14python-devsetmessages: + msg144864
2011-10-04 07:34:55python-devsetnosy: + python-dev
messages: + msg144863
2011-10-04 07:25:10pitrousetmessages: + msg144862
2011-10-04 07:13:12craigcitrosetmessages: + msg144860
2011-10-04 07:03:05pitrousetmessages: + msg144859
2011-10-04 06:57:21craigcitrosetmessages: + msg144858
2011-10-04 06:04:22pitrousetmessages: + msg144857
2011-10-04 05:42:02nlheplersetmessages: + msg144856
2011-10-03 22:08:17Brent.Paynesetnosy: + Brent.Payne
2011-10-02 10:51:42nlheplersetnosy: + nlhepler
2011-03-14 15:59:33pitrousetnosy: + ncoghlan
2011-03-13 10:01:01daniel.urbansetfiles: + issue7689.patch
versions: + Python 3.3
nosy: pitrou, jackdied, alexandre.vassalotti, bsilverthorn, eric.araujo, daniel.urban, craigcitro, dalleyg, fbissey
messages: + msg130733

components: + Extension Modules
2011-03-08 21:06:19fbisseysetnosy: + fbissey
2011-03-06 09:47:48daniel.urbansetnosy: + daniel.urban
2010-08-01 01:12:41eric.araujosetnosy: + pitrou
messages: + msg112247
2010-08-01 01:10:16eric.araujosetmessages: + msg112245
versions: + Python 3.1, Python 2.7
2010-06-16 21:08:58eric.araujosetversions: - Python 2.6, Python 2.5, Python 3.1, Python 2.7
nosy: + eric.araujo

messages: + msg107956

keywords: + needs review
stage: patch review
2010-06-16 20:49:32dalleygsetnosy: + dalleyg
messages: + msg107954
2010-05-13 01:06:56bsilverthornsetnosy: + bsilverthorn
2010-02-18 21:18:15jackdiedsetnosy: + jackdied
2010-01-15 15:50:00pitrousetnosy: + alexandre.vassalotti
2010-01-13 09:32:13craigcitrocreate