Cirq icon indicating copy to clipboard operation
Cirq copied to clipboard

Relax install_requires constraints

Open balopat opened this issue 5 years ago • 6 comments

Based on https://packaging.python.org/discussions/install-requires-vs-requirements/ "It is not considered best practice to use install_requires to pin dependencies to specific versions".

We need to loosen up install_requires while keeping the CI stable. We don't have good, documented reasons for the lower/upper boundaries. Maybe we could just pin to major versions and let the minors fly free? And then, for CI we could setup a bot that checks for updates weekly and raises a PR with an updated minor version for each requirement (with pur: https://pypi.org/project/pur/) to make sure we're testing against the latest available versions.

Context:

  • https://github.com/quantumlib/Cirq/pull/2648
  • https://github.com/quantumlib/Cirq/pull/3551
  • https://github.com/quantumlib/OpenFermion-Cirq/pull/352#issuecomment-513044359

balopat avatar Nov 30 '20 23:11 balopat

Thanks for raising this issue. For some things I think it makes more sense to test against the oldest supported version (e.g. want to make sure we don't require new features) while for other things it's nice to test against newer versions (e.g. want to test that we take advantage of new features if available). Since it's infeasible to run tests against the entire matrix of possible versions, one possibility would be to run tests twice, once against the oldest supported versions of all deps, and once against the newest supported versions. Clearly, this argues for keeping the list of requirements as small as possible; the packaging effort should help with that.

maffoo avatar Nov 30 '20 23:11 maffoo

@maffoo , your suggestion is a good one; but that speaks to what we should put in requirements.txt; i.e. how we set up the test environment(s).

I think @balopat is asking about what to put in install_requires. I'd naively prefer using only >= to set minimum versions of things and blacklist versions we know don't work. In my (perhaps naive!) view of the world, one of our small number of stable dependencies will only rarely break something in cirq. Then we'd have to push out a quick point release to exclude the problematic version. Pinning to major versions would produce more-or-less the same effect. Unless one of our dependencies is very strict about semantic versioning and releases major versions (that are largely compatible, or the api surface area used by cirq is compatible) or one of our dependencies is highly stable but refuses to move to version 1.0 like scipy of yore. Then we might cause users to have incompatible environments and we may be less responsive to pushing out a quick point release to certify that yes, the new version is fine.

Edit: If you want an example of downstream devs wishing for looser pins, look no further than https://github.com/CQCL/pytket/issues/32. Which raises an additional issue: it looks like the new resolver has no problem searching back in time. So if we make a bad pin, it may haunt us for years to come

mpharrigan avatar Dec 01 '20 00:12 mpharrigan

If a downstream user wants to use a newer version of e.g. networkx with a current or older version of cirq, things could get messy. In the olden days (i.e. right now) the pip dependency resolver would let you override some of the version specs by pinning things. But starting soon it will error out and fail to install. Their suggestion is to fork the dependency to loosen the install_requires field if you run into this (!) https://pip.pypa.io/en/latest/user_guide/#loosen-the-requirements-of-your-dependencies

mpharrigan avatar Dec 01 '20 00:12 mpharrigan

The way python devs are going at this is convoluted to say the least... I cannot believe that they pulled the switch on the new dependency resolution "feature" without allowing for a reasonable override method... forking the dependency and loosen the requirements as the standard solution is... questionable. Having said that, to fix the current blocking issue, having >= + blacklist sounds good to me.

m-szalay avatar Dec 01 '20 02:12 m-szalay

I found a major pain with the requirements.txt pinning.

Ideally the current requirements.txt files should be renamed to requirements.in files, which then can be used by setup.py as they are today - maybe loosen them up a little bit more where we can. Then we could use a tool like pip-compile which would compile these constraints to the actual latest versions of the dependencies recursively. Then using these versions we could have a reproducible CI build - no more 10 PRs failing at the same time because a dependency 5 level deep upgraded and broke something. We can then create a CI job that checks weekly the dependency graph and raises a PR if there are updates in the requirements.txt files. Great!

The catch is that the exact dependency graph depends on not just time (which determines the latest versions), but also the python version and the operating system. When you do this "on the fly", you get this "ambiguity" calculated "free" by pip install - with a pip-compile direction we'd have to recalculate for each platform and each python version supported (3 x 4 = 12 in the worst case, 9 in the best)! That's 9 times the requirements.txt files that we'd have to manage. If we include minimum and maximum versions of the graph (of which the minimum calculation is actually hard - haven't find a tool that can do that yet, probably we'd have to maintain it manually) - then it's 18 different requirements.txts generated from a single requirements.in file.

In particular when I tried to do this I got all sorts of failures on python 3.6 builds for a requirements.txt compiled locally on my mac against py3.8.

The other aspect of this is install_requires will actually work in this "flexible" way anyway. Which means, that we might live in our nice world of controlled dependency upgrades, but our users won't.

Maybe I'm missing something, but to me it seems like taking on the effort to compile to all possible elements of our matrix is very tedious, and is not worthwhile just to avoid the - typically rare - dependency related breakage of our PRs.

balopat avatar May 26 '21 00:05 balopat

Can we just freeze depth=1 or depth=2 of our dependency graph? I guess it technically wouldn't be a reproducible build but it's more than what we have now

mpharrigan avatar May 26 '21 22:05 mpharrigan