Currently enums are pickled by values. It means that if the value of enum is platform depending, pickling one enum you can unpickle other enum on other platform.

Here is a patch which makes enum pickling by name. It also get rid of not needed __getnewargs__().

See also discussions in issue20534 and on Python-Dev: http://comments.gmane.org/gmane.comp.python.devel/145536.

On Feb 17, 2014, at 07:03 AM, Serhiy Storchaka wrote:

>
>Currently enums are pickled by values. It means that if the value of enum is
>platform depending, pickling one enum you can unpickle other enum on other
>platform.

It's probably a good idea to pickle by name by default, in order to support
the stdlib use case.  Please just be sure to preserve the ability for user
code to pickle by value via subclassing.

To pickle by value, the subclass needs only restore current implementation of __reduce_ex__():

    def __reduce_ex__(self, proto):
        return self.__class__, (self.value,)

I agree that pickling by name is the better solution.

Serhiy, could you explain how the un-pickling works with protocol 4?

This is new feature of protocol 4 (PEP 3154, section 'Serializing more "lookupable" objects'). When __reduce_ex__() returns a string (instead of a tuple), this is interpreted as the name of a global, and qualnames with dots are now supported in protocol 4.

May be a number of tests which test pickling subclasses with or without some special methods are not needed longer. Fell free to improve my patch if you want to make all cleanup changes in one commit.

For the record I'm against it, but I don't have time to explain until after 3.4 has been released.

This is sad. Because after a release, change it will be much harder.

It should not be changed after the release either.

On Tue, Feb 18, 2014 at 11:57 AM, Serhiy Storchaka
<report@bugs.python.org>wrote:

>
> Serhiy Storchaka added the comment:
>
> This is sad. Because after a release, change it will be much harder.
>
> ----------
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <http://bugs.python.org/issue20653>
> _______________________________________
>

On Feb 18, 2014, at 07:57 PM, Serhiy Storchaka wrote:

>This is sad. Because after a release, change it will be much harder.

OTOH, if default-pickling-by-name could be overridden, so can
default-pickling-by-value.

And it is now possible to override and pickle by name if your custom subclass so chooses.

Patch allows subclass to override __reduce_ex__, which is useful if a mixed-in type does not have proper pickle support.

New changeset 587fd4b91120 by Ethan Furman in branch 'default':
Close issue20653: allow Enum subclasses to override __reduce_ex__
http://hg.python.org/cpython/rev/587fd4b91120

Proposal to switch to pickle by name rejected, but Enum now allows __reduce_ex__ to be overwridden in subclasses.

Ha! You just has committed a patch which I write right now.  Thank you Guido 
for your time machine.

If you want this in 3.4.0, please create a "3.4 cherry-pick" issue for it.

Personally I would feel a lot more confident in the change if there was any evidence of it being reviewed before it was checked in.  All I see is a four-minute window between the patch being uploaded and it being checked in.

Ethan, the patch you committed here seems obscure to me. Why __reduce_ex__ and not __reduce__? Where are the accompanying documentation changes? Can you clarify more how the full logic of pickling now works - preferably in comments withing the code?

When I implemented pickle support I did not have a complete understanding of the pickle protocols nor how to best use them.  As a result, I picked __getnewargs__ and only supported protocols 2 and 3 (4 didn't exist yet).

Serhiy came along and explained a bunch of it to me, so now I know that __reduce_ex__ is the best choice [1] as it supports all the protocol levels, and is always used.  The patch removes __getnewargs__ and makes __reduce_ex__ The One Obvious Way, but if it does not go in to 3.4.0 then I won't be able to remove __getnewargs__ because of backwards compatibility.

All the tests still pass, and the new test for subclassing to pickle by name passes.


[1] pickle supports two low-level methods: __reduce__ and __reduce_ex__; __reduce_ex__ is more powerful and is the preferred method.  If a mix-in class to Enum defines __reduce_ex__ and Enum only defines __reduce__, Enum's __reduce__ will not be called.

More explanation:

__getnewargs__ is not used by pickle protocol 0 and 1; to support those protocols we need __reduce_ex__.  Since __reduce_ex__ works for 0, 1, 2, 3, 4, ... there's no reason to have both __reduce_ex__ *and* __getnewargs__.

If you were enlightened about how to use the pickle protocols, please explains this better in the code. Currently the code says:

         # check for a supported pickle protocols, and if not present sabotage
+        # pickling, since it won't work anyway.
+        # if new class implements its own __reduce_ex__, do not sabotage
+        if classdict.get('__reduce_ex__') is None:
+            if member_type is not object:
+                methods = ('__getnewargs_ex__', '__getnewargs__',
+                        '__reduce_ex__', '__reduce__')
+                if not any(map(member_type.__dict__.get, methods)):
+                    _make_class_unpicklable(enum_class)

The comments aren't very useful since they rephrase in different words what the code does, rather than explaining *why* it's being done. Please provide a patch with improved comments that make the following explicit:

1. What's expected of subclasses that want custom pickling.
2. What __new__ does if subclasses don't provide (1) - this "sabotaging" thing and under what conditions - what is the significance of the "member type is not object" test and the significance of the next test.


@@ -192,8 +194,9 @@ class EnumMeta(type):
         (i.e. Color = Enum('Color', names='red green blue')).
 
         When used for the functional API: `module`, if set, will be stored in
-        the new class' __module__ attribute; `type`, if set, will be mixed in
-        as the first base class.
+        the new class' __module__ attribute; `qualname`, if set, will be stored
+        in the new class' __qualname__ attribute; `type`, if set, will be mixed
+        in as the first base class.

Is it only me, or this comment change is unrelated to the pickling change?

Also, this lacks change in the documentation. Your patch makes it possible for subclasses to provide custom pickling, overriding Enum's pickling. Some place should say explicitly what's expected of such classes (e.g. you rely on __reduce_ex__ taking precedence over __reduce__? what if subclasses rather define __reduce__? is it disallowed?)

I'm not sure why you reopened this issue and retagged it to 3.4 when you've already commiteed the change to default (which is 3.5 now?) and issue #20679 exists explicitly to discuss the cherrypick?

Nits:

  if classdict.get('__reduce_ex__') is None:

can be replaced by:
  
  if '__reduce_ex__' not in classdict:

And:

  if not any(map(member_type.__dict__.get, methods)):

Would be more Pythonic as:

  if not any(m in member_type.__dict__ for m in methods)

Thanks for your comments, Eli, I'll work on getting better comments in the code.

The qualname comment is partially related to the pickling changes as it's necessary for protocol 4 (I forgot to put that comment in on the previous pickling change that addressed that issue).

I reopened because a new (or another) patch with better comments is called for; I tagged it 3.4 because I thought default was still pointing to the 3.4 series.

To answer here your other questions (which I'll also put in comments in the code):

1.  if custom pickling is desired __reduce_ex__ needs to be defined
2.  if (1) is not done, then normal pickle behavior takes place:
    i.  check if this is a mixed or pure Enum
    ii. if mixed, check that mixed-in type (aka member_type) has a pickle
        protocol in place, and if it doesn't, have the resulting enum class
        have __reduce_ex__ be the _break_on_call_reduce method.  We do this
        because even though the class and member will pickle, the member
        will fail to unpickle.
3.  as I said in an earlier message, but didn't make clear:  __reduce_ex__ is
    the preferred method.  This means that if a class has both __reduce__ and
    __reduce_ex__, __reduce__ will be ignored (at least by pickle -- I haven't
    researched copy, which I think also uses __reduce__ and/or __reduce_ex__).
    And of course, if an ancestor class has __reduce_ex__, and a subclass has
    __reduce__, the subclass really has both.  No, __reduce__ is not checked for.

__reduce_ex__ is not the "preferred method": it's only necessary if you want so special-case according to the prototocol number.
In most cases, this is not necessary so it is simpler to define __reduce__ and not __reduce_ex__.

So I think the patch really should make use of __reduce__, not __reduce_ex__.

Many comments, Eli's and Serhey's code changes incorporated.

Antoine,

If the mixed-in class defines __reduce_ex__, and the Enum class defines __reduce__, pickle will see that the Enum class has both, and will call the _ex__ method.  It is, therefore, the preferred method (at least by pickle, which is what we are discussing).  If you don't believe me, try it and see.

> If the mixed-in class defines __reduce_ex__, and the Enum class
> defines __reduce__, pickle will see that the Enum class has both, and
> will call the _ex__ method.

Ah, I understand your concern. You are using "preferred" in a different
sense. The pickle docs don't mention __reduce_ex__ as being preferred,
as in "you should use this one", on the contrary.

Yeah, I was confused by that when I first read it as well.  The 2.7 docs are even worse in that regard (so there has been some progress :).

> Yeah, I was confused by that when I first read it as well.  The 2.7
> docs are even worse in that regard (so there has been some
> progress :).

The docs are actually correct. They may be confusing because the
customization possibilities are more numerous than people may expect.
Rewordings welcome.

> Many comments, Eli's and Serhey's code changes incorporated.

Looks better, thanks. I left some comments in Rietveld.

Antoine commented:
------------------
> The pickle docs don't mention __reduce_ex__ as being preferred, as in "you should
> use this one", on the contrary.


Are we reading the same docs?

http://docs.python.org/dev/library/pickle.html#object.__reduce_ex__
-------------------------------------------------------------------
[...] When defined, pickle will prefer it over the __reduce__() method.

> Antoine commented:
> ------------------
> > The pickle docs don't mention __reduce_ex__ as being preferred, as in "you should
> > use this one", on the contrary.
> 
> 
> Are we reading the same docs?

Are we reading the same comments?

On 02/21/2014 11:26 AM, Antoine Pitrou wrote:
>
> Are we reading the same comments?

LOL, apparently not.

Can you upload the new patch?

New changeset b637064cc696 by Ethan Furman in branch 'default':
Close issue20653: improve functional API docs; minor code changes
http://hg.python.org/cpython/rev/b637064cc696

New changeset 54ab95407288 by Ethan Furman in branch 'default':
Issue20653: fix ReST for Enum
http://hg.python.org/cpython/rev/54ab95407288

New changeset 010723a7bd25 by Ethan Furman in branch '3.4':
Close issue20653: allow Enum subclasses to override __reduce_ex__
http://hg.python.org/cpython/rev/010723a7bd25

New changeset 737f2be5e80c by Ethan Furman in branch '3.4':
Close issue20653: improve functional API docs; minor code changes
http://hg.python.org/cpython/rev/737f2be5e80c

New changeset 2c5a5fa0692c by Ethan Furman in branch '3.4':
Issue20653: fix ReST for Enum
http://hg.python.org/cpython/rev/2c5a5fa0692c