Skip to content

Conversation

@HyukjinKwon
Copy link
Member

@HyukjinKwon HyukjinKwon commented Jan 4, 2021

What changes were proposed in this pull request?

This PR proposes to upgrade cloudpickle from 1.5.0 to 1.6.0.
It virtually contains one fix:

cloudpipe/cloudpickle@4510be8

From a cursory look, this isn't a regression, and not even properly supported in Python:

>>> import pickle
>>> pickle.dumps({}.keys())
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: cannot pickle 'dict_keys' object

So it seems fine not to backport.

Why are the changes needed?

To leverage bug fixes from the cloudpickle upstream.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Jenkins build and GitHub actions build will test it out.


# Declaring a function inside another one using the "def ..."
# syntax generates a constant code object corresponding to the one
# syntax generates a constant code object corresonding to the one
Copy link
Member

Choose a reason for hiding this comment

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

typo?

Copy link
Member Author

Choose a reason for hiding this comment

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

Its a copy from cloudpickle. I think I would just keep it as is.

Copy link
Member Author

Choose a reason for hiding this comment

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

cloudpipe/cloudpickle#406. Let's fix it back when we upgrade next time.


# Track the provenance of reconstructed dynamic classes to make it possible to
# reconstruct instances from the matching singleton class definition when
# recontruct instances from the matching singleton class definition when
Copy link
Member

Choose a reason for hiding this comment

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

typo?

Copy link
Member

@MaxGekk MaxGekk left a comment

Choose a reason for hiding this comment

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

Could you re-check all changes in comments.

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

+1, LGTM. I also agree with @HyukjinKwon . We had better fix the upstream and keep our fork as a plain copy.

Historically, 13fd272 fixed the typos here. I'm okay to lose the typo corrections.

Thank you, @HyukjinKwon and @MaxGekk . Merged to master for Apache Spark 3.2.0.

@HyukjinKwon
Copy link
Member Author

Thanks @MaxGekk and @dongjoon-hyun !

@HyukjinKwon HyukjinKwon deleted the cloudpickle-upgrade branch January 4, 2022 00:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants