Skip to content

Upgrade to cloudpickle 0.9.0.dev0 #4073

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.

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

Closed
wants to merge 2 commits into from

Conversation

pcmoritz
Copy link
Contributor

@pcmoritz pcmoritz commented Feb 16, 2019

This includes support for enum classes, see cloudpipe/cloudpickle#246.

This is to test things out, we should wait merging this until the final version of cloudpipe/cloudpickle#246 is merged.

Related issues: #3996 #3917

@pcmoritz pcmoritz marked this pull request as ready for review February 16, 2019 20:51
@pcmoritz pcmoritz closed this Feb 16, 2019
@pcmoritz pcmoritz reopened this Feb 16, 2019
@AmplabJenkins
Copy link

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

@ogrisel
Copy link

ogrisel commented Feb 17, 2019

There are some tests failing at test_actors_and_tasks_with_gpus_version_two which does not seem to be related to cloudpickle.

There are other Python 2.7 tests that fail because the enum module does not exist. This is weird because the Python 2.7 tests pass on my tox -e py27 env for cloudpickle. Maybe older Python 2.7 versions did not have enums?

Anyway, it should be quite easy to do a conditional import for this module in cloudpickle.

@ogrisel
Copy link

ogrisel commented Feb 17, 2019

I've just pushed a new commit to cloudpipe/cloudpickle#246 to make the dependency on enum34 optional (under Python < 3.4).

@ogrisel
Copy link

ogrisel commented Feb 18, 2019

The failure in https://travis-ci.com/ray-project/ray/jobs/178328001 is suspicious (the operation lasts 19s instead of less than 10s). It could be caused by a performance regression in cloudpickle but this test does not seem to involve any dynamic class definition so I am not sure how cloudpipe/cloudpickle#246 could cause this.

@ogrisel
Copy link

ogrisel commented Feb 18, 2019

The failure in https://travis-ci.com/ray-project/ray/jobs/178328007 is weird: no output for 10min (in python/ray/tune/test/trial_runner_test.py::TrainableFunctionApiTest::testBadParams5 ) looks like a deadlock. Has this been observed before in ray or it this the first time?

Using the pytest-timeout plugin would be useful to dump the traceback of all the python threads in case of deadlock.

@ogrisel
Copy link

ogrisel commented Feb 18, 2019

There is also this ArrowIOError: Encountered unexpected EOF in test_multithreading:

https://travis-ci.com/ray-project/ray/jobs/178327998

@pcmoritz
Copy link
Contributor Author

pcmoritz commented Feb 18, 2019

@ogrisel Thanks for all the help, I just updated to the latest version of the code! The tests look good except the testBadParams5 one which I haven't seen fail. I'll look some more about the cloudpickle changes and the code that is involved here and see if I can spot anything suspicious.

@AmplabJenkins
Copy link

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

@ogrisel
Copy link

ogrisel commented Feb 20, 2019

Alright thanks @pcmoritz. Let me know if you have any questions on the new semantics for dynamic class defintions introduced in the cloudpickle PR.

@pcmoritz pcmoritz force-pushed the cloudpickle-0.9.0.dev0 branch from 10e2850 to 6fb394b Compare February 21, 2019 00:43
@AmplabJenkins
Copy link

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

@pcmoritz
Copy link
Contributor Author

Jenkins retest this please

@AmplabJenkins
Copy link

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

@pcmoritz
Copy link
Contributor Author

Jenkins retest this please

@AmplabJenkins
Copy link

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

@pcmoritz
Copy link
Contributor Author

@ogrisel The PR looks good to me!

@ogrisel
Copy link

ogrisel commented Feb 22, 2019

@ogrisel The PR looks good to me!

That's excellent news. You mean that the remaining CI failures on Ray are unrelated? If so, please put a comment on the cloudpickle PR :)

@AmplabJenkins
Copy link

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

@AmplabJenkins
Copy link

Can one of the admins verify this patch?

@simon-mo
Copy link
Contributor

simon-mo commented Dec 9, 2019

We already updated cloudpickle. Thanks for the PR! Closing

@simon-mo simon-mo closed this Dec 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants