test: Restrict tf dependency protobuf to ABI compatible releases

[matthewfeickert]

Description

protobuf v3.20.1 is the last release to support ABI compatibility with tensorflow pre tensorflow v2.9.1. The next release of protobuf, v4.21.0 (released 2022-05-25), breaks ABI compatibility and so requires an upper bound of protobuf v3.20.1 for older tensorflow releases to not break.

tensorflow released a series of patch releases in response to protobuf v4.21.0: v2.6.5, v2.7.3, v2.8.2, v2.9.1. As the oldest tensorflow that pyhf supports API compatibility with is tensorflow v2.3.1 add an upper bound of protobuf<=3.20.1 to ci/constraints.txt update the lower bound to tensorflow>=2.6.5.

c.f. tensorflow/tensorflow#56077

edit: Motivated by discussions below, this PR is now updating the lower bound on tensorflow to v2.6.5+. This is motivated primarily by the fact that this is significantly easier to maintain as a standard user is going to prefer a release of TensorFlow that works "out of the box" and doesn't require additional modification of the dependencies. The "oldest" (in the SemVer sense, not CalVer sense) release of TensorFlow that are not broken at install now is v2.6.5, so this will be the new lower bound. As

pyhf/setup.py

Line 7 in 0409ab5

'tensorflow-probability>=0.11.0', # c.f. PR #1657

is still compatible with tensorflow v2.6.5. this seems pretty uncontroversial in terms of an API jump.

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

* protobuf v3.20.1 is the last release to support ABI compatibility
with tensorflow pre tensorflow 2.9.1. The next release of protobuf,
v4.21.0, breaks ABI compatibility and so requires an upper bound of
protobuf v3.20.1 for older tensorflow releases to not break. tensorflow
released a series of patch releases in response to protobuf v4.21.0:
v2.6.5, v2.7.3, v2.8.2, v2.9.1.
   - c.f. https://github.com/tensorflow/tensorflow/issues/56077

[codecov]

Codecov Report

Merging #1869 (fdbb615) into master (031218c) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1869   +/-   ##
=======================================
  Coverage   98.17%   98.17%           
=======================================
  Files          68       68           
  Lines        4322     4322           
  Branches      726      726           
=======================================
  Hits         4243     4243           
  Misses         46       46           
  Partials       33       33           

Flag	Coverage Δ
contrib	26.39% <ø> (ø)
doctest	60.73% <ø> (ø)
unittests-3.10	96.04% <ø> (ø)
unittests-3.7	96.02% <ø> (ø)

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 031218c...fdbb615. Read the comment docs.

[matthewfeickert]

I think(?) it is okay to add this restriction to the constraints file over bumping the oldest supported tensorflow from

pyhf/setup.py

Lines 5 to 8 in 0409ab5

	'tensorflow': [
	'tensorflow>=2.3.1', # c.f. https://github.com/tensorflow/tensorflow/pull/40789
	'tensorflow-probability>=0.11.0', # c.f. PR #1657
	],

to be v2.6.5, but at the same time as tensorflow is only patching back to v2.6.5 then they're basically saying that as a project tensorflow v2.5.x and older are no longer supported. This part gives me a bit of pause as if the project itself is no longer supporting older releases I'm not sure we should be either.

Though this is where I'm also not sure if I'm trying to be too restrictive to users though, given @tacaswell's good advice RE: PR #1378 that we recorded in PR #1382 (and here again):

I think this is another library vs application difference. Your users are adults who are responsible for keeping their own house in order 😉 It is not a random libraries job to keep their systems up-to-date and secure (that falls to the end users and the packagers).

I think this is also a good case for why libraries should be as loose as possible with their version pinning as say other library was being super picky. Say some other library was being strict to some version range of pyyaml < 5.4 (for whatever reasons). Bumping your minimum to >= 5.4 with good intentions just drove someone to have unsatisfyable constraints.

Thoughts @kratsg @lukasheinrich @henryiii?

[tacaswell]

Practicality beats purity ;)

[matthewfeickert]

Motivated by discussion here and on the IRIS-HEP Slack, this PR is now updating the lower bound on tensorflow to v2.6.5+. This is motivated primarily by the fact that this is significantly easier to maintain as a standard user is going to prefer a release of TensorFlow that works "out of the box" and doesn't require additional modification of the dependencies. The "oldest" (in the SemVer sense, not CalVer sense) release of TensorFlow that are not broken at install now is v2.6.5, so this will be the new lower bound. As

pyhf/setup.py

Line 7 in 0409ab5

'tensorflow-probability>=0.11.0', # c.f. PR #1657

is still compatible with tensorflow v2.6.5. this seems pretty uncontroversial in terms of an API jump. 😬

---

Maintenance bonus: v2.6.5 is a big security release so I won't have to keep ignoring a huge number of CVE alerts from Dependabot because of TensorFlow anymore!

[matthewfeickert]

As there has been further discussion, for the time being to just get nightly CI passing I'm going back to the constraints.txt solution (which Giordon had already approved) and I'll open up a Discussion to see if there is a better general solution RE: ABI compatibility issues.