Refactor builtin method pickling

[pierreglaser]

The way cloudpickle handles builtin_function_or_method is very confusing for many reasons:

• it does not explicitly separate the cases between builtin_function and builtin_methods.
• it interleaves builtin_function_or_method handling with more classic function handling
• it special-cases things that do not need to be special-cases, such as builtin types constructors.

In realilty, things are not that complex:

• builtin_function_or_method are correctly handled by the standard pickle in python3, so we do not need to special case them AT ALL in python3
• in python2, builtin functions are picklable. We only need to special-case builtin methods.

This PR addresses all the previously stated issues: it separates clearly builtin_function_or_method from classic functions, add a special case only for builtin_method in python2, and as a consequence, removes a lot of obscure special-cases.

[codecov]

Codecov Report

Merging #262 into master will increase coverage by 0.44%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #262      +/-   ##
==========================================
+ Coverage    88.9%   89.35%   +0.44%     
==========================================
  Files           1        1              
  Lines         631      620      -11     
  Branches      131      131              
==========================================
- Hits          561      554       -7     
+ Misses         46       43       -3     
+ Partials       24       23       -1

Impacted Files	Coverage Δ
cloudpickle/cloudpickle.py	89.35% <100%> (+0.44%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 74d69d7...43e358b. Read the comment docs.

[pierreglaser]

This change solves a related bug, but it deserves a different PR.

[ogrisel]

Can you be more explicit? Maybe you could include a non-regression test directly in this PR and document it the changelog?

[pierreglaser]

precisely, python3 does not make a difference between unbound methods and functions.

Consider pickle._Pickler.dump: this is a non-builtin unbound method.
The bug is was mentioning is that cloudpickle master currently fails at detecting this as global, and instead pickles this dynamically:

import cloudpickle
import pickle

cloudpickle.dumps(pickle._Pickler.dump)

gives:

Out[4]: b'\x80\x04\x95c\x03\x00\x00\x00\x00\x00\x00\x8c\x17cloudpickle.cloudpickle\x94\x8c\x0e_fill_function\x94\x93\x94(h\x00\x8c\x0f_                                                                                                                                        
make_skel_func\x94\x93\x94h\x00\x8c\r_builtin_type\x94\x93\x94\x8c\x08CodeType\x94\x85\x94R\x94(K\x02K\x00K\x02K\x06KCCrt\x00|\x00d\x01                                                                                                                                        
\x83\x02s\x1ct\x01d\x02|\x00j\x02j\x03f\x01\x16\x00\x83\x01\x82\x01|\x00j\x04d\x03k\x05r<|\x00\xa0\x05t\x06t\x07d\x04|\x00j\x04\x83\x02                                                                                                                                        
\x17\x00\xa1\x01\x01\x00|\x00j\x04d\x05k\x05rP|\x00j\x08\xa0\t\xa1\x00\x01\x00|\x00\xa0\n|\x01\xa1\x01\x01\x00|\x00\xa0\x05t\x0b\xa1\x0                                                                                                                                        
1\x01\x00|\x00j\x08\xa0\x0c\xa1\x00\x01\x00d\x06S\x00\x94(\x8c7Write a pickled representation of obj to the open file.\x94\x8c\x0b_file                                                                                                                                        
_write\x94\x8c2Pickler.__init__() was not called by %s.__init__()\x94K\x02\x8c\x02<B\x94K\x04Nt\x94(\x8c\x07hasattr\x94\x8c\rPicklingEr                                                                                                                                        
ror\x94\x8c\t__class__\x94\x8c\x08__name__\x94\x8c\x05proto\x94\x8c\x05write\x94\x8c\x05PROTO\x94\x8c\x04pack\x94\x8c\x06framer\x94\x8c                                                                                                                                        
\rstart_framing\x94\x8c\x04save\x94\x8c\x04STOP\x94\x8c\x0bend_framing\x94t\x94\x8c\x04self\x94\x8c\x03obj\x94\x86\x94\x8c\x1c/usr/lib/                                                                                                                                        
python3.7/pickle.py\x94\x8c\x04dump\x94M\xaa\x01C\x14\x00\x04\n\x01\x04\x01\x0e\x01\n\x01\x16\x01\n\x01\n\x01\n\x01\n\x01\x94))t\x94R\x                                                                                                                                        
94J\xff\xff\xff\xff}\x94(\x8c\x0b__package__\x94\x8c\x00\x94h\x13\x8c\x06pickle\x94\x8c\x08__file__\x94\x8c\x1c/usr/lib/python3.7/pickl                                                                                                                                        
e.py\x94u\x87\x94R\x94}\x94(\x8c\x07globals\x94}\x94(h\x17\x8c\x07_struct\x94\x8c\x04pack\x94\x93\x94h\x1bC\x01.\x94h\x16C\x01\x80\x94h                                                                                                                                        
\x11\x8c\x07_pickle\x94\x8c\rPicklingError\x94\x93\x94u\x8c\x08defaults\x94N\x8c\x04dict\x94}\x94\x8c\x0eclosure_values\x94N\x8c\x06mod                                                                                                                                        
ule\x94h)\x8c\x04name\x94h"\x8c\x03doc\x94h\x0b\x8c\x0bannotations\x94}\x94\x8c\x08qualname\x94\x8c\r_Pickler.dump\x94utR.'

The reason being that pickle._Pickler.dump.__name__ is simply dump, whereas to retrieve this function, we need to get the qualified name of this function, namely _Pickler.dump

If we use __qualname__ instead of __name__, we cannot use getattr though, because getattr does not accept dotted path. Instead, we need to use pickle._getattribute.

[ogrisel]

If we use qualname instead of name, we cannot use getattr though, because getattr does not accept dotted path. Instead, we need to use pickle._getattribute.

But this (using getattr) is what you are doing here, no?

Sorry, I misread what you said. That sounds good.

[ogrisel]

So to summarize, this can only be (easily) fixed in Python 3?

I think we still need a specific test for this fix.

[pierreglaser]

in python2, unbound methods are methods, and get serialized using save_instancemethod:

In [57]: isinstance(pickle.Pickler.dump, types.MethodType)
Out[57]: True

[pierreglaser]

ditto

[ogrisel]

This is indeed much cleaner. Here are some minor comments:

[pierreglaser]

OK so the failing test happens only on pypy35 (not later versions, such as pypy36 for example), because for some reason, in pypy3.5, tuple.__new__.__qualname__ = "__new__", and not "tuple.__new__" as it should be. This is evil.

Technically though, cloudpickle manages to re-create tuple.__new__, it simply pickles it as if it was dynamic, as it cannot retrieve the function as an attribute of a module. So it is "half-a-bug" 😄

[ogrisel]

I just changed the test to make it weaker. So it's actually a bug in PyPy35. Maybe we can re-add this assertion and skip only this line on PyPy 3.5.

[ogrisel]

My weaker variant of the test is also failing on PyPy:

    def test_builtin_method(self):
        # Note that builtin_function_or_method are special-cased by cloudpickle
        # only in python2.
    
        # pickle_depickle some builtin methods of the __builtin__ module
        for t in list, tuple, set, frozenset, dict, object:
            cloned_new = pickle_depickle(t.__new__, protocol=self.protocol)
>           assert isinstance(cloned_new(t), t)
E           TypeError: __new__() missing 1 required positional argument: 'sequence'

So +1 for using the old assertion but skipping it on PyPy3.5

[pierreglaser]

Maybe we can re-add this assertion and skip only this line on PyPy 3.5.

Yeah that was what I wanted to suggest.

Let's wait a bit before merging this one though. I'll make one more pass through the method vs member_descriptor vs method-wrapper vs wrapper-descriptor vs unbound_method vs bound_method vs function vs builtin_function_or_method terminology in the different pythons, maybe add a few more examples in the tests.

[pierreglaser]

TypeError: new() missing 1 required positional argument: 'sequence'

This is yet another bug though: visibly we forget the default value of the sequence kwarg when pickling tuple.__new__:

In [28]: tuple.__new__                                                                                                                               
Out[28]: <function __new__(tupletype, sequence=<no value>)>

In [29]: cloudpickle.loads(cloudpickle.dumps(tuple.__new__))                                                                                         
Out[29]: <function __new__(tupletype, sequence)>

[ogrisel]

Some good news at least: the downstream tests pass:

https://travis-ci.org/cloudpipe/cloudpickle/builds/522056482?utm_source=github_status&utm_medium=notification

[ogrisel]

This is yet another bug though: visibly we forget the default value of the sequence kwarg when pickling tuple.new

Do you think it's specific to dynamic builtin functions / methods or it's a bug of dynamic functions in general?

[pierreglaser]

No it seems to be related only to some builtin functions/methods:

in pypy3.5:

In [30]: def f(a=1): 
    ...:    pass 

In [31]: cloudpickle.loads(cloudpickle.dumps(f))                                                                                                     
Out[31]: <function __main__.f(a=1)>

[ogrisel]

Ok. BTW something surprising but we cannot probably fix easily (on CPython):

>>> import cloudpickle                                                                                                                                                          
>>> default_value = object()                                                                                                                                                    
>>> def f(a=default_value): 
...     return id(a) 
...                                                                                                                                                                             
>>> cloudpickle.loads(cloudpickle.dumps(f))()                                                                                                                                   
140550658935136
>>> f()                                                                                                                                                                         
140550658935104

[pierreglaser]

Yes: defaults value get shipped into the function code. In your case, the default value is an instance, so it gets re-created at unpickling time. Sadly, we cannot pickle any global variable as global (per attribute) because most of them do not have a __module__ attribute, we would have to run the costly whichmodule loop to find them.

[ogrisel]

I agree. We could do something similar for provenance tracking to what I did for the type definitions themselves in #246 but I am afraid of the overhead and the unanticipated side effects.

[ogrisel]

Anyway this is unrelated to this PR so leave this aside for now.

[pierreglaser]

I made a new pass on this PR. I found PEP 579, which turned out to be a very useful resource to cover all flavours in which builtin functions and methods can appear.

Building upon this, I first wrote new comprehensive tests for all possible cases (b6953f4) that showed that some exotic methods flavours could not be pickled under python2: classmethod_descriptor, wrapper_descriptor and method-wrapper objects. With the latest cloudpickle, all three examples here fail:

cloudpickle.dumps(1.5.__repr__)
cloudpickle.dumps(float.__repr__)
cloudpickle.dumps(float.__dict__['fromhex'])

Starting from these tests, I adjusted the implementation of cloudpickle's builtin method saving logic. All builtin functions and methods are now redirected into one single, simple function.

Also, given this consideration, I think we can safely remove the save_builtin_method test, that predated these new changes, and that adds some unnecessary confounding.

[pierreglaser]

Downstream tests pass with the new version as well.

[ogrisel]

Here are a few more comments. But otherwise this PR looks ready for merge.

Please add an entry to the change log to target version 1.2.0.

[pierreglaser]

Just rebased. Also, I mentioned the PR number and not the issue number in CHANGES.md as there is no issue in this case.

[ogrisel]

Merged, thank you very much @pierreglaser!