Fix import of pickle module on PYPY 3.8 (#455)

[schettino72]

Based on feedback given for #454.
Also update GH actions to use PYPY 3.8

[ogrisel]

Assuming CI is green, this LGTM. Please consider the suggestion below and document the change in the changelog for the next release.

[ogrisel]

would it be possible to explicitly set one of those 2 pypy config to use pypy-3.7?

[ogrisel]

actually I was confused by the way the matrix is defined negatively. We need to add "pypy-3.7" to the list of Python version and then exclude the all the macos / windows + pypy combos that still fail.

[codecov]

Codecov Report

Merging #461 (9ceafa1) into master (f5472e1) will decrease coverage by 7.94%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #461      +/-   ##
==========================================
- Coverage   92.60%   84.65%   -7.95%     
==========================================
  Files           4        4              
  Lines         717      717              
  Branches      158      158              
==========================================
- Hits          664      607      -57     
- Misses         32       86      +54     
- Partials       21       24       +3     

Impacted Files	Coverage Δ
cloudpickle/cloudpickle.py	88.00% <100.00%> (-0.60%)	⬇️
cloudpickle/cloudpickle_fast.py	80.98% <100.00%> (-15.96%)	⬇️
cloudpickle/compat.py	75.00% <100.00%> (-25.00%)	⬇️

---

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 f5472e1...9ceafa1. Read the comment docs.

[schettino72]

Ok, so it is actually broken on PYPY3.8. It is not only a matter of changing imports...

I started to track down the problem, PYPY pickle implementation expects the "reduce" implementation to be a method (bound or unbound) [1].
But cloudpickle pass a plain function.

Now, I am not sure about the specification to decide who should fix something.

Also note that on default branch, their "pickle.py" implementation file is not there any more.
Maybe it works on their latest, did not have enough time to test it.

[1] https://foss.heptapod.net/pypy/pypy/-/blob/branch/py3.8/lib-python/3/pickle.py#L573

[Yikun]

Any progress on this? I just hitted this error in pypy3.8.

[mattip]

I am not sure about the specification

The "specification" is basically the implementation. If there is something that works in the CPython pure python pickle.py and does not work on PyPy, it is a PyPy bug. If there is something that works on CPython when using the _pickle c-extension and does not work when using the pure-python pickle.py, it should be discussed on the CPython issue tracker.

[mattip]

on default branch, their "pickle.py" implementation file is not there any more

? PyPy's default branch is for python2.7, which is not what you want to be looking at. For the python3.8 implementation, you want the py3.8 branch, and for the py3.9 implementation you want the py3.9 branch. In both cases, pypy copies the CPython stdlib pickle.py

[mattip]

The CI logs are gone so I cannot see what test fails.

[ogrisel]

@mattip @schettino72 I attempted to resolve the conflicts with the master branch. This should trigger a new CI run to collect some up-to-date logs.

[mattip]

Wow, that's a lot of failures. This seems to be the first problem:

            if f is not None:
>               f(self, obj)  # Call unbound method with explicit self
E               TypeError: _file_reduce() takes 1 positional argument but 2 were given
/opt/hostedtoolcache/PyPy/3.8.13/x64/lib/pypy3.8/pickle.py:573: TypeError

In CPython with _pickle this passes, but if I remove the _pickle c-extension module, the pure python code fails like PyPy does, so indeed it seems that calling the pure-python _Pickler.save(cloudpickle_fast.CloudPickler, io.TextIOWrapper, save_persistent_id = True) somehow differs from the cpython c-extension save

It would be nice if this could be "reduced" (pun not intended) to a minimal reproducer so it can be filed as a CPython bug, since the c-extension _pickle module is not meant to change semantics. I don't see a related open issue when searching for _pickle and pickle, but maybe my issue foo is not up to the task.

[lesteve]

Wow, that's a lot of failures. This seems to be the first problem:

            if f is not None:
>               f(self, obj)  # Call unbound method with explicit self
E               TypeError: _file_reduce() takes 1 positional argument but 2 were given
/opt/hostedtoolcache/PyPy/3.8.13/x64/lib/pypy3.8/pickle.py:573: TypeError

I debugged this one and figured the reason for the failure is this line:

cloudpickle/cloudpickle/cloudpickle_fast.py

Line 654 in f5472e1

dispatch = dispatch_table

It was added in #370. I am not sure I understand exactly what is is supposed to be doing (maybe previous implementations of pickle5 were using dispatch rather than dispatch_table not sure ...)

For the Python-based pickle dispatch and dispatch_table are two different dictionaries . The values of dispatch have a signature f(self, obj) whereas the one of dispatch_table have a signature of f(obj) hence the weird TypeError

Code for Python 3.9:
https://github.com/python/cpython/blob/83886261fa584b45d5e70c5a422f69cad7422a0a/Lib/pickle.py#L555-L567

Commenting out the dispatch = dispatch_table line, I get 3 test failures locally, which seems a lot more manageable already.

[lesteve]

It was added in #370. I am not sure I understand exactly what is is supposed to be doing (maybe previous implementations of pickle5 were using dispatch rather than dispatch_table not sure ...)

Actually it was added in cloudpickle 1.5 for backward-compatibility with cloudpickle 1.4.1 where CloudPickler had a dispatch attribute`:

cloudpickle/cloudpickle/cloudpickle_fast.py

Lines 396 to 411 in b8ed4d0

	class CloudPickler(Pickler):
	"""Fast C Pickler extension with additional reducing routines.
	CloudPickler's extensions exist into into:
	* its dispatch_table containing reducers that are called only if ALL
	built-in saving functions were previously discarded.
	* a special callback named "reducer_override", invoked before standard
	function/class builtin-saving method (save_global), to serialize dynamic
	functions
	"""
	# cloudpickle's own dispatch_table, containing the additional set of
	# objects (compared to the standard library pickle) that cloupickle can
	# serialize.
	dispatch = {}

It has been a while now (1.4.1 is April 2020, 1.5 is July 2020), maybe dispatch = dispatch_table can be removed completely?