Proper response for testing lower bound on TensorFlow following protobuf ABI breaking change

[matthewfeickert]

I'm hoping to have a bit of a community discussion RE: how I tried to respond in PR #1869 to protobuf v4.21.0 (now yanked — which is interesting) having ABI breaking changes that caused all releases of TensorFlow except for emergency patch releases v2.6.5, v2.7.3, v2.8.2, and v2.9.1 (which all cap the upper bound on protobuf to be 'protobuf >= 3.9.2, < 3.2') to be broken at install (c.f. tensorflow/tensorflow#56077).

In pyhf we empirically test all of the lower bounds in setup.cfg and setup.py with a nightly CI workflow that uses a constraints.txt file to install the lower bound of all dependencies as we have defined them. Before protobuf v4.21.0 was released we had

pyhf/tests/constraints.txt

Lines 13 to 15 in 031218c

	# tensorflow
	tensorflow==2.3.1 # tensorflow-probability v0.11.0 requires tensorflow>=2.3
	tensorflow-probability==0.11.0 # c.f. PR #1657

however, as a project TensorFlow only ported patch releases back to v2.6.x (c.f. Fixes are not always backported section of @henryiii's Should You Use Upper Bound Version Constraints? blog), and so tensorflow v2.3.1 would be broken on install. We had two options:

1. As "users" of TensorFlow for the TensorFlow pyhf backend, add an upper bound on protobuf in the constraints.txt file to keep tensorflow v2.3.1 from breaking on install.
2. Follow TensorFlow's example and drop support for TensorFlow version "older" than v2.6.x and set the lower bound on tensorflow in the tensorflow extra in setup.py to be tensorflow>=2.6.5.

This was PR #1869. We ended up going with option 1 after some discussion mainly because it would only affect the pyhf lower bound test suite and not make any changes for any future users.

pyhf/tests/constraints.txt

Lines 13 to 16 in ea3249a

	# tensorflow
	tensorflow==2.3.1 # tensorflow-probability v0.11.0 requires tensorflow>=2.3
	protobuf<=3.20.1 # protobuf v4.21.0 breaks ABI compatability
	tensorflow-probability==0.11.0 # c.f. PR #1657

I can see arguments for both options 1 and 2 though, especially as now that TensorFlow has basically dropped support for anything pre-v2.6.x there is extra maintenance burden to support v2.3.1. pyhf does not have anything that breaks the v2.3.x API though, so there is no hard requirement from our useage of tensorflow or tensorflow_probability to make a change for API compatability. Though the It conflicts with tight lower bounds section of the blog seems to indicate that we wouldn't be the most terrible project if we kept tighter lower bounds then we currently do.

I'm curious on what people in the Scikit-HEP community think is the appropriate thing to do here. If this was a project that you maintained, what actions would you take?

Things to note:

• We do not think that the TensorFlow pyhf backend is used extensively (compared to the JAX or PyTorch backends) so this may be more of an exercise in trying to adhere to best practices when it doesn't matter more than a situation in which we would actually affect pyhf users very much.
• protobuf actually gives some pretty good instructions to users. Here's what happens when you install tensorflow v2.3.1 and (I have to force it as the release was yanked) the ABI incompatible protobuf v4.21.0.

$ docker run --rm -ti python:3.7 /bin/bash
root@c959479d92fc:/# python -m venv venv && . venv/bin/activate
(venv) root@c959479d92fc:/# python --version --version
Python 3.7.13 (default, May 11 2022, 08:49:57) 
[GCC 10.2.1 20210110]
(venv) root@c959479d92fc:/# python -m pip --quiet install --upgrade pip setuptools wheel
(venv) root@c959479d92fc:/# python -m pip --quiet install 'tensorflow==2.3.1' 'protobuf==4.21.0'
WARNING: The candidate selected for download or install is a yanked version: 'protobuf' candidate (version 4.21.0 at https://files.pythonhosted.org/packages/9d/82/b3131637daf2a27eab76b0de8e139ecf0f6624832c03531dce8a7d59ddc1/protobuf-4.21.0-cp37-abi3-manylinux2014_x86_64.whl#sha256=9a65e012bc06022e98a57165ea48438b3b9f652eee33db7cbecb883588f9f169 (from https://pypi.org/simple/protobuf/))
Reason for being yanked: Compatibility issue with cloud client libraries
(venv) root@c959479d92fc:/# python -m pip list | grep 'tensorflow\|protobuf'
protobuf                4.21.0
tensorflow              2.3.1
tensorflow-estimator    2.3.0
(venv) root@c959479d92fc:/# python -c 'import tensorflow'
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/venv/lib/python3.7/site-packages/tensorflow/__init__.py", line 41, in <module>
    from tensorflow.python.tools import module_util as _module_util
  File "/venv/lib/python3.7/site-packages/tensorflow/python/__init__.py", line 40, in <module>
    from tensorflow.python.eager import context
  File "/venv/lib/python3.7/site-packages/tensorflow/python/eager/context.py", line 32, in <module>
    from tensorflow.core.framework import function_pb2
  File "/venv/lib/python3.7/site-packages/tensorflow/core/framework/function_pb2.py", line 16, in <module>
    from tensorflow.core.framework import attr_value_pb2 as tensorflow_dot_core_dot_framework_dot_attr__value__pb2
  File "/venv/lib/python3.7/site-packages/tensorflow/core/framework/attr_value_pb2.py", line 16, in <module>
    from tensorflow.core.framework import tensor_pb2 as tensorflow_dot_core_dot_framework_dot_tensor__pb2
  File "/venv/lib/python3.7/site-packages/tensorflow/core/framework/tensor_pb2.py", line 16, in <module>
    from tensorflow.core.framework import resource_handle_pb2 as tensorflow_dot_core_dot_framework_dot_resource__handle__pb2
  File "/venv/lib/python3.7/site-packages/tensorflow/core/framework/resource_handle_pb2.py", line 16, in <module>
    from tensorflow.core.framework import tensor_shape_pb2 as tensorflow_dot_core_dot_framework_dot_tensor__shape__pb2
  File "/venv/lib/python3.7/site-packages/tensorflow/core/framework/tensor_shape_pb2.py", line 42, in <module>
    serialized_options=None, file=DESCRIPTOR),
  File "/venv/lib/python3.7/site-packages/google/protobuf/descriptor.py", line 560, in __new__
    _message.Message._CheckCalledFromGeneratedFile()
TypeError: Descriptors cannot not be created directly.
If this call came from a _pb2.py file, your generated code is out of date and must be regenerated with protoc >= 3.19.0.
If you cannot immediately regenerate your protos, some other possible workarounds are:
 1. Downgrade the protobuf package to 3.20.x or lower.
 2. Set PROTOCOL_BUFFERS_PYTHON_IMPLEMENTATION=python (but this will use pure-Python parsing and will be much slower).

More information: https://developers.google.com/protocol-buffers/docs/news/2022-05-06#python-updates
(venv) root@c959479d92fc:/# 

cc @kratsg @lukasheinrich @alexander-held @henryiii @agoose77 @jpivarski

[jonas-eschle]

That's a good topic for thoughts! My take on this would tend to go for option 2, mainly to reduce technical debt.

pyhf isn't broken

While it may sounds a bit strict as an argument, the problem clearly lies with TensorFlow and protobuf, not with pyhf. Strictly speaking, pyhf makes sure that pyhf runs with TensorFlow, but installing a running version of TensorFlow is not directly pyhf's responsibility.
This, of course, cannot be left alone as we want to improve the user experience and a smooth installation is in pyhf's interest! But it means that it's a problem any library faces that uses TensorFlow and that has been already tackled on the TF level with a good error message.
But, in general, as a user, if I install a library A and a dependency B doesn't install correctly, I don't expect that library A helps me to install library B except if explicit guarantees have been given (see also below "For the pyhf 1.x version")

general argument on version

pyhf is in the 0.x development phase. This means it is not stable. It means that there are potentially bugs around that forces users to upgrade and "dropping" older versions seems like a reasonable choice. It's like saying that an older version had a major bug and cannot be used.
It could even be reasonable to simply check the TF version on runtime, if it's below 2.6, error and explain the reasoning. If upgrading is not possible, they may could contact you (I don't assume that anyone will do that then). Also, TF does this quite well in the error message already, so that may even suffice.

different backends

As pyhf supports many backends, users can not only upgrade TF (which anyways should be considered), but also use a different backends. This makes choosing an alternative backend and therefore avoiding the problem a lot easier.

TF supported versions

The actual question, IMHO, is whether one should support versions of libraries that are deemed unsupported by the library itself, as is the case for TF<2.6. I would say that the move of TF not to backport to older versions was somewhat foreseeable, as the backends, JAX and TF especially, are still undergoing changes (in zfit, using a lot more advanced features, we're even forced to clamp to 1 or 2 minor versions due to compatibility issues). Therefore, dropping explicit (as in "making sure they work flawlessly") support for older, unsupported backend versions may rather be considered the default overall.

For the pyhf 1.x version

For the stable release (but also for now), I would suggest to adapt a clear policy on this, i.e. that all the versions are supported of a backend that are supported by the library itself and mention it in a "backend" section.

[henryiii]

Good points and ideas so far, just to throw in a few random thoughts:

Someone stuck on an older version of anything is going to have to workaround issues if they don't fully pin, full stop. If you cap anything that is no longer supported, you are signing up to potentially run into issues, bugs, security issues, etc. So if someone is capping tf<2.6, they already are signing up for this issue; it's not PyHF's fault. If they are not capping tf<2.6, why are they running into this at all?¹

Second, placing a minimum here in PyHF will do nothing other than install an old version of PyHF to make this solve. You're not fixing the problem (forcing an tensorflow upgrade) if they've capped tensorflow, and it's not your problem, it's tensorflow's, so getting an old version of your library won't help. (Again, assuming no "upgrade one package in place" scenario here).

If the cap is due to some other library, again, the solver will just pick an older PyHF.

Anyway, on the other side, I'd generally argue that you should support only supported versions. It's a lot of extra technical debt to support unsupported versions of anything, and convincing a user to upgrade is likely easier than maintaining old support, and crippling the new features and optimizations you could otherwise add. Someone can always select an older PyHF if they are on an older tensorflow. Someone okay with an old tensorflow / python / etc should be okay with an older pyhf.

Footnotes

1. If someone is manually upgrading just pyhf and not tensorflow in an existing environment, maybe they would hit that. But this is already a bad practice unless they are using an environment file based system (pdm, poetry, environment.yml), where this also won't be a problem (everything gets upgraded in those). ↩