Skip to content

[NOMRG] test pickle5 + ray + refactored cloudpickle #389

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 70 commits into from

Conversation

pierreglaser
Copy link
Member

@pierreglaser pierreglaser commented Jun 30, 2020

This is a rebase of #370 with the newest changes in #368. in this PR, ray's downstream build should run succesfully.
I'm making a temporary PR to not mess up #370 before I'm sure ray tests succeed.

@pierreglaser pierreglaser added the ci ray Signal the CI to run the test suite of ray (downstream project of cloudpickle) label Jun 30, 2020
@codecov
Copy link

codecov bot commented Jun 30, 2020

Codecov Report

Merging #389 into master will decrease coverage by 10.41%.
The diff coverage is 65.08%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #389       +/-   ##
===========================================
- Coverage   92.91%   82.50%   -10.42%     
===========================================
  Files           2        3        +1     
  Lines         805      640      -165     
  Branches      164      134       -30     
===========================================
- Hits          748      528      -220     
- Misses         29       88       +59     
+ Partials       28       24        -4     
Impacted Files Coverage Δ
cloudpickle/cloudpickle_fast.py 79.19% <53.84%> (-16.54%) ⬇️
cloudpickle/compat.py 70.00% <70.00%> (ø)
cloudpickle/cloudpickle.py 85.84% <95.23%> (-5.93%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e2fcfeb...7aabd2a. Read the comment docs.

@pierreglaser
Copy link
Member Author

pierreglaser commented Jun 30, 2020

@jakirkham the failing test mentions a TypeError in the traceback. Looks similar to pitrou/pickle5-backport#19 no?

I'm trying to have ray use the latest pickle5 by pre-installing pickle5 on the CI before installing ray, so that ray does not use its pinned, vendored pickle5. But it does not seem to work...

EDIT: actually, I'm not sure it's about that. Seems like an interaction with a custom registered reducer implemented by ray.

@pierreglaser
Copy link
Member Author

Alright. the ray downstream build is passing. That was instructive, as it made me realize that #370 extends the pure-python Pickler instead of the C-implemented Pickler if using pickle5.

@jakirkham
Copy link
Member

@jakirkham the failing test mentions a TypeError in the traceback. Looks similar to pitrou/pickle5-backport#19 no?

Think that is fixed in pickle5 0.0.11+. Or at least PR ( pitrou/pickle5-backport#20 ) fixed a similar issue for me.

EDIT: actually, I'm not sure it's about that. Seems like an interaction with a custom registered reducer implemented by ray.

Yeah actually I knew the fix worked when I tried it, but I think more recent changes in PR ( #368 ) may have alleviated the underlying problem (as I had trouble reproducing the issue with an older pickle5). Tricky keeping track of code in multiple libraries/PRs 😅

@jakirkham
Copy link
Member

That was instructive, as it made me realize that #370 extends the pure-python Pickler instead of the C-implemented Pickler if using pickle5.

Interesting. Thanks for surfacing that. I'll take another look and see if we can fix it. 🙂

@pierreglaser
Copy link
Member Author

Interesting. Thanks for surfacing that. I'll take another look and see if we can fix it.

merging this PR inside #370 should do the trick, as I had to fix the issue I mentioned to make the test pass :)

@jakirkham
Copy link
Member

Oh great! Thanks Pierre 😄

@pierreglaser
Copy link
Member Author

pierreglaser commented Jun 30, 2020

Closing as this is superseded by #370

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci ray Signal the CI to run the test suite of ray (downstream project of cloudpickle)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants