Skip to content
3 changes: 3 additions & 0 deletions CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,9 @@
and expand the support for pickling `TypeVar` instances (dynamic or non-dynamic)
to Python 3.5-3.6 ([PR #350](https://github.com/cloudpipe/cloudpickle/pull/350))

- Add support for pickling dynamic generics on 3.7+
Comment thread
ogrisel marked this conversation as resolved.
Outdated
([PR #351](https://github.com/cloudpipe/cloudpickle/pull/351))

1.3.0
=====

Expand Down
19 changes: 16 additions & 3 deletions cloudpickle/cloudpickle.py
Original file line number Diff line number Diff line change
Expand Up @@ -446,7 +446,7 @@ def dump(self, obj):
raise

def save_typevar(self, obj):
self.save_reduce(*_typevar_reduce(obj))
self.save_reduce(*_typevar_reduce(obj), obj=obj)

dispatch[typing.TypeVar] = save_typevar

Expand Down Expand Up @@ -645,7 +645,7 @@ def save_dynamic_class(self, obj):
# "Regular" class definition:
tp = type(obj)
self.save_reduce(_make_skeleton_class,
(tp, obj.__name__, obj.__bases__, type_kwargs,
(tp, obj.__name__, _get_bases(obj), type_kwargs,
_ensure_tracking(obj), None),
obj=obj)

Expand Down Expand Up @@ -1163,7 +1163,10 @@ class id will also reuse this class definition.
The "extra" variable is meant to be a dict (or None) that can be used for
forward compatibility shall the need arise.
"""
skeleton_class = type_constructor(name, bases, type_kwargs)
skeleton_class = types.new_class(
name, bases, {'metaclass': type_constructor},
lambda ns: ns.update(type_kwargs)
)
return _lookup_class_or_track(class_tracker_id, skeleton_class)


Expand Down Expand Up @@ -1268,3 +1271,13 @@ def _typevar_reduce(obj):
if module_and_name is None:
return (_make_typevar, _decompose_typevar(obj))
return (getattr, module_and_name)


def _get_bases(typ):
if hasattr(typ, '__orig_bases__'):
# For generic types (see PEP 560)
bases_attr = '__orig_bases__'
Comment thread
valtron marked this conversation as resolved.
Comment thread
valtron marked this conversation as resolved.
else:
# For regular class objects
bases_attr = '__bases__'
Comment thread
valtron marked this conversation as resolved.
return getattr(typ, bases_attr)
4 changes: 2 additions & 2 deletions cloudpickle/cloudpickle_fast.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
_is_dynamic, _extract_code_globals, _BUILTIN_TYPE_NAMES, DEFAULT_PROTOCOL,
_find_imported_submodules, _get_cell_contents, _is_importable_by_name, _builtin_type,
Enum, _ensure_tracking, _make_skeleton_class, _make_skeleton_enum,
_extract_class_dict, dynamic_subimport, subimport, _typevar_reduce,
_extract_class_dict, dynamic_subimport, subimport, _typevar_reduce, _get_bases,
)

load, loads = _pickle.load, _pickle.loads
Expand Down Expand Up @@ -76,7 +76,7 @@ def _class_getnewargs(obj):
if isinstance(__dict__, property):
type_kwargs['__dict__'] = __dict__

return (type(obj), obj.__name__, obj.__bases__, type_kwargs,
return (type(obj), obj.__name__, _get_bases(obj), type_kwargs,
_ensure_tracking(obj), None)


Expand Down
49 changes: 49 additions & 0 deletions tests/cloudpickle_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@

from .testutils import subprocess_pickle_echo
from .testutils import assert_run_python_script
from .testutils import subprocess_worker


_TEST_GLOBAL_VARIABLE = "default_value"
Expand Down Expand Up @@ -2121,6 +2122,12 @@ def test_pickle_dynamic_typevar(self):
for attr in attr_list:
assert getattr(T, attr) == getattr(depickled_T, attr)

def test_pickle_dynamic_typevar_memoization(self):
T = typing.TypeVar('T')
depickled_T1, depickled_T2 = pickle_depickle((T, T),
protocol=self.protocol)
assert depickled_T1 is depickled_T2

def test_pickle_importable_typevar(self):
from .mypkg import T
T1 = pickle_depickle(T, protocol=self.protocol)
Expand All @@ -2130,6 +2137,38 @@ def test_pickle_importable_typevar(self):
from typing import AnyStr
assert AnyStr is pickle_depickle(AnyStr, protocol=self.protocol)

@unittest.skipIf(sys.version_info < (3, 7),
"Pickling generics not supported below py37")
def test_generic_type(self):
for obj in _generic_objects_to_test():
pickle_depickle(obj, protocol=self.protocol)

@unittest.skipIf(sys.version_info < (3, 7),
"Pickling type hints not supported below py37")
def test_locally_defined_class_with_type_hints(self):

@pierreglaser pierreglaser Mar 15, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The name used in this test is a little bit confusing: are we testing classes inheriting from Generic constructs, or the pickling of Generic type hints? It seems like we are testing both, and I am not sure why we should test both things in the same tests. See my comments below for additional remarks.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I my initial commit, this test was testing for all kinds of type annotations, both simple literal types and more complex custom types based on generics and their subclasses.

with subprocess_worker(protocol=self.protocol) as worker:
for type_ in _generic_objects_to_test():

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would be in favor of using this test to check not only generic types, but all kinds of type annotations, including simple literals from the typing module as was done before.

@pierreglaser pierreglaser Mar 15, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Trying to understand here: why would you want the pickling of these constructs to be tested in the same, single test (and not in test_pickling_stlib_typing_module_globals or something like that)? The pickling process and branches traversed to pickle C (which is the only purpose of this PR) is not the same as when pickling globals defined in the typing module. To the best of my knowledge, there is no reason for why pickling these two different constructs should be tested in a single unit-test.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

All type annotations should be preserved, dynamic or non-dynamic.

We could have additional tests for things that are specific to dynamic ones but we should check that we do not break the pickling of annotations with non-dynamic types too, no?

@pierreglaser pierreglaser Mar 15, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I agree. I simply want [my/your/any-cloudpickle-contributor/maintainer] future-self to be able to quickly understand why a specific line/commit exists, and this through atomic unit tests (we can also have composite ones, but to me atomic ones are necessary), related to the exact feature we decided to introduce in the said line/commit. In our case, this PR introduces support for pickling classes subclassing typing.Generic constructs. I think we should have an atomic unit test for this feature.

  • If we are missing tests testing the pickling of typing.Generic constructs only, let's add another test.
  • If we are missing tests testing the pickling of the globals of the typing module, let's add another test.
  • If we are missing tests testing the pickling what's inside a dynamic class/function __annotations__ (be it Generic construct or other typing constructs), let's add another test.
  • If we want to test any combination of the 4 situations above (the first one being subclassing typing.Generic), then let's add another test.

But why mixing everything up in one single test? The test suite of cloudpickle stills run pretty fast (<1 minute against CPython), not sure there is a need for optimization on this end yet.

Two final remarks:

  • I hope I don't sound like I'm bikeshedding -- I just want to make sure every one is on the same line when hitting merge.
  • If this message, along with the other ones I wrote about this topic, did not convince you, then I'll withdraw. I can also write a PR by myself once we merge this PR and Pickling of generic annotations/types in 3.5+ #318 if we don't want to spend too much time reviewing these PRs.

@ogrisel ogrisel Mar 15, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

+1 for an atomic test for public Generic type subclasses alone (without the subprocess thing). But I like to also have a big integration test that test dynamic classes annotated with all the possible kinds of types (literal, typevars, generic and generic subclasses).

# The type annotation syntax causes a SyntaxError on Python 3.5
code = textwrap.dedent("""\
class MyClass:
attribute: type_

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

To make things clear: pickling Generic constructs works fine on master right now. The only thing this PR introduces is supporting the pickling of dynamic classes defined using the

class A(typing.Generic[T]):
   ...

semantic. the type hint used in attribute as well as in method is correctly pickled (for Python 3.7+) in cloudpickle master as of now. I think adding such type hints in this tests generates more confusion that it helps.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

To make things clear: pickling Generic constructs works fine on master right now.

Maybe, but it was not properly tested. So better take the opportunity to increase the test coverage in general, not just the specific fix from this PR.


def method(self, arg: type_) -> type_:
return arg
""")
ns = {"type_": type_}
exec(code, ns)
MyClass = ns["MyClass"]

def check_annotations(obj, expected_type):
assert obj.__annotations__["attribute"] is expected_type
assert obj.method.__annotations__["arg"] is expected_type
Comment thread
ogrisel marked this conversation as resolved.
return "ok"

obj = MyClass()
assert check_annotations(obj, type_) == "ok"
assert worker.run(check_annotations, obj, type_) == "ok"


class Protocol2CloudPickleTest(CloudPickleTest):

Expand Down Expand Up @@ -2161,5 +2200,15 @@ def test_lookup_module_and_qualname_stdlib_typevar():
assert name == 'AnyStr'


def _generic_objects_to_test():
T = typing.TypeVar('T')

class C(typing.Generic[T]):
pass

yield C
yield C[int]


if __name__ == '__main__':
unittest.main()