Skip to content

allow pickling / depickling of odict_keys, odict_values, odict_items - #423

Closed
smacke wants to merge 0 commit into
cloudpipe:masterfrom
smacke:master
Closed

allow pickling / depickling of odict_keys, odict_values, odict_items#423
smacke wants to merge 0 commit into
cloudpipe:masterfrom
smacke:master

Conversation

@smacke

@smackesmacke commented Jun 2, 2021

Copy link
Copy Markdown
Contributor

In newer versions of Python, vanilla dictionaries have guaranteed ordering, effectively obsoleting OrderedDict. However, legacy codebases still create OrderedDict objects, which can lead to pickling errors if someone unwittingly creates an instance of, e.g., odict_keys, perhaps unaware that the object is odict_keys and not dict_keys. The error can show up far downstream of the original object creation. This PR attempts to obviate such issues in the case of OrderedDict.

ogrisel
ogrisel approved these changes Jun 22, 2021
View reviewed changes

@ogriselogrisel left a comment

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.

LGTM. Thanks for the PR. Could you please document the change in the changelog?

@codecov

codecovBot commented Jun 22, 2021•
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #423 (c295f5d) into master (685bf0c) will increase coverage by 0.19%.
The diff coverage is 100.00%.

❗ Current head c295f5d differs from pull request most recent head b127a38. Consider uploading reports for the commit b127a38 to get more accurate results
Impacted file tree graph

@@ Coverage Diff @@## master #423 +/- ##
==========================================
+ Coverage 91.80% 91.99% +0.19% 
==========================================
Files 4 4 Lines 671 687 +16 Branches 137 141 +4 ==========================================
+ Hits 616 632 +16 
Misses 34 34 Partials 21 21
Impacted FilesCoverage Δ
cloudpickle/cloudpickle.py87.21% <100.00%> (+0.25%)⬆️
cloudpickle/cloudpickle_fast.py96.87% <100.00%> (+0.09%)⬆️

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 685bf0c...b127a38. Read the comment docs.

@smacke

smacke commented Jun 22, 2021

Copy link
Copy Markdown
ContributorAuthor

LGTM. Thanks for the PR. Could you please document the change in the changelog?

Just updated. Thanks for the approval!

@ogrisel

ogrisel commented Jun 22, 2021

Copy link
Copy Markdown
Contributor

@pierreglaser feel free to merge.

pierreglaser
pierreglaser reviewed Jun 23, 2021
View reviewed changes
Comment threadtests/cloudpickle_test.py Outdated
def test_odict_values(self):
values = collections.OrderedDict([("a", 1), ("b", 2)]).values()
results = pickle_depickle(values)
self.assertEqual(sorted(results), sorted(values))

@pierreglaserpierreglaserJun 23, 2021

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.

Out of curiosity, can you explain why we to check for equality on the sorted values, and not the "raw" values?

@smackesmackeJun 23, 2021•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I think it's probably not necessary to sort. It is necessary to pull the individual elements out of the view, though. From the documentation on dict.values() (which also applies to OrderedDict.values()):

An equality comparison between one dict.values() view and another will always return False.

I was basically copying the test for dict.values() which passes the views through sorted(...). Perhaps in older versions of Python and on a different implementation than CPython, regular dictionaries could potentially give values() in different order. OrderedDict should preserve the order for values() (though the documentation doesn't explicitly say it will) so I could change it from sorted(...) to list(...) to pull the values out if you'd like.

@smackesmackeJun 29, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@pierreglaser any preference on whether to change sorted(...) to list(...)?

@ogriselogriselJun 30, 2021

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 list.

@smackesmackeJun 30, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Done.

pierreglaser
pierreglaser approved these changes Jun 23, 2021
View reviewed changes

@pierreglaserpierreglaser left a comment •
edited
Loading

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.

LGTM, I just have one question.

@smacke

smacke commented Jun 23, 2021

Copy link
Copy Markdown
ContributorAuthor

LGTM, I just have one question.

I replied inline.

ogrisel
ogrisel reviewed Jul 1, 2021
View reviewed changes
Comment threadcloudpickle/cloudpickle_fast.py Outdated
@ogrisel

ogrisel commented Jul 1, 2021

Copy link
Copy Markdown
Contributor

Oops sorry I broke the PR by editing it too quickly on github. Let me undo my last commit.

@ogriselogrisel closed this Jul 1, 2021
@ogrisel

ogrisel commented Jul 1, 2021

Copy link
Copy Markdown
Contributor

I force pushed master into you branch because it is also named master and it close the PR as result... I have a local copy of your work and will open a new PR. Sorry for the confusion.

@ogriselogrisel mentioned this pull request Jul 1, 2021
Merged
@ogrisel

ogrisel commented Jul 1, 2021

Copy link
Copy Markdown
Contributor

Ok I merged #429. Thanks for the contrib @smacke!

@smacke

smacke commented Jul 1, 2021

Copy link
Copy Markdown
ContributorAuthor

Ok I merged #429. Thanks for the contrib @smacke!

My pleasure, thanks for the stamp!

wip-sync pushed a commit to NetBSD/pkgsrc-wip that referenced this pull request Apr 4, 2022
Matthew Danielson
py-cloudpickle: updated to 2.0.0
71089d6
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
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ogriselogriselogrisel approved these changes
@pierreglaserpierreglaserpierreglaser approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@smacke@ogrisel@pierreglaser