FIX pickling for ABC in python3.7 #189
Conversation
Codecov Report
@@ Coverage Diff @@
## master #189 +/- ##
==========================================
+ Coverage 84.09% 84.38% +0.28%
==========================================
Files 1 1
Lines 547 557 +10
Branches 99 104 +5
==========================================
+ Hits 460 470 +10
Misses 63 63
Partials 24 24
Continue to review full report at Codecov.
|
tomMoral
force-pushed
the
FIX_abc_reduce
branch
from
5acea3a
to
654ea89
Compare
tomMoral
force-pushed
the
FIX_abc_reduce
branch
2 times, most recently
from
d485714
to
8fdbe68
Compare
tomMoral
force-pushed
the
FIX_abc_reduce
branch
2 times, most recently
from
fd506e6
to
c917956
Compare
|
The trick to get an up-to-date python3.7 release was given here! |
tests/cloudpickle_test.py
Outdated
| @@ -655,11 +655,16 @@ class ConcreteClass(AbstractClass): | |||
| def foo(self): | |||
| return 'it works!' | |||
|
|
|||
| AbstractClass.register(tuple) | |||
|
|
|||
| depickled_base = pickle_depickle(AbstractClass, protocol=self.protocol) | |||
There was a problem hiding this comment.
This introduces a side effect in the test. I don't think it's possible to unregister tuple. Cloud you please instead use a new locally defined class, e.g.:
class SomeOtherClass(object):
pass
AbstractClass.register(SomeOtherClass)
...There was a problem hiding this comment.
Actually, it should also be possible to do:
AbstractClass.register(tuple)
try:
...
finally:
AbstractClass._abc_registry_clear()do as you wish.
There was a problem hiding this comment.
We would also need to clean the registry of depickled_base. Better not register tuple at all.
cloudpickle/cloudpickle.py
Outdated
| @@ -460,6 +460,14 @@ def save_dynamic_class(self, obj): | |||
| clsdict = dict(obj.__dict__) # copy dict proxy to a dict | |||
| clsdict.pop('__weakref__', None) | |||
|
|
|||
| # For ABCMeta in python3.7, remove _abc_impl as it is not picklable. | |||
cloudpickle/cloudpickle.py
Outdated
| if "_abc_impl" in clsdict: | ||
| import abc | ||
| (registry, _, _, _) = abc._get_dump(obj) | ||
| clsdict["_abc_impl"] = [wr() for wr in registry] |
|
Thanks! Merged. However, I forgot to ask about adding an entry to the changelog. Can you please open a new PR for that? |
Fix #180
The issue was that the
abcmodule have been entirely re-written in C. (python/cpython#5273)The
registermechanism is not accessible anymore. The proposed implementation simply pickle the registered classes usingabc._get_dumpand then re-register them. This does not conserve thecachebut this does not seem to be an issue (it is only a matter of performance for the first call toissubclass).