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

build: Set only lower bounds on backend dependencies #1698

Merged
merged 5 commits into from
Nov 16, 2021

Conversation

matthewfeickert
Copy link
Member

@matthewfeickert matthewfeickert commented Nov 15, 2021

Description

Set only lower bounds for all the pyhf backend extras. pyhf is testing daily the lower bounds, latest releases, and pre-releases, so these are empirically arrived at and tested. Note this is simply just enforcing the lower bounds already tested

$ grep 'tensor\|torch' lower-bound-requirements.txt 
# tensorflow
tensorflow==2.3.1  # tensorflow-probability v0.11.0 requires tensorflow>=2.3
tensorflow-probability==0.11.0  # c.f. PR #1657
# torch
torch==1.10.0

c.f. Henry's upcoming blog post for the deep dive on why this is optimal for libraries

Also add a note for the PR that caused these lower bounds to exist for easy reference for the core devs or the curious contributor.

Checklist Before Requesting Reviewer

  • Tests are passing
  • "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
* Set only lower bounds on the requirements for all backend extras.
   - This is done to avoid creating installation blocking scenarios
     and in situations where users find that the latest release of
     a dependency breaks pyhf code the user can be recommended to
     install the version of that dependency that was available when
     their version of pyhf was released.
* Add notes in setup.py pointing to the PR that caused the lower bound
to be needed for easy future reference

@matthewfeickert matthewfeickert added the build Changes that affect the build system or external dependencies label Nov 15, 2021
@matthewfeickert matthewfeickert self-assigned this Nov 15, 2021
@codecov
Copy link

codecov bot commented Nov 15, 2021

Codecov Report

Merging #1698 (c36087f) into master (d5b73c0) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1698   +/-   ##
=======================================
  Coverage   98.10%   98.10%           
=======================================
  Files          64       64           
  Lines        4226     4226           
  Branches      587      587           
=======================================
  Hits         4146     4146           
  Misses         46       46           
  Partials       34       34           
Flag Coverage Δ
contrib 25.41% <ø> (ø)
doctest 61.19% <ø> (ø)
unittests 96.42% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


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 d5b73c0...c36087f. Read the comment docs.

@matthewfeickert matthewfeickert merged commit 8c90450 into master Nov 16, 2021
@matthewfeickert matthewfeickert deleted the build/unconstraint-all-dependencies branch November 16, 2021 19:53
@matthewfeickert matthewfeickert added the dependencies Pull requests that update a dependency file label Nov 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Changes that affect the build system or external dependencies dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants