Support pickling dynamic classes subclassing `typing.Generic` instances on 3.7+ #351

valtron · 2020-03-12T03:01:10Z

This PR depends on #350. Only the last commit should be reviewed. When #350 is merged I'll rebase this.

This involved using types.new_class. I checked and the new tests fail without it.

codecov · 2020-03-12T03:02:16Z

Codecov Report

Merging #351 into master will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #351      +/-   ##
==========================================
+ Coverage   93.28%   93.32%   +0.04%     
==========================================
  Files           2        2              
  Lines         759      764       +5     
  Branches      154      156       +2     
==========================================
+ Hits          708      713       +5     
  Misses         26       26              
  Partials       25       25

Impacted Files	Coverage Δ
cloudpickle/cloudpickle.py	`92.26% <100.00%> (+0.07%)`	⬆️
cloudpickle/cloudpickle_fast.py	`95.72% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 215d3dd...5791b84. Read the comment docs.

ogrisel

I don't think the scope of this PR is respected, please see comment below.

cloudpickle/cloudpickle.py

pierreglaser · 2020-03-13T00:01:13Z

For the sake of every one, I think we should review this once we merge #350, and @valtron rebases this branch with master.

pierreglaser · 2020-03-14T14:20:53Z

@valtron feel free to rebase with master now that #350 is merged.

pierreglaser

Now that this is rebased, here are some comments. You also need a new entry in the changelog.

cloudpickle/cloudpickle.py

tests/cloudpickle_test.py

cloudpickle/cloudpickle.py

ogrisel

LGTM. This also need a dedicated changelog entry.

Here are a few nitpicking suggestions:

tests/cloudpickle_test.py

cloudpickle/cloudpickle.py

tests/cloudpickle_test.py

ogrisel · 2020-03-14T19:01:03Z

It's weird that my new test passes with Python 3.8 but does not work with Python 3.7. I thought that the built-in pickle memoization would ensure that the type definition would make the physical equality check work:

https://github.com/cloudpipe/cloudpickle/pull/351/checks?check_run_id=508124864

ogrisel · 2020-03-14T19:26:18Z

I have added a simpler failing test (under py37, but not py38) to debug the memoization problem in
dc25224.

Any idea?

pierreglaser · 2020-03-14T19:47:20Z

I think we miss a obj=obj in the save_reduce function call within save_typevar in cloudpickle.py. This is typically a Python < 3.8 problem, as in Python <3.8, we have to take care of calling save_reduce (as opposed to 3.8, where _pickle.c does it for us).

Good catch @ogrisel, apologies for missing that.

valtron · 2020-03-14T20:06:52Z

Thanks for the comments, I addressed all the easy ones for now.

~~Regarding #351 (comment), I still kept T, since that and subprocess_worker trigger the bug.~~ My bad, missed dc25224.

I didn't commit the save_reduce(..., obj=obj) change, though I can confirm it makes the test pass.

ogrisel · 2020-03-15T09:27:50Z

I didn't commit the save_reduce(..., obj=obj) change, though I can confirm it makes the test pass.

Why?

ogrisel · 2020-03-15T09:33:03Z

By the way I think we should also enable the definition tracking (currently only used for dynamic classes and enums) for dynamic typevars. But this can be done in a dedicated PR.

ogrisel · 2020-03-15T09:38:23Z

I pushed the memoization fix to see if the full CI will pass.

ogrisel · 2020-03-15T09:54:24Z

tests/cloudpickle_test.py

+                     "Pickling type hints not supported below py37")
+    def test_locally_defined_class_with_type_hints(self):
+        with subprocess_worker(protocol=self.protocol) as worker:
+            for type_ in _generic_objects_to_test():


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.

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.

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?

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.

+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).

pierreglaser · 2020-03-15T11:26:16Z

tests/cloudpickle_test.py

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


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.

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.

pierreglaser · 2020-03-15T11:27:43Z

tests/cloudpickle_test.py

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


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.

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.

pierreglaser

LGTM once the discussion going on about the tests is resolved.

CHANGES.md

tests/cloudpickle_test.py

Co-Authored-By: Pierre Glaser <pierreglaser@msn.com>

ogrisel · 2020-03-15T19:48:35Z

I pushed the new tests to take @pierreglaser comments into account. Everything looks good. Let's merge. I will also merge #353 after that.

Thank you very much @valtron!

2.0.0 ===== - Python 3.5 is no longer supported. - Support for registering modules to be serialised by value. This allows code defined in local modules to be serialised and executed remotely without those local modules installed on the remote machine. ([PR #417](cloudpipe/cloudpickle#417)) - Fix a side effect altering dynamic modules at pickling time. ([PR #426](cloudpipe/cloudpickle#426)) - Support for pickling type annotations on Python 3.10 as per [PEP 563]( https://www.python.org/dev/peps/pep-0563/) ([PR #400](cloudpipe/cloudpickle#400)) - Stricter parametrized type detection heuristics in _is_parametrized_type_hint to limit false positives. ([PR #409](cloudpipe/cloudpickle#409)) - Support pickling / depickling of OrderedDict KeysView, ValuesView, and ItemsView, following similar strategy for vanilla Python dictionaries. ([PR #423](cloudpipe/cloudpickle#423)) - Suppressed a source of non-determinism when pickling dynamically defined functions and handles the deprecation of co_lnotab in Python 3.10+. ([PR #428](cloudpipe/cloudpickle#428)) 1.6.0 ===== - `cloudpickle`'s pickle.Pickler subclass (currently defined as `cloudpickle.cloudpickle_fast.CloudPickler`) can and should now be accessed as `cloudpickle.Pickler`. This is the only officially supported way of accessing it. ([issue #366](cloudpipe/cloudpickle#366)) - `cloudpickle` now supports pickling `dict_keys`, `dict_items` and `dict_values`. ([PR #384](cloudpipe/cloudpickle#384)) 1.5.0 ===== - Fix a bug causing cloudpickle to crash when pickling dynamically created, importable modules. ([issue #360](cloudpipe/cloudpickle#354)) - Add optional dependency on `pickle5` to get improved performance on Python 3.6 and 3.7. ([PR #370](cloudpipe/cloudpickle#370)) - Internal refactoring to ease the use of `pickle5` in cloudpickle for Python 3.6 and 3.7. ([PR #368](cloudpipe/cloudpickle#368)) 1.4.1 ===== - Fix incompatibilities between cloudpickle 1.4.0 and Python 3.5.0/1/2 introduced by the new support of cloudpickle for pickling typing constructs. ([issue #360](cloudpipe/cloudpickle#360)) - Restore compat with loading dynamic classes pickled with cloudpickle version 1.2.1 that would reference the `types.ClassType` attribute. ([PR #359](cloudpipe/cloudpickle#359)) 1.4.0 ===== **This version requires Python 3.5 or later** - cloudpickle can now all pickle all constructs from the ``typing`` module and the ``typing_extensions`` library in Python 3.5+ ([PR #318](cloudpipe/cloudpickle#318)) - Stop pickling the annotations of a dynamic class for Python < 3.6 (follow up on #276) ([issue #347](cloudpipe/cloudpickle#347)) - Fix a bug affecting the pickling of dynamic `TypeVar` instances on Python 3.7+, and expand the support for pickling `TypeVar` instances (dynamic or non-dynamic) to Python 3.5-3.6 ([PR #350](cloudpipe/cloudpickle#350)) - Add support for pickling dynamic classes subclassing `typing.Generic` instances on Python 3.7+ ([PR #351](cloudpipe/cloudpickle#351)) 1.3.0 ===== - Fix a bug affecting dynamic modules occuring with modified builtins ([issue #316](cloudpipe/cloudpickle#316)) - Fix a bug affecting cloudpickle when non-modules objects are added into sys.modules ([PR #326](cloudpipe/cloudpickle#326)). - Fix a regression in cloudpickle and python3.8 causing an error when trying to pickle property objects. ([PR #329](cloudpipe/cloudpickle#329)). - Fix a bug when a thread imports a module while cloudpickle iterates over the module list ([PR #322](cloudpipe/cloudpickle#322)). - Add support for out-of-band pickling (Python 3.8 and later). https://docs.python.org/3/library/pickle.html#example ([issue #308](cloudpipe/cloudpickle#308)) - Fix a side effect that would redefine `types.ClassTypes` as `type` when importing cloudpickle. ([issue #337](cloudpipe/cloudpickle#337)) - Fix a bug affecting subclasses of slotted classes. ([issue #311](cloudpipe/cloudpickle#311)) - Dont pickle the abc cache of dynamically defined classes for Python 3.6- (This was already the case for python3.7+) ([issue #302](cloudpipe/cloudpickle#302)) 1.2.2 ===== - Revert the change introduced in ([issue #276](cloudpipe/cloudpickle#276)) attempting to pickle functions annotations for Python 3.4 to 3.6. It is not possible to pickle complex typing constructs for those versions (see [issue #193]( cloudpipe/cloudpickle#193)) - Fix a bug affecting bound classmethod saving on Python 2. ([issue #288](cloudpipe/cloudpickle#288)) - Add support for pickling "getset" descriptors ([issue #290](cloudpipe/cloudpickle#290)) 1.2.1 ===== - Restore (partial) support for Python 3.4 for downstream projects that have LTS versions that would benefit from cloudpickle bug fixes. 1.2.0 ===== - Leverage the C-accelerated Pickler new subclassing API (available in Python 3.8) in cloudpickle. This allows cloudpickle to pickle Python objects up to 30 times faster. ([issue #253](cloudpipe/cloudpickle#253)) - Support pickling of classmethod and staticmethod objects in python2. arguments. ([issue #262](cloudpipe/cloudpickle#262)) - Add support to pickle type annotations for Python 3.5 and 3.6 (pickling type annotations was already supported for Python 3.7, Python 3.4 might also work but is no longer officially supported by cloudpickle) ([issue #276](cloudpipe/cloudpickle#276)) - Internal refactoring to proactively detect dynamic functions and classes when pickling them. This refactoring also yields small performance improvements when pickling dynamic classes (~10%) ([issue #273](cloudpipe/cloudpickle#273)) 1.1.1 ===== - Minor release to fix a packaging issue (Markdown formatting of the long description rendered on pypi.org). The code itself is the same as 1.1.0. 1.1.0 ===== - Support the pickling of interactively-defined functions with positional-only arguments. ([issue #266](cloudpipe/cloudpickle#266)) - Track the provenance of dynamic classes and enums so as to preseve the usual `isinstance` relationship between pickled objects and their original class defintions. ([issue #246](cloudpipe/cloudpickle#246)) 1.0.0 ===== - Fix a bug making functions with keyword-only arguments forget the default values of these arguments after being pickled. ([issue #264](cloudpipe/cloudpickle#264)) 0.8.1 ===== - Fix a bug (already present before 0.5.3 and re-introduced in 0.8.0) affecting relative import instructions inside depickled functions ([issue #254](cloudpipe/cloudpickle#254)) 0.8.0 ===== - Add support for pickling interactively defined dataclasses. ([issue #245](cloudpipe/cloudpickle#245)) - Global variables referenced by functions pickled by cloudpickle are now unpickled in a new and isolated namespace scoped by the CloudPickler instance. This restores the (previously untested) behavior of cloudpickle prior to changes done in 0.5.4 for functions defined in the `__main__` module, and 0.6.0/1 for other dynamic functions. 0.7.0 ===== - Correctly serialize dynamically defined classes that have a `__slots__` attribute. ([issue #225](cloudpipe/cloudpickle#225)) 0.6.1 ===== - Fix regression in 0.6.0 which breaks the pickling of local function defined in a module, making it impossible to access builtins. ([issue #211](cloudpipe/cloudpickle#211)) 0.6.0 ===== - Ensure that unpickling a function defined in a dynamic module several times sequentially does not reset the values of global variables. ([issue #187](cloudpipe/cloudpickle#205)) - Restrict the ability to pickle annotations to python3.7+ ([issue #193]( cloudpipe/cloudpickle#193) and [issue #196]( cloudpipe/cloudpickle#196)) - Stop using the deprecated `imp` module under Python 3. ([issue #207](cloudpipe/cloudpickle#207)) - Fixed pickling issue with singleton types `NoneType`, `type(...)` and `type(NotImplemented)` ([issue #209](cloudpipe/cloudpickle#209)) 0.5.6 ===== - Ensure that unpickling a locally defined function that accesses the global variables of a module does not reset the values of the global variables if they are already initialized. ([issue #187](cloudpipe/cloudpickle#187)) 0.5.5 ===== - Fixed inconsistent version in `cloudpickle.__version__`. 0.5.4 ===== - Fixed a pickling issue for ABC in python3.7+ ([issue #180]( cloudpipe/cloudpickle#180)). - Fixed a bug when pickling functions in `__main__` that access global variables ([issue #187]( cloudpipe/cloudpickle#187)). 0.5.3 ===== - Fixed a crash in Python 2 when serializing non-hashable instancemethods of built-in types ([issue #144](cloudpipe/cloudpickle#144)). - itertools objects can also pickled ([PR #156](cloudpipe/cloudpickle#156)). - `logging.RootLogger` can be also pickled ([PR #160](cloudpipe/cloudpickle#160)). 0.5.2 ===== - Fixed a regression: `AttributeError` when loading pickles that hold a reference to a dynamically defined class from the `__main__` module. ([issue #131]( cloudpipe/cloudpickle#131)). - Make it possible to pickle classes and functions defined in faulty modules that raise an exception when trying to look-up their attributes by name. 0.5.1 ===== - Fixed `cloudpickle.__version__`. 0.5.0 ===== - Use `pickle.HIGHEST_PROTOCOL` by default. 0.4.4 ===== - `logging.RootLogger` can be also pickled ([PR #160](cloudpipe/cloudpickle#160)). 0.4.3 ===== - Fixed a regression: `AttributeError` when loading pickles that hold a reference to a dynamically defined class from the `__main__` module. ([issue #131]( cloudpipe/cloudpickle#131)). - Fixed a crash in Python 2 when serializing non-hashable instancemethods of built-in types. ([issue #144](cloudpipe/cloudpickle#144)) 0.4.2 ===== - Restored compatibility with pickles from 0.4.0. - Handle the `func.__qualname__` attribute. 0.4.1 ===== - Fixed a crash when pickling dynamic classes whose `__dict__` attribute was defined as a [`property`](https://docs.python.org/3/library/functions.html#property). Most notably, this affected dynamic [namedtuples](https://docs.python.org/2/library/collections.html#namedtuple-factory-function-for-tuples-with-named-fields) in Python 2. (cloudpipe/cloudpickle#113) - Cloudpickle now preserves the `__module__` attribute of functions (cloudpipe/cloudpickle#118). - Fixed a crash when pickling modules that don't have a `__package__` attribute (cloudpipe/cloudpickle#116). 0.4.0 ===== * Fix functions with empty cells * Allow pickling Logger objects * Fix crash when pickling dynamic class cycles * Ignore "None" mdoules added to sys.modules * Support WeakSets and ABCMeta instances * Remove non-standard `__transient__` support * Catch exception from `pickle.whichmodule()` 0.3.1 ===== * Fix version information and ship a changelog 0.3.0 ===== * Import submodules accessed by pickled functions * Support recursive functions inside closures * Fix `ResourceWarnings` and `DeprecationWarnings` * Assume modules with `__file__` attribute are not dynamic 0.2.2 ===== * Support Python 3.6 * Support Tornado Coroutines * Support builtin methods

valtron mentioned this pull request Mar 12, 2020

Pickling of generic annotations/types in 3.5+ #318

Merged

5 tasks

ogrisel reviewed Mar 12, 2020

View reviewed changes

cloudpickle/cloudpickle.py Outdated Show resolved Hide resolved

cloudpickle/cloudpickle.py Outdated Show resolved Hide resolved

Add support for pickling dynamic generics on 3.7+

7b4bb6d

ogrisel self-requested a review March 14, 2020 17:39

pierreglaser reviewed Mar 14, 2020

View reviewed changes

ogrisel approved these changes Mar 14, 2020

View reviewed changes

ogrisel added 3 commits March 14, 2020 19:43

Add new test for class with type hints in subprocess

7fd4412

PEP8

d67aad0

Workaround Python 3.5 syntax error

20a8835

ogrisel added 2 commits March 14, 2020 20:20

Sanity check

59e648e

Simpler non-regression test for dynamic typevar memoization

dc25224

Address review comments

82b5f31

Fix memoization of dynamic typevars

5326c8c

ogrisel reviewed Mar 15, 2020

View reviewed changes

pierreglaser reviewed Mar 15, 2020

View reviewed changes

pierreglaser approved these changes Mar 15, 2020

View reviewed changes

CHANGES.md Outdated Show resolved Hide resolved

pierreglaser changed the title ~~Add support for pickling dynamic generics on 3.7+~~ Support pickling dynamic classes subclassing typing.Generic instances on 3.7+ Mar 15, 2020

ogrisel reviewed Mar 15, 2020

View reviewed changes

tests/cloudpickle_test.py Show resolved Hide resolved

ogrisel mentioned this pull request Mar 15, 2020

Enable tracking for dynamically defined TypeVar instances #353

Merged

ogrisel and others added 3 commits March 15, 2020 16:29

Update tests/cloudpickle_test.py

43ef753

Update CHANGES.md

6854502

Co-Authored-By: Pierre Glaser <pierreglaser@msn.com>

Test dynamic generic subclasses and annotated dynamic classes

5791b84

ogrisel merged commit 3e80b26 into cloudpipe:master Mar 15, 2020

valtron deleted the use-new-class branch March 15, 2020 23:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support pickling dynamic classes subclassing `typing.Generic` instances on 3.7+ #351

Support pickling dynamic classes subclassing `typing.Generic` instances on 3.7+ #351

valtron commented Mar 12, 2020 •

edited

Loading

codecov bot commented Mar 12, 2020 •

edited

Loading

ogrisel left a comment

pierreglaser commented Mar 13, 2020

pierreglaser commented Mar 14, 2020

pierreglaser left a comment •

edited

Loading

ogrisel left a comment

ogrisel commented Mar 14, 2020 •

edited

Loading

ogrisel commented Mar 14, 2020 •

edited

Loading

pierreglaser commented Mar 14, 2020

valtron commented Mar 14, 2020 •

edited

Loading

ogrisel commented Mar 15, 2020

ogrisel commented Mar 15, 2020

ogrisel commented Mar 15, 2020

ogrisel Mar 15, 2020

pierreglaser Mar 15, 2020 •

edited

Loading

ogrisel Mar 15, 2020

pierreglaser Mar 15, 2020 •

edited

Loading

ogrisel Mar 15, 2020 •

edited

Loading

pierreglaser Mar 15, 2020

ogrisel Mar 15, 2020

pierreglaser Mar 15, 2020 •

edited

Loading

ogrisel Mar 15, 2020

pierreglaser left a comment

ogrisel commented Mar 15, 2020

Support pickling dynamic classes subclassing typing.Generic instances on 3.7+ #351

Support pickling dynamic classes subclassing typing.Generic instances on 3.7+ #351

Conversation

valtron commented Mar 12, 2020 • edited Loading

codecov bot commented Mar 12, 2020 • edited Loading

Codecov Report

ogrisel left a comment

Choose a reason for hiding this comment

pierreglaser commented Mar 13, 2020

pierreglaser commented Mar 14, 2020

pierreglaser left a comment • edited Loading

Choose a reason for hiding this comment

ogrisel left a comment

Choose a reason for hiding this comment

ogrisel commented Mar 14, 2020 • edited Loading

ogrisel commented Mar 14, 2020 • edited Loading

pierreglaser commented Mar 14, 2020

valtron commented Mar 14, 2020 • edited Loading

ogrisel commented Mar 15, 2020

ogrisel commented Mar 15, 2020

ogrisel commented Mar 15, 2020

ogrisel Mar 15, 2020

Choose a reason for hiding this comment

pierreglaser Mar 15, 2020 • edited Loading

Choose a reason for hiding this comment

ogrisel Mar 15, 2020

Choose a reason for hiding this comment

pierreglaser Mar 15, 2020 • edited Loading

Choose a reason for hiding this comment

ogrisel Mar 15, 2020 • edited Loading

Choose a reason for hiding this comment

pierreglaser Mar 15, 2020

Choose a reason for hiding this comment

ogrisel Mar 15, 2020

Choose a reason for hiding this comment

pierreglaser Mar 15, 2020 • edited Loading

Choose a reason for hiding this comment

ogrisel Mar 15, 2020

Choose a reason for hiding this comment

pierreglaser left a comment

Choose a reason for hiding this comment

ogrisel commented Mar 15, 2020

Support pickling dynamic classes subclassing `typing.Generic` instances on 3.7+ #351

Support pickling dynamic classes subclassing `typing.Generic` instances on 3.7+ #351

valtron commented Mar 12, 2020 •

edited

Loading

codecov bot commented Mar 12, 2020 •

edited

Loading

pierreglaser left a comment •

edited

Loading

ogrisel commented Mar 14, 2020 •

edited

Loading

ogrisel commented Mar 14, 2020 •

edited

Loading

valtron commented Mar 14, 2020 •

edited

Loading

pierreglaser Mar 15, 2020 •

edited

Loading

pierreglaser Mar 15, 2020 •

edited

Loading

ogrisel Mar 15, 2020 •

edited

Loading

pierreglaser Mar 15, 2020 •

edited

Loading