Skip to content
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.

Sign up for GitHub

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

FIX: backward compat for types.ClassType attribute #359

Merged
merged 3 commits into from
Apr 28, 2020
Merged

FIX: backward compat for types.ClassType attribute #359

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f0908ea
FIX: backward compat for types.ClassType attribute
ogrisel Apr 27, 2020
1c49f19
typo
ogrisel Apr 27, 2020
45309c1
Add entry to changelog
ogrisel Apr 28, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
1.4.1 (in development)
======================

- Restore compat with loading dynamic classes pickled with cloudpickle
version 1.2.1 that would reference the `types.ClassType` attribute.
([PR #359](https://github.com/cloudpipe/cloudpickle/pull/359))


1.4.0
Expand Down
5 changes: 5 additions & 0 deletions cloudpickle/cloudpickle.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -388,6 +388,11 @@ def _cell_set_factory(value):


def _builtin_type(name):
if name == "ClassType": # pragma: no cover
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if in this kind of situation we could

  • be nice, provide a fix but emit a warning saying that this pickle has been created using an old version of cloudpickle and that this is not part of cloudpickle's contract
  • then, one or two minor versions later, we can safely suppress the fix.

Pretty much like a deprecation warning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+1 although I could not trigger the issue when pickling an instance of a dynamic class in a ipython shell with cloudpickle 1.2.1 installed and unpickled later in another shell with cloudpickle master. Maybe I did something wrong?

Copy link
Member

Choose a reason for hiding this comment

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

The following reproduces the issue on my machine:
dump script (cloudpickle 1.2.1)

In [3]: import cloudpickle
   ...: class A:
   ...:     pass
   ...: with open('pickle_file.pkl', 'wb') as f:
   ...:     cloudpickle.dump(A, f)

load script (cloudpickle 1.4.0)

In [3]: import pickle
   ...: with open('pickle_file.pkl', 'rb') as f:
   ...:     A = pickle.load(f)

Copy link
Contributor Author

@ogrisel ogrisel Apr 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Weird I thought I did this. Let me try again. Update: Ok it works as expected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually let's be nice and silent for now and we will add a FutureWarning in an upcoming release.

Copy link
Member

Choose a reason for hiding this comment

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

I'm okay with this.

# Backward compat to load pickle files generated with cloudpickle
# < 1.3 even if loading pickle files from older versions is not
# officially supported.
return type
return getattr(types, name)


Expand Down