Skip to content

[Serialization] Update CloudPickle to 1.6.0#9694

Merged
rkooo567 merged 2 commits intoray-project:masterfrom
suquark:update_cloudpickle
Aug 30, 2020
Merged

[Serialization] Update CloudPickle to 1.6.0#9694
rkooo567 merged 2 commits intoray-project:masterfrom
suquark:update_cloudpickle

Conversation

@suquark
Copy link
Copy Markdown
Member

@suquark suquark commented Jul 24, 2020 •
edited
Loading

Why are these changes needed?

After #9622, we no longer need the numpy workaround in cloudpickle. Also the latest cloudpickle natively supports pickle5. This PR ships the latest cloudpickle to Ray without any patch (except adding "ray" prefix in __init__.py for correct module import)

We also move some serialization tests to the correct test script.

Related issue number

Checks

  • I've run scripts/format.sh to lint the changes in this PR.
  • I've included any doc changes needed for https://docs.ray.io/en/latest/.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failure rates at https://ray-travis-tracker.herokuapp.com/.
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested (please justify below)

@AmplabJenkins
Copy link
Copy Markdown

AmplabJenkins commented Jul 24, 2020

Can one of the admins verify this patch?

@suquark suquark force-pushed the update_cloudpickle branch from a5d4483 to 05a759c Compare July 25, 2020 01:16
@suquark suquark requested a review from pcmoritz July 25, 2020 01:59
@suquark suquark force-pushed the update_cloudpickle branch from 84b77e1 to 4a19fb7 Compare July 25, 2020 07:27
@AmplabJenkins
Copy link
Copy Markdown

AmplabJenkins commented Jul 25, 2020

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/28910/
Test FAILed.

@AmplabJenkins
Copy link
Copy Markdown

AmplabJenkins commented Jul 25, 2020

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/28916/
Test FAILed.

@AmplabJenkins
Copy link
Copy Markdown

AmplabJenkins commented Jul 25, 2020

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/28922/
Test FAILed.

@suquark suquark requested a review from ericl July 25, 2020 22:53
@suquark
Copy link
Copy Markdown
Member Author

suquark commented Jul 31, 2020

retriggered build to use the latest upstream

@suquark suquark force-pushed the update_cloudpickle branch from 4a19fb7 to 6ae637e Compare August 4, 2020 20:16
@suquark
Copy link
Copy Markdown
Member Author

suquark commented Aug 4, 2020

retriggered build to use the latest upstream

@AmplabJenkins
Copy link
Copy Markdown

AmplabJenkins commented Aug 4, 2020

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/29403/
Test FAILed.

@suquark
Copy link
Copy Markdown
Member Author

suquark commented Aug 4, 2020

It seems test_serialization gets timeout. Let me look deeper into it.

@suquark suquark changed the title Update CloudPickle to 1.5.0 [WIP] Update CloudPickle to 1.5.0 Aug 5, 2020
@rkooo567 rkooo567 self-assigned this Aug 22, 2020
@rkooo567
Copy link
Copy Markdown
Contributor

rkooo567 commented Aug 24, 2020

@suquark Do you have any plan to take a look at this sooner or later?

@rkooo567 rkooo567 added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Aug 24, 2020
@suquark suquark force-pushed the update_cloudpickle branch from 6ae637e to 66f2c3e Compare August 25, 2020 01:43
@suquark suquark removed the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Aug 25, 2020
@suquark
Copy link
Copy Markdown
Member Author

suquark commented Aug 25, 2020

let's see if rebasing would result in a successful build

@rkooo567
Copy link
Copy Markdown
Contributor

rkooo567 commented Aug 25, 2020

Still fails on test_serialization.

@suquark
Copy link
Copy Markdown
Member Author

suquark commented Aug 25, 2020

not sure what actually happened. the test is green on my local macbook. I may not have enough time to fully investigate into this issue recently.

@rkooo567
Copy link
Copy Markdown
Contributor

rkooo567 commented Aug 25, 2020

No problem! Please take ur time. If you can point me resources, I can take a look if I can fix the issue

@suquark suquark changed the title [WIP] Update CloudPickle to 1.5.0 [WIP] Update CloudPickle to 1.6.0 Aug 27, 2020
@suquark
Copy link
Copy Markdown
Member Author

suquark commented Aug 27, 2020

It only seems like the macos run takes more time than the linux one. I do not find any problem on my own macbook

@rkooo567
Copy link
Copy Markdown
Contributor

rkooo567 commented Aug 28, 2020

Ah, I see. Our Mac test is usually slower than Linux, and it causes timeout error for "small" bazel tests (which is supposed to be done within 60 seconds I believe). Just move the test to medium size test

ray/python/ray/tests/BUILD

Line 84 in 0d22c0b

"test_serialization.py",

@rkooo567 rkooo567 changed the title [WIP] Update CloudPickle to 1.6.0 [Serialization] Update CloudPickle to 1.6.0 Aug 28, 2020
@suquark suquark force-pushed the update_cloudpickle branch from efba55b to 18f8e4c Compare August 28, 2020 19:35
@suquark suquark requested a review from rkooo567 August 29, 2020 20:06
rkooo567
rkooo567 approved these changes Aug 30, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@rkooo567 rkooo567 left a comment

Choose a reason for hiding this comment

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

@suquark Thanks for fixing issues!

@rkooo567 rkooo567 merged commit f0c3910 into ray-project:master Aug 30, 2020
simon-mo added a commit to simon-mo/ray that referenced this pull request Sep 1, 2020
@simon-mo
Revert "[Serialization] Update CloudPickle to 1.6.0 (ray-project#9694)"
fc396b8 
This reverts commit f0c3910.
simon-mo added a commit that referenced this pull request Sep 1, 2020
@simon-mo
Revert "[Serialization] Update CloudPickle to 1.6.0 (#9694)" (#10460)
ddd62a1 
This reverts commit f0c3910.
simon-mo pushed a commit to simon-mo/ray that referenced this pull request Nov 5, 2020
@suquark @simon-mo
[Serialization] Update CloudPickle to 1.6.0 (ray-project#9694)
5def825 
* update cloudpickle to 1.6.0

* fix CI timeout
simon-mo added a commit that referenced this pull request Nov 6, 2020
@simon-mo
Re-Revert: [Serialization] Update CloudPickle to 1.6.0 (#9694) (#11837)
871cde9
@suquark suquark deleted the update_cloudpickle branch March 15, 2021 21:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rkooo567 rkooo567 rkooo567 approved these changes

@pcmoritz pcmoritz Awaiting requested review from pcmoritz

@ericl ericl Awaiting requested review from ericl

Assignees

@rkooo567 rkooo567

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@suquark @AmplabJenkins @rkooo567