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

Official support for python 3.12 and code clean-up after EOLed Python 3.6 and 3.7. #517

Merged
merged 43 commits into from
Oct 13, 2023

Conversation

ogrisel
Copy link
Contributor

@ogrisel ogrisel commented Oct 9, 2023

This is a takeover of #514 by @hugovk to be able to:

  • run the CI with the result of the merge with master,
  • update the changelog,
  • do more dead-code clean-up after the update of the minimum Python version requirement.

@codecov
Copy link

codecov bot commented Oct 9, 2023

Codecov Report

Attention: 50 lines in your changes are missing coverage. Please review.

Comparison is base (45dcb75) 83.44% compared to head (dc794d1) 88.90%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #517      +/-   ##
==========================================
+ Coverage   83.44%   88.90%   +5.46%     
==========================================
  Files           4        3       -1     
  Lines         725      550     -175     
  Branches      157      114      -43     
==========================================
- Hits          605      489     -116     
+ Misses         95       48      -47     
+ Partials       25       13      -12     
Files Coverage Δ
cloudpickle/__init__.py 100.00% <100.00%> (ø)
cloudpickle/cloudpickle_fast.py 100.00% <100.00%> (+8.38%) ⬆️
cloudpickle/cloudpickle.py 88.80% <84.32%> (+12.41%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ogrisel ogrisel changed the title Official support for python 3.12 Official support for python 3.12 and code clean-up after EOLed Python 3.6 and 3.7. Oct 10, 2023
@ogrisel
Copy link
Contributor Author

ogrisel commented Oct 10, 2023

@HyukjinKwon I cleaned up a lot of dead code. As a result, the code base is much simpler and tests/test_backward_compat.py still pass with Python 3.8 and I added new compat pickle files for Python 3.9 and cloudpickle 1.4.1.

Please feel free to have a look at this PR to let me know if you see potential problems.

I still haven't tested with the downstream tests since their config need an update. I think we should do that before merging this PR. I will try to work a distinct PR against master to do that.

I would also like to blackify the code base before the release, but again in a dedicated PR to be able to remove the blackification commit from the git blame history.

@HyukjinKwon
Copy link
Member

HyukjinKwon commented Oct 12, 2023

(I will likely be able to test this w/ PySpark few weeks later. would be great if we don't block this by me, and merge this first - I will report/fix separately if there's any issue)

@ogrisel ogrisel mentioned this pull request Oct 12, 2023
@ogrisel
Copy link
Contributor Author

ogrisel commented Oct 12, 2023

Thank you very much @HyukjinKwon. BTW, if you know a way to run a fraction of the tests of PySpark with a patched nightly build with the dev version of cloudpickle as we do for Ray in #519 that would help automate this process.

@ogrisel ogrisel added the ci downstream Signal the CI to run the test suite of all registered cloudpickle downstream projects. label Oct 12, 2023
@codecov
Copy link

codecov bot commented Oct 12, 2023

Codecov Report

Attention: 11 lines in your changes are missing coverage. Please review.

Comparison is base (45dcb75) 83.44% compared to head (c5c0b44) 96.07%.

Additional details and impacted files
@@             Coverage Diff             @@
##           master     #517       +/-   ##
===========================================
+ Coverage   83.44%   96.07%   +12.62%     
===========================================
  Files           4        3        -1     
  Lines         725      560      -165     
  Branches      157      116       -41     
===========================================
- Hits          605      538       -67     
+ Misses         95       11       -84     
+ Partials       25       11       -14     
Files Coverage Δ
cloudpickle/__init__.py 100.00% <100.00%> (ø)
cloudpickle/cloudpickle_fast.py 100.00% <100.00%> (+8.38%) ⬆️
cloudpickle/cloudpickle.py 96.02% <96.56%> (+19.63%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ogrisel
Copy link
Contributor Author

ogrisel commented Oct 12, 2023

Yay, downstream CI is green!

@ogrisel
Copy link
Contributor Author

ogrisel commented Oct 12, 2023

@pierreglaser I think this should be ready for review if your have the time.

Copy link
Member

@pierreglaser pierreglaser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking great, thank you @ogrisel! Just one minor comment.

cloudpickle/cloudpickle.py Show resolved Hide resolved
tests/cloudpickle_test.py Show resolved Hide resolved
dev-requirements.txt Show resolved Hide resolved
@ogrisel
Copy link
Contributor Author

ogrisel commented Oct 13, 2023

Let me merge and follow-up with a PR dedicated to using black to enforce a consistent code style automatically.

@ogrisel ogrisel merged commit c6f8cd4 into cloudpipe:master Oct 13, 2023
20 of 21 checks passed
@ogrisel ogrisel deleted the official-support-for-python-3.12 branch October 13, 2023 09:28
dongjoon-hyun pushed a commit to apache/spark that referenced this pull request Nov 24, 2023
### What changes were proposed in this pull request?

This PR proposes to upgrade Cloudpickle from 2.2.1 to 3.0.0.

### Why are the changes needed?

It includes official support of Python 3.12 (cloudpipe/cloudpickle#517)

### Does this PR introduce _any_ user-facing change?

It includes official support of Python 3.12 (cloudpipe/cloudpickle#517)

### How was this patch tested?

Relies on cloudpickle's unittests. Existing test cases should pass too.

Closes #43989 from HyukjinKwon/SPARK-46080.

Authored-by: Hyukjin Kwon <[email protected]>
Signed-off-by: Dongjoon Hyun <[email protected]>
@HyukjinKwon
Copy link
Member

Apologies for being late. I tested all, and at least all CI seems working fine with PySpark. We upgraded cloudpickle to 3.0.0 for Apache Spark 4.0.0.

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

Successfully merging this pull request may close these issues.

4 participants