classification
Title: 3.4 cherry-pick: 587fd4b91120 improve Enum subclass behavior
Type: behavior Stage: commit review
Components: Versions: Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: larry Nosy List: barry, eli.bendersky, ethan.furman, larry, serhiy.storchaka
Priority: release blocker Keywords:

Created on 2014-02-18 20:47 by ethan.furman, last changed 2014-02-23 06:54 by larry. This issue is now closed.

Messages (14)
msg211552 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2014-02-18 20:47
587fd4b91120: Better pickle support for Enum subclasses.
msg211595 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-02-19 03:23
I'd like a second opinion about this before it goes in, just due to the size of the patch.
msg211708 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-02-20 07:32
Besides insignificant style nitpick ("if classdict.get('__reduce_ex__') is None" can be written as "if '__reduce_ex__' not in classdict") the patch LGTM at first glance. I'm not very familiar with Enum's complicated machinery and don't sure that no bugs left here (I'm not sure that we should sabotage pickling at all, other types have no such paranoid guard), but at least I'm sure this patch doesn't make things worse. On other hand, this doesn't look as critical bugfix and may be can wait for 3.4.1. Ethan's, Eli's and Barry's opinions have more weight in this matter.
msg211724 - (view) Author: Eli Bendersky (eli.bendersky) * (Python committer) Date: 2014-02-20 13:44
I left some comments in #20653

As for cherry-picking this into 3.4, I'm not sure. Ethan - what is the worst scenario this patch enables to overcome? Someone getting locked in to by-value pickling with certain enums in 3.4?
msg211792 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2014-02-21 02:34
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 as it supports all the protocol levels, and is always used [1].  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.
msg211801 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2014-02-21 04:56
Larry,

If I make changes to the patch, should I reverse the original and then commit one new one so you only have one to merge in, or do you mind having two to do?
msg211803 - (view) Author: Eli Bendersky (eli.bendersky) * (Python committer) Date: 2014-02-21 05:14
The discussion in #20653 is ongoing but I have to say I don't feel confident about this issue at all.

If anything, I'd prefer to explicitly mark "advanced pickling support" for enums as provisional in 3.4 - this is a simple documentation fix Larry should have no qualms accepting into the RC. We can say that unless folks do funky things, pickling enums should work. But as for support for defining all kinds of strange custom pickling behaviors, this is not supported at this point.

I think we all need some time to think about the interaction of enums with pickling and properly document all the scenarios we want, and - more importantly - don't want to support.

Enum internals are too complex. Interaction with pickles is even trickier. We don't have to make these decisions in a rush right now.

In short, I'm -1 on cherry picking, with +1 on putting provisional disclaimers in strategic places within the enum doc to save us from backwards compatibility worries in this domain for the upcoming cycle.
msg211805 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2014-02-21 05:24
Can we do that?  If a doc change can make it so we're not locked in to supporting __getnewargs__ I could live with that.

Although, to be honest, I'd rather get the change in *and* have a doc warning that pickling specifics are subject to change in 3.5.  Really, getting rid of __getnewargs__ and going with just __reduce_ex__ is a change that started in issue20534, but at that point I still didn't totally grok __reduce_ex__ et al and so didn't complete the change there.
msg211807 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-02-21 06:29
If you've committed something already, and want to change it, then please request both revisions be cherry-picked.
msg211812 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-02-21 07:03
We still have a couple of weeks left.  Ethan, can you start a discussion in python-dev about cherry-picking this for 3.4?
msg211845 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2014-02-21 10:30
Discussion started on PyDev.

While working on that I realized/remembered that the main reason to get this in now is that without it a user *cannot* change how pickling works -- (s)he can write the methods, but they will be ignored.
msg211909 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-02-22 07:30
Okay, I'm accepting this.  I really just wanted to get more eyes on it, and I'm happy to see that python-dev was pretty unanimous.
msg211915 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2014-02-22 08:12
Thanks, Larry.  I'll have one more patch which will be much better comments in the code, and a small doc enhancement.  When it's ready should I reopen this issue or create a new one?
msg211977 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-02-23 06:54
This issue is okay, but it helps my workflow if you'd create a new "3.4 cherry-pick" revision for me when it's ready.
History
Date User Action Args
2014-02-23 06:54:21larrysetmessages: + msg211977
2014-02-22 08:12:09ethan.furmansetmessages: + msg211915
2014-02-22 07:34:43larrysetstatus: open -> closed
resolution: fixed
2014-02-22 07:30:31larrysetmessages: + msg211909
2014-02-21 10:30:52ethan.furmansetmessages: + msg211845
2014-02-21 07:03:05larrysetmessages: + msg211812
2014-02-21 06:29:36larrysetmessages: + msg211807
2014-02-21 05:24:55ethan.furmansetmessages: + msg211805
2014-02-21 05:14:20eli.benderskysetmessages: + msg211803
2014-02-21 04:56:16ethan.furmansetmessages: + msg211801
2014-02-21 02:34:52ethan.furmansetmessages: + msg211792
2014-02-21 02:25:12ethan.furmansetmessages: - msg211739
2014-02-20 18:05:26ethan.furmansetmessages: + msg211739
2014-02-20 13:44:01eli.benderskysetmessages: + msg211724
2014-02-20 07:32:56serhiy.storchakasetnosy: + barry, eli.bendersky, serhiy.storchaka
messages: + msg211708
2014-02-19 03:23:25larrysetmessages: + msg211595
2014-02-18 20:47:05ethan.furmancreate