Initial implementation for pickle5 support #5611

suquark · 2019-09-01T22:15:30Z

Why are these changes needed?

This one of the plans to refactor the serialization part.

Pass all tests on python3.8
Backport pickle5
Check compatibility of cloudpickle_fast.py
~~Direct Arrow zero-copy support for PickleBuffer~~(should implement in the future)

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://ray.readthedocs.io/en/latest/.

pcmoritz · 2019-09-01T22:31:40Z

This looks great @suquark! Can we make it so there is a flag that uses one or the other codepath (the default should be the old one) so we can merge this earlier and start experimenting with it?

AmplabJenkins · 2019-09-01T22:46:44Z

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

AmplabJenkins · 2019-09-02T04:30:20Z

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

AmplabJenkins · 2019-09-02T09:32:08Z

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

pcmoritz · 2019-09-05T18:19:30Z

@suquark There are some more linting errors, can you also fix them? Let's merge the PR as it is and address the other issues in follow up PRs :)

suquark · 2019-09-05T22:17:01Z

@pcmoritz Sorry for my late response. I am still working on it, it's not ready for review.

pcmoritz · 2019-09-05T23:37:13Z

@richardliaw has also created a PR to update cloudpickle, you should coordinate to avoid conflicts (also make sure the version gets updated), see #5643 :)

suquark · 2019-09-05T23:58:33Z

@pcmoritz Thanks for your mention! I am also using the lastest cloudpickle but slightly modified. I think I should wait #5643 to merge so that this PR can work as a small patch.

pcmoritz · 2019-09-06T00:02:55Z

@suquark Yeah, makes sense! :)

AmplabJenkins · 2019-09-06T00:15:03Z

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

AmplabJenkins · 2019-09-06T01:10:58Z

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

AmplabJenkins · 2019-09-06T01:46:06Z

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

AmplabJenkins · 2019-09-06T02:58:05Z

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

pcmoritz · 2019-09-06T07:07:30Z

@suquark Looks like the other PR has some problem to pass Jenkins. We shouldn't have this PR be blocked on the other one, how about we just remove the update of cloudpickle.py from this PR, so we can update it separately in a different PR?

suquark · 2019-09-06T08:03:57Z

@pcmoritz Yep, but then I have to drop the Python3.8 cloudpickle_fast support (it depends on the lastest cloudpickle). But it seems totally fine for me since we will not support python3.8 recently.

AmplabJenkins · 2019-09-06T12:59:53Z

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

AmplabJenkins · 2019-09-07T00:29:38Z

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

suquark · 2019-09-07T06:10:57Z

The jenkins build failed with:

    Autodetected HDF5 1.10.2
    ********************************************************************************
                           Summary of the h5py configuration
    
        Path to HDF5: None
        HDF5 Version: '1.10.2'
         MPI Enabled: False
    Rebuild Required: True
    
    ********************************************************************************
    Executing cythonize()
    h5py requires pkg-config unless the HDF5 path is explicitly specified
    error: pkg-config probably not installed: FileNotFoundError(2, "No such file or directory: 'pkg-config'")

suquark · 2019-09-07T06:16:01Z

It seems that all tests have failed due to h5py. Not sure what has happened.

suquark · 2019-09-07T23:01:48Z

jenkins retest it please

suquark · 2019-09-07T23:05:57Z

close and reopen the PR to re-trigger all tests

pcmoritz · 2019-09-10T00:19:08Z

Now that we have #5643 merged, can you rebase this PR and we can get it merged as well?

AmplabJenkins · 2019-09-10T03:11:25Z

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

AmplabJenkins · 2019-09-10T06:55:24Z

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

AmplabJenkins · 2019-09-10T10:35:03Z

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

suquark · 2019-09-11T00:40:02Z

@pcmoritz it's ready to review and merge

pcmoritz · 2019-09-12T00:23:44Z

python/ray/cloudpickle/cloudpickle.py

@@ -59,6 +58,14 @@
 import uuid
 import threading

+PICKLE5_ENABLED = False


This should only be needed for cloudpickle_fast, right? Let's focus on supporting that codepath, so it doesn't break other things.

If we we have pickle5 enabled (python3 && python<3.8 && pickle5 installed), then we will try to use the old cloudpickle

pcmoritz · 2019-09-12T00:26:29Z

Thanks, we need to make sure that cloudpickle as well as cloudpickle_fast are unchanged from upstream. We can't start to diverge from them, otherwise it will be very hard to maintain our fork in the future (there are frequent changes in cloudpickle).

If they need to be changed, we need to get the changes upstream. If possible let's try to avoid that (there is a way to not change them by re-running the CloudPickler.init as in the code I shared with you, right?)

suquark · 2019-09-12T00:29:36Z

@pcmoritz Yep, but that code doesn't work somehow. We didn't find out why it failed weeks ago.

pcmoritz · 2019-09-12T00:32:15Z

@suquark Do you know what is going wrong?

pcmoritz · 2019-09-12T00:34:21Z

It's fine if we cannot support the old cloudpickle with pickle5 for now I think (the old cloudpickle can be quite slow like 20x slower already on python objects since it uses the python pickle implementation, without C extensions)

suquark · 2019-09-12T00:38:34Z

@pcmoritz I am not sure. But it could because of how python3.8 deals with the C++ hook function in cloudpickle_fast. I haven't tried on pickle5-backport, but it is likely to work because pickle5-backport haven't implemented the hook function. BTW, the hook function is critical for performance. It is not possible to make use of cPickle without the hook function (everything falls back to python-based pickle). I also created this issue for pickle5-backport: pitrou/pickle5-backport#9

suquark · 2019-09-12T00:39:57Z

@pcmoritz yes, but I can create a PR to make the old cloudpickle 20x faster with pickle5-backport by implementing the hook function.

suquark · 2019-09-12T01:13:16Z

Here's the cloudpickle PR: cloudpipe/cloudpickle#308

pcmoritz · 2019-09-12T01:16:37Z

Thanks for doing it, looks great!

AmplabJenkins · 2019-09-12T05:10:06Z

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

pcmoritz · 2019-09-13T00:04:29Z

Jenkins retest this please

pcmoritz

Can be merged when tests pass

AmplabJenkins · 2019-09-13T04:11:49Z

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

AmplabJenkins · 2019-09-13T19:18:44Z

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

AmplabJenkins · 2019-09-14T00:00:01Z

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

AmplabJenkins · 2019-09-14T00:20:45Z

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

AmplabJenkins · 2019-09-14T03:23:23Z

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

suquark force-pushed the pickle5serializaion branch from e80b4e5 to b430168 Compare September 2, 2019 04:02

pcmoritz marked this pull request as ready for review September 5, 2019 18:19

suquark force-pushed the pickle5serializaion branch from 5260a17 to 11d1b97 Compare September 5, 2019 22:34

suquark requested a review from pcmoritz September 7, 2019 22:28

suquark closed this Sep 7, 2019

suquark reopened this Sep 7, 2019

pcmoritz mentioned this pull request Sep 8, 2019

Update Cloudpickle #5643

Merged

2 tasks

suquark force-pushed the pickle5serializaion branch from 1f3a100 to fbceecf Compare September 10, 2019 01:40

lint

fd67311

suquark force-pushed the pickle5serializaion branch from 82a0ca4 to fd67311 Compare September 10, 2019 04:13

pcmoritz reviewed Sep 12, 2019

View reviewed changes

revert changes about pickle5

7364f0d

pcmoritz approved these changes Sep 13, 2019

View reviewed changes

pcmoritz added 2 commits September 13, 2019 10:58

Merge branch 'master' into pickle5serializaion

ed409ac

small fix

7ad7c41

pcmoritz added 2 commits September 13, 2019 13:15

lint

56dd493

port on core_worker

2748512

pcmoritz merged commit 4c964c0 into ray-project:master Sep 14, 2019

suquark deleted the pickle5serializaion branch October 3, 2019 23:23

Initial implementation for pickle5 support #5611

Initial implementation for pickle5 support #5611

Conversation

suquark commented Sep 1, 2019 • edited Loading

Why are these changes needed?

Related issue number

Checks

pcmoritz commented Sep 1, 2019

AmplabJenkins commented Sep 1, 2019

AmplabJenkins commented Sep 2, 2019

AmplabJenkins commented Sep 2, 2019

pcmoritz commented Sep 5, 2019

suquark commented Sep 5, 2019

pcmoritz commented Sep 5, 2019

suquark commented Sep 5, 2019

pcmoritz commented Sep 6, 2019

AmplabJenkins commented Sep 6, 2019

AmplabJenkins commented Sep 6, 2019

AmplabJenkins commented Sep 6, 2019

AmplabJenkins commented Sep 6, 2019

pcmoritz commented Sep 6, 2019

suquark commented Sep 6, 2019 • edited Loading

AmplabJenkins commented Sep 6, 2019

AmplabJenkins commented Sep 7, 2019

suquark commented Sep 7, 2019

suquark commented Sep 7, 2019

suquark commented Sep 7, 2019

suquark commented Sep 7, 2019

pcmoritz commented Sep 10, 2019

AmplabJenkins commented Sep 10, 2019

AmplabJenkins commented Sep 10, 2019

AmplabJenkins commented Sep 10, 2019

suquark commented Sep 11, 2019

pcmoritz Sep 12, 2019

Choose a reason for hiding this comment

suquark Sep 12, 2019 • edited Loading

Choose a reason for hiding this comment

pcmoritz commented Sep 12, 2019

suquark commented Sep 12, 2019 • edited Loading

pcmoritz commented Sep 12, 2019

pcmoritz commented Sep 12, 2019 • edited Loading

suquark commented Sep 12, 2019

suquark commented Sep 12, 2019 • edited Loading

suquark commented Sep 12, 2019

pcmoritz commented Sep 12, 2019

AmplabJenkins commented Sep 12, 2019

pcmoritz commented Sep 13, 2019

pcmoritz left a comment

Choose a reason for hiding this comment

AmplabJenkins commented Sep 13, 2019

AmplabJenkins commented Sep 13, 2019

AmplabJenkins commented Sep 14, 2019

AmplabJenkins commented Sep 14, 2019

AmplabJenkins commented Sep 14, 2019

suquark commented Sep 1, 2019 •

edited

Loading

suquark commented Sep 6, 2019 •

edited

Loading

suquark Sep 12, 2019 •

edited

Loading

suquark commented Sep 12, 2019 •

edited

Loading

pcmoritz commented Sep 12, 2019 •

edited

Loading

suquark commented Sep 12, 2019 •

edited

Loading