-
Notifications
You must be signed in to change notification settings - Fork 167
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
Changes from 3 commits
dafaae3
2416bb8
df5a5eb
e5e4c81
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -1802,6 +1802,73 @@ 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), | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
) | ||||||
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.""") | ||||||
|
There was a problem hiding this comment.
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?There was a problem hiding this comment.
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
:)