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

Support for recursive functions inside closures #89

Closed
wants to merge 1 commit into from
Closed

Support for recursive functions inside closures #89

wants to merge 1 commit into from

Conversation

mehrdadn
Copy link

Adds the ability to reconstruct closures in distributions, such as CPython, that support ctypes.pythonapi.PyCell_Set.

@mehrdadn
Copy link
Author

Note that this pull request is intended to fix issue #62.

@codecov-io
Copy link

codecov-io commented May 25, 2017

Codecov Report

Merging #89 into master will increase coverage by 0.61%.
The diff coverage is 90%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #89      +/-   ##
==========================================
+ Coverage    79.8%   80.41%   +0.61%     
==========================================
  Files           2        2              
  Lines         505      531      +26     
  Branches      104      109       +5     
==========================================
+ Hits          403      427      +24     
- Misses         73       75       +2     
  Partials       29       29
Impacted Files Coverage Δ
cloudpickle/cloudpickle.py 80.3% <90%> (+0.62%) ⬆️

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 825e5aa...982da06. Read the comment docs.

@pcmoritz
Copy link
Contributor

@mrocklin Can you have a look at the PR and merge it if you don't see problems? I hope it will also be helpful for dask!

@mrocklin
Copy link
Contributor

Glad to see Ray folks contributing to cloudpickle, which has a strong tradition of being developed by essentially every parallel python computing project :)

FYI I'm currently somewhat slammed and so may not review this immediately. However, I'm also not the main maintainer of this library (there are several of us that each do a little bit).

@mrocklin
Copy link
Contributor

I wonder if @llllllllll has any interest in reviewing and helping out with cloudpickle? This is very slightly in his realm of interest.

@pcmoritz
Copy link
Contributor

Thanks for letting us know! It makes sense to distribute the development on many shoulders, given how many different corner cases there are and how complex it is to get all the details right :)

It is very unfortunate that there is so much complexity in the whole python pickling landscape, a more modern alternative like https://jsonpickle.github.io/ (maybe based on a good binary serialization format) would be great if there was a way to cover as much functionality as with (cloud)pickle.

@pitrou
Copy link
Member

pitrou commented May 30, 2017

Superseded by PR #90.

@pitrou pitrou closed this May 30, 2017
rgbkrk added a commit to rgbkrk/spark that referenced this pull request Jun 12, 2017
This brings in fixes and upgrades from the [cloudpickle](https://github.com/cloudpipe/cloudpickle) module, notably:

* Import submodules accessed by pickled functions (cloudpipe/cloudpickle#80)
* Support recursive functions inside closures (cloudpipe/cloudpickle#89, cloudpipe/cloudpickle#90)
* Fix ResourceWarnings and DeprecationWarnings (cloudpipe/cloudpickle#88)
* Assume modules with __file__ attribute are not dynamic (cloudpipe/cloudpickle#85)
* Make cloudpickle Python 3.6 compatible (cloudpipe/cloudpickle#72)
* Allow pickling of builtin methods (cloudpipe/cloudpickle#57)
* Add ability to pickle dynamically created modules (cloudpipe/cloudpickle#52)
* Support method descriptor (cloudpipe/cloudpickle#46)
* No more pickling of closed files, was broken on Python 3 (cloudpipe/cloudpickle#32)
ghost pushed a commit to dbtsai/spark that referenced this pull request Aug 22, 2017
## What changes were proposed in this pull request?

Based on apache#18282 by rgbkrk this PR attempts to update to the current released cloudpickle and minimize the difference between Spark cloudpickle and "stock" cloud pickle with the goal of eventually using the stock cloud pickle.

Some notable changes:
* Import submodules accessed by pickled functions (cloudpipe/cloudpickle#80)
* Support recursive functions inside closures (cloudpipe/cloudpickle#89, cloudpipe/cloudpickle#90)
* Fix ResourceWarnings and DeprecationWarnings (cloudpipe/cloudpickle#88)
* Assume modules with __file__ attribute are not dynamic (cloudpipe/cloudpickle#85)
* Make cloudpickle Python 3.6 compatible (cloudpipe/cloudpickle#72)
* Allow pickling of builtin methods (cloudpipe/cloudpickle#57)
* Add ability to pickle dynamically created modules (cloudpipe/cloudpickle#52)
* Support method descriptor (cloudpipe/cloudpickle#46)
* No more pickling of closed files, was broken on Python 3 (cloudpipe/cloudpickle#32)
* ** Remove non-standard __transient__check (cloudpipe/cloudpickle#110)** -- while we don't use this internally, and have no tests or documentation for its use, downstream code may use __transient__, although it has never been part of the API, if we merge this we should include a note about this in the release notes.
* Support for pickling loggers (yay!) (cloudpipe/cloudpickle#96)
* BUG: Fix crash when pickling dynamic class cycles. (cloudpipe/cloudpickle#102)

## How was this patch tested?

Existing PySpark unit tests + the unit tests from the cloudpickle project on their own.

Author: Holden Karau <[email protected]>
Author: Kyle Kelley <[email protected]>

Closes apache#18734 from holdenk/holden-rgbkrk-cloudpickle-upgrades.
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.

5 participants