Pickle `typing.TypeVar`s not as globals #350

valtron · 2020-03-11T01:40:17Z

Broken out of #318.

codecov · 2020-03-11T01:41:31Z

Codecov Report

Merging #350 into master will increase coverage by 0.33%.
The diff coverage is 97.05%.

@@            Coverage Diff             @@
##           master     #350      +/-   ##
==========================================
+ Coverage   92.94%   93.28%   +0.33%     
==========================================
  Files           2        2              
  Lines         737      759      +22     
  Branches      152      154       +2     
==========================================
+ Hits          685      708      +23     
  Misses         26       26              
+ Partials       26       25       -1

Impacted Files	Coverage Δ
cloudpickle/cloudpickle_fast.py	`95.72% <100%> (+0.03%)`	⬆️
cloudpickle/cloudpickle.py	`92.19% <96.66%> (+0.5%)`	⬆️

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 9a0cd08...122ed0e. Read the comment docs.

pierreglaser

Thanks for the PR. Some comments below.

tests/cloudpickle_test.py

cloudpickle/cloudpickle_fast.py

cloudpickle/cloudpickle.py

valtron · 2020-03-12T02:21:46Z

(Replying to #350 (comment) and #350 (comment))

Made it keep pickling TypeVars as global, where possible. Some notes:

TypeVar.__module__ is incorrect in Python < 3.7, so the newly added _get_module_attr returns None for TypeVars. This makes _whichmodule fall back to searching for it in all the modules.
Likewise, it's not possible to Pickler.save_global TypeVars in <3.7 because pickle does an internal lookup, which fails because the typevar's module is wrong. So I hand-wrote "saving as global" here. Because I needed the module and name of the object being imported globally, I "added" _get_module_if_global (it's just the logic from _is_global, but instead of returning True/False it returns either None or the pair (module object, name of object within module)).

ogrisel

This is looking good overall. Here are some smaller suggestions. Please also add a new entry to CHANGES.md in the 1.4.0 section to document this fix.

cloudpickle/cloudpickle.py

tests/cloudpickle_test.py

cloudpickle/cloudpickle.py

pierreglaser

New pass. Mostly nitpicks, and a few more important comments. Should be good on my end once all of this is addressed.

cloudpickle/cloudpickle_fast.py

cloudpickle/cloudpickle.py

tests/cloudpickle_test.py

CHANGES.md

pierreglaser · 2020-03-13T09:19:07Z

In response to #350 (comment):

I'm sorry if you lost a lot of time trying to understand obscure specificities of the Pickler implementation, and if my explanations sound like dark magic. the pickling process is complicated, even more in cloudpickle_fast.py, where we rely on the C implementation of the Pickler, which has some differences w.r.t the pure Python Pickler.

I would rather have you ask me a couple of questions when you hit a blocker instead of loosing a lot of time, and I'm considering creating a gitter channel to facilitate informal communication in cases like this. Pinging @ogrisel

ogrisel · 2020-03-13T10:44:23Z

Ok for setting up a gitter channel.

ogrisel · 2020-03-13T10:45:19Z

Actually, it already exist: https://gitter.im/cloudpipe/cloudpickle :)

valtron · 2020-03-14T03:48:17Z

@pierreglaser Fret not, I knew what I was getting into. Debugging is an uncertain, long-tailed activity.

ogrisel · 2020-03-14T11:56:22Z

I pushed two small commits to always pass the name as a kwarg instead of positional arg + add some comments to explain non-trivial things and align the style the bullet points in the change log to the existing convention (even if wrong?).

ogrisel · 2020-03-14T11:57:08Z

@pierreglaser I think all your comments have been addressed. Merge?

cloudpickle/cloudpickle.py

pierreglaser

LGTM. Once https://github.com/cloudpipe/cloudpickle/pull/350/files#r392581912 is addressed, I'll push a commit to enhance a little bit the tests -- then this will be good to merge.

ogrisel · 2020-03-14T12:51:01Z

Done.

ogrisel · 2020-03-14T12:51:16Z

Feel free to rephrase my comment and merge.

pierreglaser · 2020-03-14T12:55:47Z

@ogrisel CI downstream maybe?

ogrisel · 2020-03-14T13:23:18Z

Sure, I let you do it.

pierreglaser · 2020-03-14T14:18:08Z

Merged. Thank you very much for this contribution @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

pierreglaser requested changes Mar 11, 2020

View reviewed changes

tests/cloudpickle_test.py Show resolved Hide resolved

cloudpickle/cloudpickle_fast.py Outdated Show resolved Hide resolved

cloudpickle/cloudpickle.py Show resolved Hide resolved

pierreglaser self-assigned this Mar 11, 2020

valtron mentioned this pull request Mar 12, 2020

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

Merged

ogrisel approved these changes Mar 12, 2020

View reviewed changes

ogrisel reviewed Mar 12, 2020

View reviewed changes

cloudpickle/cloudpickle.py Outdated Show resolved Hide resolved

pierreglaser reviewed Mar 12, 2020

View reviewed changes

cloudpickle/cloudpickle.py Outdated Show resolved Hide resolved

pierreglaser reviewed Mar 12, 2020

View reviewed changes

cloudpickle/cloudpickle.py Show resolved Hide resolved

pierreglaser reviewed Mar 12, 2020

View reviewed changes

Pickle dynamic typing.TypeVars

59d930e

ogrisel added 2 commits March 14, 2020 12:51

Always pass name as kwarg + insert comments

d3fa5ea

Cosmit

4e356a8

pierreglaser reviewed Mar 14, 2020

View reviewed changes

cloudpickle/cloudpickle.py Outdated Show resolved Hide resolved

pierreglaser approved these changes Mar 14, 2020

View reviewed changes

Fix _get_module_attr comment

7ac7a66

test for attribute equality of roundtripped typevars

fcfb2d5

flag version specific code with coverage pragmas

8a7d319

pierreglaser added the ci downstream Signal the CI to run the test suite of all registered cloudpickle downstream projects. label Mar 14, 2020

pierreglaser added the ci distributed Signal the CI to run the test suite of distributed (downstream project of cloudpickle) label Mar 14, 2020

CI trigger

122ed0e

pierreglaser merged commit 215d3dd into cloudpipe:master Mar 14, 2020

valtron deleted the local-typevars branch March 14, 2020 17:08

ogrisel mentioned this pull request Mar 15, 2020

Enable tracking for dynamically defined TypeVar instances #353

Merged

jakirkham mentioned this pull request Jun 22, 2020

deduplicate cloudpickle reducers. #368

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pickle `typing.TypeVar`s not as globals #350

Pickle `typing.TypeVar`s not as globals #350

valtron commented Mar 11, 2020

codecov bot commented Mar 11, 2020 •

edited

Loading

pierreglaser left a comment

valtron commented Mar 12, 2020

ogrisel left a comment

pierreglaser left a comment

pierreglaser commented Mar 13, 2020

ogrisel commented Mar 13, 2020

ogrisel commented Mar 13, 2020

valtron commented Mar 14, 2020

ogrisel commented Mar 14, 2020

ogrisel commented Mar 14, 2020

pierreglaser left a comment

ogrisel commented Mar 14, 2020

ogrisel commented Mar 14, 2020

pierreglaser commented Mar 14, 2020

ogrisel commented Mar 14, 2020

pierreglaser commented Mar 14, 2020

Pickle typing.TypeVars not as globals #350

Pickle typing.TypeVars not as globals #350

Conversation

valtron commented Mar 11, 2020

codecov bot commented Mar 11, 2020 • edited Loading

Codecov Report

pierreglaser left a comment

Choose a reason for hiding this comment

valtron commented Mar 12, 2020

ogrisel left a comment

Choose a reason for hiding this comment

pierreglaser left a comment

Choose a reason for hiding this comment

pierreglaser commented Mar 13, 2020

ogrisel commented Mar 13, 2020

ogrisel commented Mar 13, 2020

valtron commented Mar 14, 2020

ogrisel commented Mar 14, 2020

ogrisel commented Mar 14, 2020

pierreglaser left a comment

Choose a reason for hiding this comment

ogrisel commented Mar 14, 2020

ogrisel commented Mar 14, 2020

pierreglaser commented Mar 14, 2020

ogrisel commented Mar 14, 2020

pierreglaser commented Mar 14, 2020

Pickle `typing.TypeVar`s not as globals #350

Pickle `typing.TypeVar`s not as globals #350

codecov bot commented Mar 11, 2020 •

edited

Loading