Revamp CI: matrix, multi-Cirq

quantumlib / OpenFermion

The electronic structure package for quantum computers.

Apache License 2.0

1.51k stars 372 forks source link

Revamp CI: matrix, multi-Cirq #755

Closed mpharrigan closed 2 years ago

mpharrigan commented 2 years ago

Use CI "matrix" to reduce duplication in testing infrastructure
stop pinning cirq in install_requires. Lower bound only. More often than not, we are artificially restricting dependers from using the latest Cirq than preventing real problems. Testing against pre-release should prevent surprises.
Test 1) lowest supported version of things 2) latest version of things 3) vs. cirq pre-release
Clarify and bump supported python version: latest two versions
Clarify and bump supported cirq version: latest two versions (unless this is difficult to maintain, then we'd just support the latest version and drop it from the matrix)

MichaelBroughton commented 2 years ago

So in order to merge a PR now, one must write working changes for OpenFermion that span Cirq ~= 0.12, Cirq 0.13.0 and the latest Cirq-Dev ?

mpharrigan commented 2 years ago

If there's a true need for a recent Cirq version, that would be made explicit by bumping the min version in install_requies and removing that version from the testing matrix.

In practice, openfermion has rarely been in this situation as of late.

MichaelBroughton commented 2 years ago

What about the case of a breaking change between cirq 0.13 and 0.14-dev ?

mpharrigan commented 2 years ago

we should fix openfermion to not break with 0.14-dev?

mpharrigan commented 2 years ago

@ncrubin what do you think

ncrubin commented 2 years ago

I agree with Matt. if issues come up we can resolve and bump min version.

mpharrigan commented 2 years ago

Since this changes the names of expected checks, we need to configure the repository settings or it will keep waiting for the old ci job names

mpharrigan commented 2 years ago

Actually I guess TFQ has next-cirq running in a nightly build. This is better, as it won't hold up unrelated PRs (but perhaps is more easily ignored?)

mpharrigan commented 2 years ago

@ncrubin @MichaelBroughton my push dismissed your LGTMs. Please re-approve. I'll force-merge and then edit the list of required checks to match the new name. When we bump Cirq or Python it will change the name of the matrix build and we (I) will have to change the list of required checks. If this is too onerous, there's a clunky workaround (where you make one final job that depends on the matrix build job succeeding), but I don't think that will be necessary.

mpharrigan commented 2 years ago

bump @MichaelBroughton @ncrubin

mpharrigan commented 2 years ago

I force merged this and I've updated the required test for future PRs. Please ping me if there are issues with any future PRs