classification
Title: Pickle enums by name
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: ethan.furman Nosy List: alexandre.vassalotti, barry, eli.bendersky, ethan.furman, gvanrossum, larry, pitrou, python-dev, serhiy.storchaka
Priority: high Keywords: patch

Created on 2014-02-17 07:03 by serhiy.storchaka, last changed 2014-03-17 06:30 by python-dev. This issue is now closed.

Files
File name Uploaded Description Edit
enum_pickle_by_name.patch serhiy.storchaka, 2014-02-17 07:03 review
issue20653.stoneleaf.01.patch ethan.furman, 2014-02-18 20:23 review
issue20653.stoneleaf.02.patch ethan.furman, 2014-02-21 10:30 review
issue20653.stoneleaf.03.patch ethan.furman, 2014-02-22 15:48 review
issue20653.stoneleaf.04.patch ethan.furman, 2014-02-22 19:40 review
Messages (35)
msg211394 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-02-17 07:03
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.
msg211420 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2014-02-17 15:27
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.
msg211428 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-02-17 17:19
To pickle by value, the subclass needs only restore current implementation of __reduce_ex__():

    def __reduce_ex__(self, proto):
        return self.__class__, (self.value,)
msg211430 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2014-02-17 17:36
I agree that pickling by name is the better solution.

Serhiy, could you explain how the un-pickling works with protocol 4?
msg211433 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-02-17 17:55
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.
msg211434 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-02-17 17:58
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.
msg211539 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2014-02-18 19:37
For the record I'm against it, but I don't have time to explain until after 3.4 has been released.
msg211540 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-02-18 19:57
This is sad. Because after a release, change it will be much harder.
msg211541 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2014-02-18 19:58
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>
> _______________________________________
>
msg211542 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2014-02-18 20:04
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.
msg211543 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2014-02-18 20:18
And it is now possible to override and pickle by name if your custom subclass so chooses.
msg211545 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2014-02-18 20:23
Patch allows subclass to override __reduce_ex__, which is useful if a mixed-in type does not have proper pickle support.
msg211549 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2014-02-18 20:37
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
msg211550 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2014-02-18 20:39
Proposal to switch to pickle by name rejected, but Enum now allows __reduce_ex__ to be overwridden in subclasses.
msg211555 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-02-18 21:02
Ha! You just has committed a patch which I write right now.  Thank you Guido 
for your time machine.
msg211590 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-02-19 03:19
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.
msg211723 - (view) Author: Eli Bendersky (eli.bendersky) * (Python committer) Date: 2014-02-20 13:42
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?
msg211793 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2014-02-21 02:36
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.
msg211797 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2014-02-21 04:40
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__.
msg211798 - (view) Author: Eli Bendersky (eli.bendersky) * (Python committer) Date: 2014-02-21 04:45
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)
msg211804 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2014-02-21 05:18
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.
msg211843 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-02-21 10:28
__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__.
msg211844 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2014-02-21 10:30
Many comments, Eli's and Serhey's code changes incorporated.
msg211846 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2014-02-21 10:35
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.
msg211847 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-02-21 10:37
> 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.
msg211848 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2014-02-21 10:41
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 :).
msg211849 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-02-21 10:42
> 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.
msg211858 - (view) Author: Eli Bendersky (eli.bendersky) * (Python committer) Date: 2014-02-21 13:42
> Many comments, Eli's and Serhey's code changes incorporated.

Looks better, thanks. I left some comments in Rietveld.
msg211874 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2014-02-21 19:22
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.
msg211875 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-02-21 19:26
> 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?
msg211876 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2014-02-21 19:34
On 02/21/2014 11:26 AM, Antoine Pitrou wrote:
>
> Are we reading the same comments?

LOL, apparently not.
msg211923 - (view) Author: Eli Bendersky (eli.bendersky) * (Python committer) Date: 2014-02-22 13:36
Can you upload the new patch?
msg212669 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2014-03-03 20:43
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
msg212685 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2014-03-03 23:02
New changeset 54ab95407288 by Ethan Furman in branch 'default':
Issue20653: fix ReST for Enum
http://hg.python.org/cpython/rev/54ab95407288
msg213804 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2014-03-17 06:30
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
History
Date User Action Args
2014-03-17 06:30:48python-devsetmessages: + msg213804
2014-03-03 23:02:04python-devsetmessages: + msg212685
2014-03-03 20:43:08python-devsetstatus: open -> closed
resolution: fixed
messages: + msg212669

stage: commit review -> resolved
2014-02-22 19:40:09ethan.furmansetfiles: + issue20653.stoneleaf.04.patch
2014-02-22 15:48:07ethan.furmansetfiles: + issue20653.stoneleaf.03.patch
2014-02-22 13:36:27eli.benderskysetmessages: + msg211923
2014-02-21 19:34:24ethan.furmansetmessages: + msg211876
2014-02-21 19:26:02pitrousetmessages: + msg211875
2014-02-21 19:22:34ethan.furmansetmessages: + msg211874
2014-02-21 13:42:59eli.benderskysetmessages: + msg211858
2014-02-21 10:42:30pitrousetmessages: + msg211849
2014-02-21 10:41:21ethan.furmansetmessages: + msg211848
2014-02-21 10:37:47pitrousetmessages: + msg211847
2014-02-21 10:35:55ethan.furmansetmessages: + msg211846
2014-02-21 10:30:16ethan.furmansetfiles: + issue20653.stoneleaf.02.patch

messages: + msg211844
2014-02-21 10:28:40pitrousetmessages: + msg211843
2014-02-21 05:19:00ethan.furmansetmessages: + msg211804
2014-02-21 04:45:49eli.benderskysetmessages: + msg211798
2014-02-21 04:40:51ethan.furmansetmessages: + msg211797
2014-02-21 02:36:15ethan.furmansetstatus: closed -> open
priority: normal -> high
versions: + Python 3.4, - Python 3.5
messages: + msg211793

resolution: fixed -> (no value)
stage: resolved -> commit review
2014-02-20 13:42:26eli.benderskysetmessages: + msg211723
versions: + Python 3.5, - Python 3.4
2014-02-19 03:19:43larrysetmessages: + msg211590
2014-02-18 21:02:30serhiy.storchakasetmessages: + msg211555
2014-02-18 20:39:32ethan.furmansetpriority: release blocker -> normal

messages: + msg211550
2014-02-18 20:37:07python-devsetstatus: open -> closed

nosy: + python-dev
messages: + msg211549

resolution: fixed
stage: patch review -> resolved
2014-02-18 20:23:16ethan.furmansetfiles: + issue20653.stoneleaf.01.patch

messages: + msg211545
2014-02-18 20:18:31ethan.furmansetmessages: + msg211543
2014-02-18 20:04:28barrysetmessages: + msg211542
2014-02-18 19:58:50gvanrossumsetmessages: + msg211541
2014-02-18 19:57:26serhiy.storchakasetmessages: + msg211540
2014-02-18 19:37:47gvanrossumsetnosy: + gvanrossum
messages: + msg211539
2014-02-17 17:58:30serhiy.storchakasetmessages: + msg211434
2014-02-17 17:55:26serhiy.storchakasetmessages: + msg211433
2014-02-17 17:36:19ethan.furmansetassignee: ethan.furman
messages: + msg211430
2014-02-17 17:19:02serhiy.storchakasetmessages: + msg211428
2014-02-17 15:27:09barrysetmessages: + msg211420
2014-02-17 07:03:56serhiy.storchakacreate