Skip to content
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

Move all dumps to cloudpickle #30

Merged
merged 5 commits into from
Oct 22, 2020
Merged

Conversation

jrapin
Copy link
Contributor

@jrapin jrapin commented Oct 22, 2020

This reproduces facebookresearch/hydra#1083 as a test and fixes it by moving all dumps to cloudpickle.

@jrapin jrapin added the bug Something isn't working label Oct 22, 2020
@jrapin jrapin requested a review from gwenzek October 22, 2020 08:41
@jrapin jrapin self-assigned this Oct 22, 2020
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 22, 2020
@jrapin
Copy link
Contributor Author

jrapin commented Oct 22, 2020

@gwenzek do you remember any reason why we used the standard pickle for dumping results? That may just have been historical, and we would be better off with cloudpickle all along

@gwenzek
Copy link
Contributor

gwenzek commented Oct 22, 2020

LGTM. I don't think there are good reason to use pickle instead of cloudpickle for return.
It's even curious that we only hit this issue only now.

Should we cut a bug fix release ?

@gwenzek gwenzek merged commit fc70208 into facebookincubator:master Oct 22, 2020
@jrapin
Copy link
Contributor Author

jrapin commented Oct 22, 2020

Should we cut a bug fix release ?

I think that would help hydra people yes

@jrapin jrapin deleted the cloudpickle branch October 22, 2020 09:02
@omry
Copy link

omry commented Oct 22, 2020

I think it was just an oversight. @LowikC may remember ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants