classification
Title: Enum tests fail with pickle protocol 4
Type: behavior Stage: resolved
Components: Versions: Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: ethan.furman Nosy List: alexandre.vassalotti, barry, eli.bendersky, ethan.furman, pitrou, python-dev, serhiy.storchaka
Priority: high Keywords: patch

Created on 2014-02-06 21:25 by ethan.furman, last changed 2014-02-08 19:36 by python-dev. This issue is now closed.

Files
File name Uploaded Description Edit
issue20534.stoneleaf.01.patch ethan.furman, 2014-02-06 23:24 review
enum_pickle.patch serhiy.storchaka, 2014-02-07 06:31 review
issue20534.stoneleaf.02.patch ethan.furman, 2014-02-07 19:26 review
issue20534.stoneleaf.03.patch ethan.furman, 2014-02-08 13:00 review
Messages (20)
msg210419 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2014-02-06 21:25
enum34, the Enum backport, specifically uses `protocol=HIGHEST_PROTOCOL`, while the current enum tests just use the default.

Running the enum34 test expose an issue with pickle protocol 4:

======================================================================
ERROR: test_subclasses_with_getnewargs (__main__.TestEnum)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "test_enum.py", line 1022, in test_subclasses_with_getnewargs
    self.assertEqual(loads(dumps(NI5, protocol=HIGHEST_PROTOCOL)), 5)
_pickle.PicklingError: Can't pickle <class '__main__.TestEnum.test_subclasses_with_getnewargs.<locals>.NamedInt'>: attribute lookup TestEnum.test_subclasses_with_getnewargs.<locals>.NamedInt on __main__ failed

======================================================================
ERROR: test_tuple_subclass (__main__.TestEnum)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "test_enum.py", line 1100, in test_tuple_subclass
    self.assertTrue(loads(dumps(SomeTuple.first, protocol=HIGHEST_PROTOCOL)) is SomeTuple.first)
_pickle.PicklingError: Can't pickle <enum 'SomeTuple'>: attribute lookup TestEnum.test_tuple_subclass.<locals>.SomeTuple on __main__ failed
msg210420 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-02-06 21:37
Pickle tests should test all protocols in range(pickle.HIGHEST_PROTOCOL + 1).
msg210421 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2014-02-06 21:44
Working on fixing tests now.  Not sure I can fix pickle (at least not in time for RC1).
msg210425 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-02-06 22:09
Those tracebacks don't correspond to line numbers in the stdlib enum.py.

Regardless, the two tests fail understandably: they define a class in the local namespace and then monkeypatch globals() with it, but don't adjust the __qualname__.
msg210426 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2014-02-06 23:24
Thanks, Antoine, that was what I needed.
msg210429 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2014-02-07 01:24
Serhiy, the minimum supported pickle protocol is 2.  Now testing all protocols from 2 to HIGHEST_PROTOCOL, inclusive.
msg210430 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2014-02-07 01:28
New changeset 35f57ab9389b by Ethan Furman in branch 'default':
Close issue20534: test_enum now tests all supported pickle protocols (2 - HIGHEST_PROTOCOL, inclusive).
http://hg.python.org/cpython/rev/35f57ab9389b
msg210435 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-02-07 06:31
But why the minimum supported pickle protocol is 2? Here is a patch which makes enums pickleable with all protocols.
msg210490 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2014-02-07 16:45
Because I didn't know how to make it work with 0 and 1.  Thank you!  I'll get that in today.
msg210533 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2014-02-07 19:26
Okay, I went with __reduce__ since we don't ever use the pickle protocol information.

I added a test for class-nested Enums since protocol 4 supports it.

I left the test for test_subclasses_without_getnewargs alone as the point of that test is to make sure that _make_class_unpicklable is working properly, not to see if we can somehow get any of it to pickle.

Running all tests now.
msg210536 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2014-02-07 19:36
I also removed the protocol 2 warning from the docs, and added this note:

.. note::

    With pickle protocol version 4 it is possible to easily pickle enums
    nested in other classes.
msg210568 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-02-07 22:12
Pickle will prefer the __reduce_ex__() method over the __reduce__() method. If base class defined the __reduce_ex__() method, Enum.__reduce__() will be ignored.

> I left the test for test_subclasses_without_getnewargs alone as the point of that test is to make sure that _make_class_unpicklable is working properly, not to see if we can somehow get any of it to pickle.

Without these changes pickling failed, but not because _make_class_unpicklable.

I added other comments on Rietveld.
msg210577 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2014-02-07 23:57
The version 2 docs:
http://docs.python.org/2/library/pickle.html#object.__reduce_ex__:
-------------------------------------------------------------------------
The object class implements both __reduce__() and __reduce_ex__();
however, if a subclass overrides __reduce__() but not __reduce_ex__(),
the __reduce_ex__() implementation detects this and calls __reduce__().
-------------------------------------------------------------------------

The current docs may say the same thing, but much less clearly:
http://docs.python.org/dev/library/pickle.html#object.__reduce_ex__:
-------------------------------------------------------------------------
Alternatively, a __reduce_ex__() method may be defined. The only
difference is this method should take a single integer argument, the
protocol version. When defined, pickle will prefer it over the
__reduce__() method. In addition, __reduce__() automatically becomes
a synonym for the extended version. The main use for this method is to
provide backwards-compatible reduce values for older Python releases.
-------------------------------------------------------------------------
msg210578 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2014-02-08 00:00
Ethan commented:
----------------
> I left the test for test_subclasses_without_getnewargs alone as the point of
> that test is to make sure that _make_class_unpicklable is working properly,
> not to see if we can somehow get any of it to pickle.

Serhiy replied:
---------------
> Without these changes pickling failed, but not because _make_class_unpicklable.

Well, the direct cause is that the Enum class in question failed to define __getnewargs__; the indirect cause is that without __getnewargs__ the metaclass will call _make_class_unpicklable.

So neither the enum nor its members should pickle.
msg210591 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-02-08 07:11
> -------------------------------------------------------------------------
> The object class implements both __reduce__() and __reduce_ex__();
> however, if a subclass overrides __reduce__() but not __reduce_ex__(),
> the __reduce_ex__() implementation detects this and calls __reduce__().
> -------------------------------------------------------------------------

This is about default  object.__reduce_ex__() implementation. But base class 
can override it.

>>> import enum, pickle
>>> class C(int):
...     def __reduce_ex__(self, proto):
...         return C, (int(self),)
... 
>>> class E(C):
...     X = C(42)
... 
>>> y = C(42)
>>> pickle.loads(pickle.dumps(y))
42
>>> type(pickle.loads(pickle.dumps(y)))
<class '__main__.C'>
>>> pickle.loads(pickle.dumps(E.X))
42
>>> type(pickle.loads(pickle.dumps(E.X)))
<class '__main__.C'>

Well, the direct cause is that the Enum class in question failed to define 
__getnewargs__; the indirect cause is that without __getnewargs__ the 
metaclass will call _make_class_unpicklable.

> Well, the direct cause is that the Enum class in question failed to define
> __getnewargs__; the indirect cause is that without __getnewargs__ the
> metaclass will call _make_class_unpicklable.
> 
> So neither the enum nor its members should pickle.

Actually they don't pickle due to wrong __module__. After adding

    NEI.__module__ = NamedInt.__module__ = __name__

PicklingError no longer raised for enum class.

And "BadPickle.__qualname__ = 'BadPickle'" in test_exploding_pickle is 
redundant, because BadPickle.__qualname__ already is 'BadPickle' (due to the 
module argument in constructor).
msg210623 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2014-02-08 13:00
The only thing I hate more than being wrong is being wrong because Python isn't acting the way I think it should.  :/

So, __qualname__ is not set properly when using the function API (although it has nothing to do with manually setting __module__ (I removed it, tested, no difference)).

And, if a subclass defines __reduce__, but an ancestor class defines __reduce_ex__, the ancestor class wins -- this seems to completely defeat the purpose of subclassing.

I'll add a 'qualname' parameter (mirroring the 'module' parameter) to the function API, and I'll use __reduce_ex__.

A little experimenting shows that if the base class implements any of __reduce__, __reduce_ex__, __getnewargs__, or __getnewargs_ex__ that pickling will work, so modified that portion of Enum as well.
msg210629 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2014-02-08 13:08
Serhiy commented:
-----------------
> Actually they don't pickle due to wrong __module__. After adding
> 
>    NEI.__module__ = NamedInt.__module__ = __name__
> 
> PicklingError no longer raised for enum class.

If you were to look at the `_make_class_unpicklable` function you would see it works by setting __module__ to <unknown>.  So, in fact, it does not pickle precisely because it has the wrong module, and your change undoes what Enum was tryng to do: if an instance cannot be pickled, don't let the class be pickled either.
msg210634 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-02-08 13:31
I don't know if this was discussed, but as far as enums will be used for platform depending constants (as ENOENT or AF_UNIX), I think that enums should be pickled by name, not by value. Here is the __reduce_ex__ implementation:

    def __reduce_ex__(self, proto):
        if proto < 4:
            return getattr, (self.__class__, self._name_)
        return self.__class__.__qualname__ + '.' + self._name_
msg210644 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2014-02-08 13:57
I do not recall if it was or not.  The main difference would be in how aliases were handled.  For example, if W and Z were the same value on system A, but different on system B, then going from A - B via pickle W and Z would still be the same using the current methods (pickling by value).

Considering that on system A, W and Z are two names for the exact same object, and that with SomeEnum(xx) the 'xx' is a value, how could we pickle just the name and be able to unpickle on sytem B?
msg210677 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2014-02-08 19:36
New changeset 9f75f8a2cbb4 by Ethan Furman in branch 'default':
Close issue20534: all pickle protocols now supported.
http://hg.python.org/cpython/rev/9f75f8a2cbb4
History
Date User Action Args
2014-02-08 19:36:24python-devsetstatus: open -> closed
resolution: fixed
messages: + msg210677

stage: patch review -> resolved
2014-02-08 13:57:43ethan.furmansetmessages: + msg210644
2014-02-08 13:31:20serhiy.storchakasetmessages: + msg210634
2014-02-08 13:08:43ethan.furmansetmessages: + msg210629
2014-02-08 13:00:47ethan.furmansetfiles: + issue20534.stoneleaf.03.patch

nosy: + eli.bendersky
messages: + msg210623

resolution: fixed -> (no value)
2014-02-08 07:11:43serhiy.storchakasetmessages: + msg210591
2014-02-08 00:00:55ethan.furmansetmessages: + msg210578
2014-02-07 23:57:15ethan.furmansetmessages: + msg210577
2014-02-07 22:12:12serhiy.storchakasetmessages: + msg210568
2014-02-07 19:36:29ethan.furmansetmessages: + msg210536
2014-02-07 19:26:15ethan.furmansetfiles: + issue20534.stoneleaf.02.patch

messages: + msg210533
2014-02-07 16:45:26ethan.furmansetstatus: closed -> open

messages: + msg210490
stage: resolved -> patch review
2014-02-07 06:31:32serhiy.storchakasetfiles: + enum_pickle.patch

messages: + msg210435
2014-02-07 01:28:48python-devsetstatus: open -> closed

nosy: + python-dev
messages: + msg210430

resolution: fixed
stage: patch review -> resolved
2014-02-07 01:24:45ethan.furmansetmessages: + msg210429
2014-02-06 23:24:27ethan.furmansetfiles: + issue20534.stoneleaf.01.patch
messages: + msg210426

assignee: ethan.furman
keywords: + patch
stage: needs patch -> patch review
2014-02-06 22:09:29pitrousetmessages: + msg210425
2014-02-06 21:44:45ethan.furmansetmessages: + msg210421
2014-02-06 21:37:23serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg210420
2014-02-06 21:33:16pitrousetstage: test needed -> needs patch
2014-02-06 21:31:54barrysetnosy: + barry
2014-02-06 21:25:50ethan.furmancreate