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

fix: Disallow cloudpickle v1.5.0 in 'tensorflow' extra #915

Merged
merged 2 commits into from Jul 2, 2020

Conversation

matthewfeickert
Copy link
Member

@matthewfeickert matthewfeickert commented Jul 2, 2020

Description

Resolves #913

This is a temporary fix to Issue #913, as the actual fix in TensorFlow Probability PR 993 won't be available in a PyPI release until TensorFlow Probability v0.11.0. By explicitly disallowing cloudpickle v1.5.0 this patch unblocks development.

Checklist Before Requesting Reviewer

  • Tests are passing (Current release tests are incapable of passing on this PR)
  • "WIP" removed from the title of the pull request
  • Selected an Assignee for the PR to be responsible for the log summary

Before Merging

For the PR Assignees:

  • Summarize commit messages into a comprehensive review of the PR
* Explicitly disallow cloudpickle v1.5.0 to avoid breaking TensorFlow Probability 
   - This is a temporary solution to unblock development, but should be removed once TensorFlow Probability v0.11 has been released
   - https://github.com/tensorflow/probability/issues/991
   - https://github.com/cloudpipe/cloudpickle/issues/390

@matthewfeickert matthewfeickert added the fix A bug fix label Jul 2, 2020
@matthewfeickert matthewfeickert self-assigned this Jul 2, 2020
@codecov
Copy link

codecov bot commented Jul 2, 2020

Codecov Report

Merging #915 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #915   +/-   ##
=======================================
  Coverage   96.28%   96.29%           
=======================================
  Files          56       56           
  Lines        3180     3188    +8     
  Branches      438      438           
=======================================
+ Hits         3062     3070    +8     
  Misses         75       75           
  Partials       43       43           
Flag Coverage Δ
#unittests 96.29% <100.00%> (+<0.01%) ⬆️
Impacted Files Coverage Δ
src/pyhf/tensor/jax_backend.py 94.26% <100.00%> (+0.09%) ⬆️
src/pyhf/tensor/numpy_backend.py 98.26% <100.00%> (+0.03%) ⬆️
src/pyhf/tensor/pytorch_backend.py 98.03% <100.00%> (+0.03%) ⬆️
src/pyhf/tensor/tensorflow_backend.py 96.46% <100.00%> (+0.06%) ⬆️

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 6a1f996...b6f1945. Read the comment docs.

cloudpickle v1.5.0 breaks TensorFlow Probability given an API change.

c.f.
- pyhf Issue 813
- TFP Issue 991
@matthewfeickert
Copy link
Member Author

This is needed before PR #914 can go in.

@matthewfeickert matthewfeickert added the bumpversion/patch Create a patch version release label Jul 2, 2020
@github-actions
Copy link

github-actions bot commented Jul 2, 2020

I've queued this up. When it gets merged, I'll create a patch release from v0.4.3 → v0.4.4 which includes the following 11 change(s) [including this PR]:

  • fix: Disallow cloudpickle v1.5.0 in 'tensorflow' extra
  • feat: Add precision information to tensorlib backends
  • ci: Verify build backend is compliant with PEP 517 in publishing workflow
  • fix: make pytorch_backend use configured dtypes
  • ci: Separate linting from testing
  • fix: Update infer.mle docstrings to match SciPy v1.5.0
  • fix: Keep Sphinx at v3.0.X releases
  • fix: Explicitly avoid Sphinx v3.1.0 in 'docs' extra
  • ci: Use Python 3.8 in all steps of publishing distributions
  • docs: Add 1Lbb hepdata likelihood record
  • fix: Check if HEAD of origin/master is accessible from latest tag

  • If you make any more changes, you probably want to re-trigger me again by removing the bumpversion/patch label and then adding it back again.

    @matthewfeickert
    Copy link
    Member Author

    As the current release tests are all breaking as a result of Issue #913, we're going to have to make a patch release from this PR unless we want to deal with the current release tests being broken until whenever TFP v0.11 comes out.

    Copy link
    Contributor

    @lukasheinrich lukasheinrich left a comment

    Choose a reason for hiding this comment

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

    are we tracking this in an issue so we remember to roll this back if tfp updates?

    @matthewfeickert
    Copy link
    Member Author

    matthewfeickert commented Jul 2, 2020

    are we tracking this in an issue so we remember to roll this back if tfp updates?

    This is now detailed in Issue #916

    @matthewfeickert matthewfeickert merged commit 9d0a85a into master Jul 2, 2020
    @matthewfeickert matthewfeickert deleted the fix/limit-cloudpickle-to-not-v1.5.0 branch July 2, 2020 17:30
    kratsg pushed a commit that referenced this pull request Jul 2, 2020
    Triggered by #915 via GitHub Actions.
    @matthewfeickert
    Copy link
    Member Author

    In retrospect, this

    'cloudpickle!=1.5.0', # TODO: Temp patch until tfp v0.11

    probably should have been

    'cloudpickle<1.5.0'

    as it is possible given their release history that cloudpickle could make a new release before TensorFlow Probability v0.11.0.

    matthewfeickert added a commit that referenced this pull request Jul 7, 2020
    …#924)
    
    * Remove constraints on cloudpickle version
       - cloudpickle version is constrained in TensorFlow Probability
       - Reverts PR #915
    * Require TensorFlow Probability to at least in the v0.10 range
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    bumpversion/patch Create a patch version release fix A bug fix
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    cloudpickle v1.5.0 breaks testing
    3 participants