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

Rework Actions to work well with Merge Queues and Enable Benchmarks in there #525

Merged
merged 15 commits into from
Aug 29, 2024

Conversation

keks
Copy link
Member

@keks keks commented Aug 22, 2024

This PR reworks the actions a little bit to work well with merge queues, and running the benchmarks in the merge queues. Since this is difficult to test, this PR will probably see some more commits :)

In order to get the structure, let's go over the way the rules work. There is a list of required checks, which is just a list of strings. Github now requires that at least one job of that name ran, and that all jobs of that name succeeded. It does not care whether these are actually the same jobs.

There now are three classes of workflow files:

  • Sporadically run: These do not run automatically, but only when triggered manually. e.g.: flake-update.yml
  • PR jobs: These are run on every PR. From now on, they also will run in merge queues. This includes stuff like running tests
  • Merge-Queue-Only: This is mostly benchmarks.

So far the problem was that in the merge queue, github only runs the jobs that have a name that is listed in the required checks list. This makes it difficult to run something only in the merge queue, because it would then also run in the PRs, or the PR checks wouldn't go because it's blocked on a check that won't run. But remember: github doesn't care whether these are the same checks! It only cares that there are checks that have the right name, and that the are successful, even if there is just one check of that name.

We only use the on setting of the workflow to configure which events trigger the jobs therein.

We now do the following:

  • we move the benchmark jobs from the PR jobs to the merge queue jobs
  • the merge queue jobs are on: [ merge_group, workflow_dispatch ]
  • we trigger PR jobs in merge queues as well and remove all the checks of event types
  • we add a PR-only job that has a job with name "benchmark" that does nothing

This way, the PR checks are satisfied, and when in the merge queue, it runs the benchmarks.

Oh, and this PR also adds the benchmark html generation step.

Working towards #54.

Implementation note: We have to add

[lib]
bench = false

to the Cargo.toml of all the crates with criterion benchmarks that we want to track in a graph. The reason is that the graph wants the benchmarks in a special format and we have to pass --output-format bencher. However, by default cargo bench only accepts command line arguments that libtest also accepts. It seems the only way is to ask libtest to not handle benchmarks. Cf. the criterion faq

Copy link
Member

@franziskuskiefer franziskuskiefer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is doing what it's supposed to.
Though, I have been thinking that before as well 😉
Let's give it a try!

Cargo.toml Outdated Show resolved Hide resolved
.github/workflows/rust-bench.yml Outdated Show resolved Hide resolved
@keks keks added this pull request to the merge queue Aug 22, 2024
@keks keks removed this pull request from the merge queue due to a manual request Aug 22, 2024
@keks keks enabled auto-merge August 22, 2024 17:49
@keks keks added this pull request to the merge queue Aug 22, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Aug 22, 2024
@keks keks added this pull request to the merge queue Aug 26, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Aug 26, 2024
@keks keks enabled auto-merge August 26, 2024 12:11
@keks keks added this pull request to the merge queue Aug 26, 2024
@keks keks removed this pull request from the merge queue due to a manual request Aug 26, 2024
keks and others added 2 commits August 26, 2024 16:21
had to move the utils into the crate, because otherwise it would be
picked up by the bechmarks and fail because libtest would again complain
about the --output-format argument
@keks keks enabled auto-merge August 26, 2024 14:21
@keks keks disabled auto-merge August 26, 2024 14:31
@keks keks added this pull request to the merge queue Aug 26, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 26, 2024
@keks keks enabled auto-merge August 26, 2024 15:50
@keks keks added this pull request to the merge queue Aug 26, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 26, 2024
@keks keks enabled auto-merge August 27, 2024 07:59
@keks keks added this pull request to the merge queue Aug 27, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 27, 2024
@keks keks added this pull request to the merge queue Aug 27, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 27, 2024
@franziskuskiefer franziskuskiefer mentioned this pull request Aug 28, 2024
14 tasks
@keks keks enabled auto-merge August 29, 2024 09:05
@keks keks added this pull request to the merge queue Aug 29, 2024
Merged via the queue into main with commit 9915bee Aug 29, 2024
48 checks passed
@keks keks deleted the keks/towards-merge-queue-and-bench-graphs branch August 29, 2024 10:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants