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

Catches the exception from pickle.whichmodule() #112

Merged
merged 2 commits into from
Aug 7, 2017

Conversation

HyukjinKwon
Copy link
Member

@HyukjinKwon HyukjinKwon commented Aug 3, 2017

This PR proposes to port apache/spark@d7223bb in PySpark into cloudpickle.

In short, pickle.whichmodule could fail, for example, in case of six - https://bitbucket.org/gutworth/six/issues/63/importing-six-breaks-pickling and it looks failed in getattr.

In such case, it looks nicer just to use __main__ for the module name, which I believe we use when we can't find the module rather than throwing an exception. True, it is rather a band-aid fix but I believe this could be more beneficial in practice.

Please check out the details in https://bitbucket.org/gutworth/six/issues/63/importing-six-breaks-pickling and https://issues.apache.org/jira/browse/SPARK-16077

@HyukjinKwon
Copy link
Member Author

@rgbkrk could you take a look when you have some time if the tests pass please? I believe this one is only the workaround we should port.

(cc @holdenk I had some time to look into this. If you see more things to port for now or I misunderstood you, I will close this one)

@codecov-io
Copy link

codecov-io commented Aug 3, 2017

Codecov Report

Merging #112 into master will increase coverage by 0.54%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #112      +/-   ##
==========================================
+ Coverage   83.12%   83.66%   +0.54%     
==========================================
  Files           2        2              
  Lines         551      557       +6     
  Branches      107      107              
==========================================
+ Hits          458      466       +8     
+ Misses         65       64       -1     
+ Partials       28       27       -1
Impacted Files Coverage Δ
cloudpickle/cloudpickle.py 83.57% <100%> (+0.54%) ⬆️

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 e6de4f4...4062d3a. Read the comment docs.

@rgbkrk
Copy link
Member

rgbkrk commented Aug 4, 2017

Travis passes, the only thing that doesn't pass is a drop in coverage. If you want to make sure your case in spark is covered, I recommend adding a test.

@HyukjinKwon
Copy link
Member Author

Will give a shot. Thanks for your guidance.

@HyukjinKwon HyukjinKwon force-pushed the whichmodule-workaround branch from 9c853d9 to 4062d3a Compare August 5, 2017 12:16
@HyukjinKwon
Copy link
Member Author

I just added the test cases.

@rgbkrk rgbkrk merged commit 3c00179 into cloudpipe:master Aug 7, 2017
@rgbkrk
Copy link
Member

rgbkrk commented Aug 7, 2017

Awesome, thank you @HyukjinKwon!

@HyukjinKwon HyukjinKwon deleted the whichmodule-workaround branch August 7, 2017 17:46
@HyukjinKwon
Copy link
Member Author

Thank you @rgbkrk!

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.

3 participants