Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Abstract Base Classes [Support ABC and Enums - Part 2] #580

Merged
merged 1 commit into from
Mar 26, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions dill/_dill.py
Original file line number Diff line number Diff line change
Expand Up @@ -1700,6 +1700,27 @@ def _get_typedict_type(cls, clsdict, attrs, postproc_list):
# clsdict.pop('__prepare__', None)
return clsdict, attrs

def _get_typedict_abc(obj, _dict, attrs, postproc_list):
if hasattr(abc, '_get_dump'):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does abc ever not have _get_dump? I believe this method exists for python 3.7 and above -- and dill supports python 3.7 and above. Haven't checked if there's older versions of 3.7 (etc) that don't have this method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't in PyPy. PyPy's implementation of abc is similar to the CPython 3.6 and older implementation because it is compiled directly into a binary by the RPython compiler and the point of PyPy is to implement Python in Python, so rewriting it in C doesn't make sense for them.

Copy link
Member

@mmckerns mmckerns Mar 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had checked pypy along with python, and found the same. pypy lags changes in python, but it seems that for the last few versions of pypy, it's there.

Python 3.7.13 (7e0ae751533460d5f89f3ac48ce366d8642d1db5, Mar 29 2022, 06:30:54)
[PyPy 7.3.9 with GCC Apple LLVM 13.0.0 (clang-1300.0.29.30)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>>> import abc
>>>> abc._get_dump
<built-in function _get_dump>
>>>> 

So, are you saying we need the code block due to older releases of pypy?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. That is interesting. Yes. There are some older PyPy builds for Python 3.7 that do not have _get_dump. I guess they added a _get_dump function to make the API identical to the CPython version. Maybe we can remove this if statement?

(registry, _, _, _) = abc._get_dump(obj)
register = obj.register
postproc_list.extend((register, (reg(),)) for reg in registry)
elif hasattr(obj, '_abc_registry'):
registry = obj._abc_registry
register = obj.register
postproc_list.extend((register, (reg,)) for reg in registry)
else:
raise PicklingError("Cannot find registry of ABC %s", obj)

if '_abc_registry' in _dict:
del _dict['_abc_registry']
del _dict['_abc_cache']
del _dict['_abc_negative_cache']
# del _dict['_abc_negative_cache_version']
else:
del _dict['_abc_impl']
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tend to use pop instead of del to delete, just because it's protected against the keyword not being there.

Copy link
Contributor Author

@anivegesana anivegesana Mar 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, strangely enough, del is older than pop. All modern Python implementations, including MicroPython, will have both, so this is not a concern.

I chose to use del because I already use the value of _abc_impl earlier in the function, so I know it exists. I guess switching to pop might be better for the other statements though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I decided to leave it as is. This is what I use and it makes sense based on the return values of the different functions/operators:

  • del a[·]: when I know the key exists in the dictionary and don't need the value
  • a.pop(·): when I know the key exists in the dictionary but do need the value
  • a.pop(·, None): when I do not know if the key exists in the dictionary

Copy link
Member

@mmckerns mmckerns Mar 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I said "keyword" and meant "key". I didn't mean that for you to think I was worried that del doesn't exist, I meant it in this sense:
a.pop(·, None): when I do not know if the key exists in the dictionary
The bug encountered with del is almost always that the key doesn't exist, so if the key might not (in some rare case) exist, then pop will protect you against that.

return _dict, attrs

@register(TypeType)
def save_type(pickler, obj, postproc_list=None):
if obj in _typemap:
Expand Down Expand Up @@ -1768,6 +1789,11 @@ def save_type(pickler, obj, postproc_list=None):
for name in slots:
_dict.pop(name, None)

if isinstance(obj, abc.ABCMeta):
logger.trace(pickler, "ABC: %s", obj)
_dict, attrs = _get_typedict_abc(obj, _dict, attrs, postproc_list)
logger.trace(pickler, "# ABC")

qualname = getattr(obj, '__qualname__', None)
if attrs is not None:
for k, v in attrs.items():
Expand Down
163 changes: 163 additions & 0 deletions dill/tests/test_abc.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,163 @@
#!/usr/bin/env python
"""
test dill's ability to pickle abstract base class objects
"""
import dill
import abc
from abc import ABC
import warnings

from types import FunctionType

dill.settings['recurse'] = True

class OneTwoThree(ABC):
@abc.abstractmethod
def foo(self):
"""A method"""
pass

@property
@abc.abstractmethod
def bar(self):
"""Property getter"""
pass

@bar.setter
@abc.abstractmethod
def bar(self, value):
"""Property setter"""
pass

@classmethod
@abc.abstractmethod
def cfoo(cls):
"""Class method"""
pass

@staticmethod
@abc.abstractmethod
def sfoo():
"""Static method"""
pass

class EasyAsAbc(OneTwoThree):
def __init__(self):
self._bar = None

def foo(self):
return "Instance Method FOO"

@property
def bar(self):
return self._bar

@bar.setter
def bar(self, value):
self._bar = value

@classmethod
def cfoo(cls):
return "Class Method CFOO"

@staticmethod
def sfoo():
return "Static Method SFOO"

def test_abc_non_local():
assert dill.copy(OneTwoThree) is not OneTwoThree
assert dill.copy(EasyAsAbc) is not EasyAsAbc

with warnings.catch_warnings():
warnings.simplefilter("ignore", dill.PicklingWarning)
assert dill.copy(OneTwoThree, byref=True) is OneTwoThree
assert dill.copy(EasyAsAbc, byref=True) is EasyAsAbc

instance = EasyAsAbc()
# Set a property that StockPickle can't preserve
instance.bar = lambda x: x**2
depickled = dill.copy(instance)
assert type(depickled) is not type(instance)
assert type(depickled.bar) is FunctionType
assert depickled.bar(3) == 9
assert depickled.sfoo() == "Static Method SFOO"
assert depickled.cfoo() == "Class Method CFOO"
assert depickled.foo() == "Instance Method FOO"

def test_abc_local():
"""
Test using locally scoped ABC class
"""
class LocalABC(ABC):
@abc.abstractmethod
def foo(self):
pass

def baz(self):
return repr(self)

labc = dill.copy(LocalABC)
assert labc is not LocalABC
assert type(labc) is type(LocalABC)
# TODO should work like it does for non local classes
# <class '__main__.LocalABC'>
# <class '__main__.test_abc_local.<locals>.LocalABC'>

class Real(labc):
def foo(self):
return "True!"

def baz(self):
return "My " + super(Real, self).baz()

real = Real()
assert real.foo() == "True!"

try:
labc()
except TypeError as e:
# Expected error
pass
else:
print('Failed to raise type error')
assert False

labc2, pik = dill.copy((labc, Real()))
assert 'Real' == type(pik).__name__
assert '.Real' in type(pik).__qualname__
assert type(pik) is not Real
assert labc2 is not LocalABC
assert labc2 is not labc
assert isinstance(pik, labc2)
assert not isinstance(pik, labc)
assert not isinstance(pik, LocalABC)
assert pik.baz() == "My " + repr(pik)

def test_meta_local_no_cache():
"""
Test calling metaclass and cache registration
"""
LocalMetaABC = abc.ABCMeta('LocalMetaABC', (), {})

class ClassyClass:
pass

class KlassyClass:
pass

LocalMetaABC.register(ClassyClass)

assert not issubclass(KlassyClass, LocalMetaABC)
assert issubclass(ClassyClass, LocalMetaABC)

res = dill.dumps((LocalMetaABC, ClassyClass, KlassyClass))

lmabc, cc, kc = dill.loads(res)
assert type(lmabc) == type(LocalMetaABC)
assert not issubclass(kc, lmabc)
assert issubclass(cc, lmabc)

if __name__ == '__main__':
test_abc_non_local()
test_abc_local()
test_meta_local_no_cache()