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

chore: trim CI set by default #2986

Merged
merged 20 commits into from
Nov 29, 2023
Merged

chore: trim CI set by default #2986

merged 20 commits into from
Nov 29, 2023

Conversation

nomeata
Copy link
Contributor

@nomeata nomeata commented Nov 29, 2023

The goal of this change is to run a trimmed-down CI on PRs by default, but allows opt-in the full CI as necessary.

Specification

The CI workflow runs in “quick” mode if it was triggered from a pull request, and that pull request does not have the full-ci label set.

In “quick” mode the build matrix contains fewer jobs. At the moment only:

  • Linux-release, to get the PR releases.

In non-quick mode everything should be as before.

Implementation notes

I created a configure job that combines all the previous set- jobs, I guess this is faster than firing up separate jobs.

The matrix is calculated in this job; this seems to be the cleanest way to get a dynamic matrix going (experiments using exclude failed). The downside is that the matrix is now in JSON rather than Yaml syntax. The upside is that we can (later) make it’s calculation simpler, e.g. set default shell values etc.

I was not able to make it so that CI runs when the full-ci label is added, but don’t do anything otherwise. I think it can be done with another workflow listening to labeled and then triggering this one, but let’s do that separately. For now, add the label and then push (or close and reopen).

The checks

  if: matrix.build-stage2 || matrix.check-stage3
  if: matrix.check-stage3

were dead code, we did not have these fields in the matrix anymore, so I replaced them with

  if: matrix.test-speedcenter

@nomeata nomeata marked this pull request as ready for review November 29, 2023 10:08
@nomeata nomeata added the awaiting-review Waiting for someone to review the PR label Nov 29, 2023
@Kha
Copy link
Member

Kha commented Nov 29, 2023

Linux, to get the speed center data

Note that the CI step only tests that the benchmarks still work, it is not a part of the !bench pipeline. So I think we can leave that check to the merge queue.

The downside is that the matrix is now in JSON rather than Yaml syntax.

One upside is that we could now actually factor out common configuration :) ? But all the comments are gone :( ?

@nomeata
Copy link
Contributor Author

nomeata commented Nov 29, 2023

Note that the CI step only tests that the benchmarks still work, it is not a part of the !bench pipeline. So I think we can leave that check to the merge queue.

Ah, great! Removed from quick

@nomeata
Copy link
Contributor Author

nomeata commented Nov 29, 2023

The downside is that the matrix is now in JSON rather than Yaml syntax.

One upside is that we could now actually factor out common configuration :) ?

That’s what I meant to say :-)

But all the comments are gone :( ?

Ups. While converting I thought “I need to put comments back in”, and then thought got lost… will fix … fixed.

@Kha Kha changed the title chore: Trim CI set by default chore: trim CI set by default Nov 29, 2023
@github-actions github-actions bot added the toolchain-available A toolchain is available for this PR, at leanprover/lean4-pr-releases:pr-release-NNNN label Nov 29, 2023
@leanprover-community-mathlib4-bot
Copy link
Collaborator

leanprover-community-mathlib4-bot commented Nov 29, 2023

  • ❗ Mathlib CI can not be attempted yet, as the 'nightly-testing-2023-11-29' branch does not exist there yet. We will retry when you push more commits. It may be necessary to rebase onto 'nightly' tomorrow. (2023-11-29 10:58:51)
  • ✅ Mathlib branch lean-pr-testing-2986 has successfully built against this PR. (2023-11-29 12:45:54) View Log
  • ❗ Mathlib CI will not be attempted unless you rebase your PR onto the 'nightly' branch. (2023-11-29 13:27:24)

@nomeata
Copy link
Contributor Author

nomeata commented Nov 29, 2023

As expected the PR release only has linux:
https://github.com/leanprover/lean4-pr-releases/releases/tag/pr-release-2986

I’ll rebase this on nightly-with-mathlib to see if the mathlib test job works as expected.

leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Nov 29, 2023
@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added the builds-mathlib CI has verified that Mathlib builds against this PR label Nov 29, 2023
@nomeata
Copy link
Contributor Author

nomeata commented Nov 29, 2023

Ok, mathlib builds. Good to go, I guess…

@nomeata nomeata added this pull request to the merge queue Nov 29, 2023
Merged via the queue into master with commit 367ac01 Nov 29, 2023
7 checks passed
nomeata added a commit that referenced this pull request Nov 30, 2023
Following up on #2986, stop running the test suite in ci.yml in quick
mode; the test suite is run in the Nix job, and we do not need to run it
twice.

With a cold nix cache, when `lean` is rebuilt, not much changes, as both
jobs take ~20mins. But when `lean` is unchanged, the nix build should
be faster, and shaving off the (currently) 4mins in the CI.yaml run
should get us to a green PR sooner.

Another benefit is that we get the PR release sooner and even get it
when the test suite fails, which can be useful if you want to test
mathlib or other things before fixing the lean test suite.
github-merge-queue bot pushed a commit that referenced this pull request Nov 30, 2023
Following up on #2986, stop running the test suite in ci.yml in quick
mode; the test suite is run in the Nix job, and we do not need to run it
twice.

With a cold nix cache, when `lean` is rebuilt, not much changes, as both
jobs take ~20mins. But when `lean` is unchanged, the nix build should
be faster, and shaving off the (currently) 4mins in the CI.yaml run
should get us to a green PR sooner.

Another benefit is that we get the PR release sooner and even get it
when the test suite fails, which can be useful if you want to test
mathlib or other things before fixing the lean test suite.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-review Waiting for someone to review the PR builds-mathlib CI has verified that Mathlib builds against this PR toolchain-available A toolchain is available for this PR, at leanprover/lean4-pr-releases:pr-release-NNNN
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants