Pickling of methodcaller, attrgetter, and itemgetter

[anntzer]

BPO	22955
Nosy	@rhettinger, @pitrou, @zware, @serhiy-storchaka, @anntzer, @MojoVampire, @thatneat
Files	pickle_getter_and_caller.patch pickle_getter_and_caller2.patch issue22955.diff: josh.r's patch with itemgetter and attrgetter reimplementations pickle_getter_and_caller3.patch pickle_getter_and_caller4.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/serhiy-storchaka'
closed_at = <Date 2015-05-27.08:49:19.200>
created_at = <Date 2014-11-27.06:33:03.813>
labels = ['type-feature', 'library']
title = 'Pickling of methodcaller, attrgetter, and itemgetter'
updated_at = <Date 2016-05-16.19:28:48.781>
user = 'https://github.com/anntzer'

bugs.python.org fields:

activity = <Date 2016-05-16.19:28:48.781>
actor = 'serhiy.storchaka'
assignee = 'serhiy.storchaka'
closed = True
closed_date = <Date 2015-05-27.08:49:19.200>
closer = 'serhiy.storchaka'
components = ['Library (Lib)']
creation = <Date 2014-11-27.06:33:03.813>
creator = 'Antony.Lee'
dependencies = []
files = ['37314', '37315', '37320', '37448', '39395']
hgrepos = []
issue_num = 22955
keywords = ['patch']
message_count = 23.0
messages = ['231752', '231768', '231831', '231841', '231848', '231849', '231850', '231851', '231872', '231873', '232363', '232364', '232370', '232512', '232616', '232617', '232641', '243364', '243676', '243687', '243746', '265718', '265727']
nosy_count = 8.0
nosy_names = ['rhettinger', 'pitrou', 'python-dev', 'zach.ware', 'serhiy.storchaka', 'Antony.Lee', 'josh.r', 'jason.curtis']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue22955'
versions = ['Python 3.5']

[anntzer]

methodcaller and attrgetter objects seem to be picklable, but in fact the pickling is erroneous:

>>> import operator, pickle
>>> pickle.loads(pickle.dumps(operator.methodcaller("foo")))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: methodcaller needs at least one argument, the method name
>>> pickle.loads(pickle.dumps(operator.attrgetter("foo")))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: attrgetter expected 1 arguments, got 0

When looking at the pickle disassembly, it seems that the argument to the constructor is indeed not pickled.

>>> import pickletools; pickletools.dis(pickle.dumps(operator.methodcaller("foo")))
    0: \x80 PROTO      3
    2: c    GLOBAL     'operator methodcaller'
   25: q    BINPUT     0
   27: )    EMPTY_TUPLE
   28: \x81 NEWOBJ
   29: q    BINPUT     1
   31: .    STOP
highest protocol among opcodes = 2

[serhiy-storchaka]

I think this issue needs different solutions for 3.5 and maintained releases. We can implement the pickling of methodcaller, attrgetter and itemgetter in 3.5 (I agree this is good idea). And it would be good if pickling of these types will raise an exception in maintained releases.

[zware]

Note that pickling of the pure Python version of methodcaller works as expected:

Python 3.4.2 (default, Nov 20 2014, 12:40:10) 
[GCC 4.8.3] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import sys
>>> sys.modules['_operator'] = None
>>> import operator
>>> import pickle
>>> pickle.loads(pickle.dumps(operator.methodcaller('foo')))
<operator.methodcaller object at 0x7ff869945898>

The pure Python attrgetter and itemgetter don't work due to using functions defined in __init__().

2.7 already raises TypeError on attempts to pickle any of the three.

[rhettinger]

+1 for adding pickling support to Python 3.5.

I don't see much of a need for any revision to 3.4.

[MojoVampire]

I've made a patch that I believe should cover all three cases, including tests.

In addition to the pickling behavior, I've made two other changes:

1. methodcaller verifies during construction that the name is a string (PyUnicode), and interns it; attrgetter did this already, and I tweaked methodcaller to match for correctness and performance reasons
2. I added proper repr functionality to all three objects. Partially this is just to make it look nicer, but it was also a decent way to spot verify that the pickle/unpickle sequence behaved correctly

Anyone care to review?

[MojoVampire]

Don't bother reviewing just yet. There is an issue with attrgetter's pickling (which the unit tests caught), and I need to update the pure Python modules to match.

[MojoVampire]

Okay, this one passes the tests for the built-in module. I'm not sure what's going wrong with the pure Python module. I'm getting the error:

_pickle.PicklingError: Can't pickle <class 'operator.attrgetter'>: it's not the same object as operator.attrgetter

once for each of the three objects. Anyone recognize this? Is this some weird artifact of the multiple imports required to test both pure Python and C versions of the module that I need to work around, or did I make a mistake somewhere else?

[MojoVampire]

Ah, solved it (I think). The bootstrapper used to import the Python and C versions of the module leaves sys.modules unpopulated (Does pickle itself may populate it when it finds no module of that name?). I added a setUp method to the unittest class for operator that explicitly sets sys.modules['operator'] to whichever version is being tested at the time so pickle's lookup works as expected. Is that the right solution? New patch uploaded with that change.

[zware]

I'd prefer to just reimplement itemgetter and attrgetter to make them picklable rather than adding pickling methods to them; see attached patch.

I also posted a few comments, but I just went ahead and addressed them myself in this patch. I'm not qualified to give the _operator.c changes a proper review, but they look good enough to me if others agree that __reduce__ is the best approach in C.

[serhiy-storchaka]

operator.methodcaller is similar to functools.partial which is pickleable and can be used as a sample.

In C implementation some code can be shared between __repr__ and __reduce__ methods.

As for tests, different protocols should be tested. Also should be tested compatibility between C and Python implementations, instances pickled with one implementation should be unpickleable with other implementation. Move pickle tests into new test class.

If add __repr__ methods, they need tests. The restriction of method name type should be tested too.

[rhettinger]

I'd prefer to just reimplement itemgetter and attrgetter to make
them picklable rather than adding pickling methods to them;
see attached patch.

That isn't the usual approach. The pickling methods are there for a reason. I prefer to leave the existing code in a stable state and avoid unnecessary code churn or risk introducing bugs into code that is working correctly and as designed.