Skip to content

Revert type hints support - #299

Merged
ogrisel merged 5 commits into
cloudpipe:masterfrom
pierreglaser:revert-type-hints-support
Sep 9, 2019
Merged

Revert type hints support#299
ogrisel merged 5 commits into
cloudpipe:masterfrom
pierreglaser:revert-type-hints-support

Conversation

@pierreglaser

Copy link
Copy Markdown
Member

Following #298 (comment), we need to revert the support for pickling annotation in Python3.4-3.6 introduced in #276 . Pickling annotations on those versions was a known issue (#193) , but I was not aware of it when #276 got merged.
I added a note in the readme, as well as some comments in the code to make it clear why we start supporting annotations from 3.7 whereas they were introduced in python3.4.

@codecov

codecovBot commented Jul 17, 2019

Copy link
Copy Markdown

Codecov Report

Merging #299 into master will decrease coverage by 25.35%.
The diff coverage is 100%.

Impacted file tree graph

@@ Coverage Diff @@## master #299 +/- ##
===========================================
- Coverage 92.84% 67.48% -25.36% 
===========================================
Files 2 2 Lines 852 852 Branches 177 177 ===========================================
- Hits 791 575 -216 - Misses 37 257 +220 + Partials 24 20 -4
Impacted FilesCoverage Δ
cloudpickle/cloudpickle.py92% <100%> (ø)⬆️
cloudpickle/cloudpickle_fast.py0% <0%> (-95.16%)⬇️

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 2db58f1...eee91f2. Read the comment docs.

@codecov

codecovBot commented Jul 17, 2019

Copy link
Copy Markdown

Codecov Report

Merging #299 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@ Coverage Diff @@## master #299 +/- ##
=======================================
Coverage 92.84% 92.84% =======================================
Files 2 2 Lines 852 852 Branches 177 177 =======================================
Hits 791 791 Misses 37 37 Partials 24 24
Impacted FilesCoverage Δ
cloudpickle/cloudpickle.py92% <100%> (ø)⬆️

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 2db58f1...27f7346. Read the comment docs.

@jrbourbeaujrbourbeau left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for your work on this @pierreglaser! I just wanted to gently check in on this PR. It look like the test failures here are unrelated to the changes in this PR and I think were fixed in #300 (by you in fact!)

@pierreglaser

Copy link
Copy Markdown
MemberAuthor

I'm going to merge soon, but maybe @ogrisel or @llllllllll could make a pass before (should be trivial)

@llllllllll

Copy link
Copy Markdown
Contributor

LGTM

@ogriselogrisel left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Some nitpicks both otherwise LGTM.

Comment threadREADME.md
Comment threadtests/cloudpickle_test.py
@ogrisel
ogrisel merged commit ad35704 into cloudpipe:masterSep 9, 2019
@ogrisel

Copy link
Copy Markdown
Contributor

Merged. Thanks @pierreglaser!

@jrbourbeau

Copy link
Copy Markdown
Member

Thanks @pierreglaser! This should resolve an issue we're seeing over in dask/dask#5317

@ksangeek

Copy link
Copy Markdown

Thanks @pierreglaser for fixing this and @jrbourbeau for identifying this as the root cause for dask/dask#5317!

@pierreglaser@ogrisel May I request a new release for this fix to be picked up in released conda packages - https://anaconda.org/search?q=cloudpickle.

Sign up for freeto 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

@pierreglaser@llllllllll@ogrisel@jrbourbeau@ksangeek