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

Drop annotation of dynamic classes when pickling for Python < 3.7 #348

Merged
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
4 changes: 4 additions & 0 deletions CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,10 @@

**This version requires Python 3.5 or later**

- Stop pickling the annotations of a dynamic class for Python < 3.6
(follow up on #276)
([issue #347](https://github.com/cloudpipe/cloudpickle/issues/347))

1.3.0
=====

Expand Down
5 changes: 5 additions & 0 deletions cloudpickle/cloudpickle.py
Original file line number Diff line number Diff line change
Expand Up @@ -581,6 +581,11 @@ def save_dynamic_class(self, obj):
if isinstance(__dict__, property):
type_kwargs['__dict__'] = __dict__

if sys.version_info < (3, 7):
Copy link
Contributor

Choose a reason for hiding this comment

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

The new tests introduced in this PR cover the case where sys.version_info >= (3, 7). We should introduce a new test precisely for Python 3.6 no?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, the new tests cover precisely the case where Python < 3.7 :)

# Although annotations were added in Python 3.4, It is not possible
# to properly pickle them until Python 3.7. (See #193)
clsdict.pop('__annotations__', None)

save = self.save
write = self.write

Expand Down
80 changes: 73 additions & 7 deletions tests/cloudpickle_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@
import weakref
import os
import enum
import typing
from functools import wraps

import pytest

Expand Down Expand Up @@ -1784,11 +1786,9 @@ def g():
self.assertEqual(f2.__doc__, f.__doc__)

@unittest.skipIf(sys.version_info < (3, 7),
"This syntax won't work on py2 and pickling annotations "
"isn't supported for py37 and below.")
"Pickling type annotations isn't supported for py36 and "
"below.")
def test_wraps_preserves_function_annotations(self):
from functools import wraps

def f(x):
pass

Expand All @@ -1802,13 +1802,79 @@ def g(x):

self.assertEqual(f2.__annotations__, f.__annotations__)

@unittest.skipIf(sys.version_info >= (3, 7),
"pickling annotations is supported starting Python 3.7")
def test_function_annotations_silent_dropping(self):
# Because of limitations of typing module, cloudpickle does not pickle
# the type annotations of a dynamic function or class for Python < 3.7

class UnpicklableAnnotation:
# Mock Annotation metaclass that errors out loudly if we try to
# pickle one of its instances
def __reduce__(self):
raise Exception("not picklable")

unpickleable_annotation = UnpicklableAnnotation()

def f(a: unpickleable_annotation):
return a

with pytest.raises(Exception):
cloudpickle.dumps(f.__annotations__)

depickled_f = pickle_depickle(f, protocol=self.protocol)
assert depickled_f.__annotations__ == {}

@unittest.skipIf(sys.version_info >= (3, 7) or sys.version_info < (3, 6),
Copy link
Contributor

Choose a reason for hiding this comment

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

This condition is really hard to parse. It should be equivalent to:

@unittest.skipIf(sys.version_info[:2] != (3, 6))

but then contratdicts the message "pickling annotations is supported starting Python 3.7" because the test is not skipped and passes on Python 3.5.

Copy link
Member Author

Choose a reason for hiding this comment

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

The tests actually test that annotations are properly dropped on Python 3.6.

"pickling annotations is supported starting Python 3.7")
def test_class_annotations_silent_dropping(self):
# Because of limitations of typing module, cloudpickle does not pickle
# the type annotations of a dynamic function or class for Python < 3.7

# Pickling and unpickling must be done in different processes when
# testing dynamic classes (see #313)

code = '''if 1:
import cloudpickle
import sys

class UnpicklableAnnotation:
# Mock Annotation metaclass that errors out loudly if we try to
# pickle one of its instances
def __reduce__(self):
raise Exception("not picklable")

unpickleable_annotation = UnpicklableAnnotation()

class A:
a: unpickleable_annotation

try:
cloudpickle.dumps(A.__annotations__)
except Exception:
pass
else:
raise AssertionError

sys.stdout.buffer.write(cloudpickle.dumps(A, protocol={protocol}))
'''
cmd = [sys.executable, '-c', code.format(protocol=self.protocol)]
proc = subprocess.Popen(
cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE
cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE

)
proc.wait()
out, err = proc.communicate()
assert proc.returncode == 0, err

depickled_a = pickle.loads(out)
assert not hasattr(depickled_a, "__annotations__")

@unittest.skipIf(sys.version_info < (3, 7),
"""This syntax won't work on py2 and pickling annotations
isn't supported for py37 and below.""")
"Pickling type hints isn't supported for py36"
" and below.")
def test_type_hint(self):
# Try to pickle compound typing constructs. This would typically fail
# on Python < 3.7 (See #193)
import typing
t = typing.Union[list, int]
assert pickle_depickle(t) == t

Expand Down