-
Notifications
You must be signed in to change notification settings - Fork 7k
Remove cloudpickle customization and just use plain cloudpickle. #588
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
Conversation
|
I have an example in my bug report here. It doesn't seem to have been marked as resolved, so I expect it to still be broken in the latest version of CloudPickle. |
|
Merged build finished. Test PASSed. |
|
Test PASSed. |
|
Thanks @mehrdadn. The reason we didn't submit your patch to cloudpickle was that it only works for CPython, is that right? |
|
Yup. |
|
see also https://github.com/spartan-array/spartan/blob/master/spartan/cloudpickle.pyx, a cythonized version of cloudpickle |
|
Thanks for the link! |
|
@wesm Thanks for the link! Did you try it out by any chance and measure the speed vs. regular cloudpickle? |
|
The test failure here is #591. |
|
Merged build finished. Test PASSed. |
|
Test PASSed. |
|
I haven't tested it personally but it may be worth evaluating at some point |
|
mehrdad is fixing this whole class of problems upstream here: cloudpipe/cloudpickle#89 |
Signed-off-by: sule <[email protected]>
I'm a little surprised that the cloudpickle modifications no longer seem to be necessary. I thought we needed those changes in order to pickle remote functions that invoked other remote functions.
@mehrdadn, do you remember any examples of functions that can't be pickled normally by cloudpickle but that could be pickled with the augmented version? I can't seem to recreate such an example at the moment.
The problem probably still exists, so if that's the case we should see if we can create a patch for cloudpickle.