CI: Run benchmarks if PR is labeled with "run/benchmark" by seisman · Pull Request #2958 · GenericMappingTools/pygmt

seisman · 2024-01-06T09:01:08Z

Support running benchmarks in PRs with the run/benchmark label.

Address comment #2952 (comment).

seisman · 2024-01-06T09:12:51Z

/bechmark

seisman · 2024-01-06T09:15:09Z

The slash command won't work until the PR is merged into main branch.

weiji14 · 2024-01-06T09:44:26Z

The slash command won't work until the PR is merged into main branch.

Actually, will CodSpeed be able to link the benchmark report to the PR branch if we trigger it via repository_dispatch instead of pull_request?

seisman · 2024-01-06T09:52:58Z

The slash command won't work until the PR is merged into main branch.

Actually, will CodSpeed be able to link the benchmark report to the PR branch if we trigger it via repository_dispatch instead of pull_request?

That's a good question. I guess the answer is no.

weiji14 · 2024-01-06T10:25:01Z

Actually, will CodSpeed be able to link the benchmark report to the PR branch if we trigger it via repository_dispatch instead of pull_request?

That's a good question. I guess the answer is no.

How about we set up the workflow to run only when there's a run-benchmark label? Can try to set up an if-condition following https://stackoverflow.com/questions/62325286/run-github-actions-when-pull-requests-have-a-specific-label

This reverts commit cbb94f4.

seisman · 2024-01-06T10:36:14Z

Actually, will CodSpeed be able to link the benchmark report to the PR branch if we trigger it via repository_dispatch instead of pull_request?

That's a good question. I guess the answer is no.

How about we set up the workflow to run only when there's a run-benchmark label? Can try to set up an if-condition following https://stackoverflow.com/questions/62325286/run-github-actions-when-pull-requests-have-a-specific-label

Sounds a clever solution.

seisman · 2024-01-06T10:51:40Z

The benchmark workflow is trigged by the "run-benchmark" label, see https://github.com/GenericMappingTools/pygmt/actions/runs/7431110786?pr=2958.

seisman · 2024-01-06T10:56:06Z

I feel triggering a workflow based on the PR label is much better than the slash commands. Some thoughts:

Migrate the other two slash commands /format and /test-gmt-dev to triggering by labels
Instead of using labels like run-benchmark, maybe labels run/benchmark, run/format, run/test-gmt-dev are better?

seisman · 2024-01-06T10:57:31Z

Hmm, the workflow is canceled because I added the 'maintenance' label.

weiji14 · 2024-01-06T11:20:40Z

Migrate the other two slash commands /format and /test-gmt-dev to triggering by labels

Yeah, I was thinking of using it for /test-gmt-dev, but maybe not for /format since the labels can only be added by contributors/maintainers with triage permissions or above, so new contributors won't be able to use it.

Instead of using labels like run-benchmark, maybe labels run/benchmark, run/format, run/test-gmt-dev are better?

Sounds good.

seisman · 2024-01-06T12:36:43Z

.github/workflows/benchmarks.yml

  benchmarks:
    runs-on: ubuntu-22.04
-    if: github.repository == 'GenericMappingTools/pygmt'
+    if: github.repository == 'GenericMappingTools/pygmt' && contains(github.event.pull_request.labels.*.name, 'run/benchmark')


Actually I'm unsure if the workflow will run in the main branch, because it's a push event not a pull_request event.

Tried in my own fork. It doesn't work in the main branch.

Changed the if condition to:

if: github.repository == 'GenericMappingTools/pygmt' && (github.event_name != 'pull_request' || contains(github.event.pull_request.labels.*.name, 'run/benchmark'))

Now it works in my own fork, see the changes between my fork and the pygmt repository: main...seisman:pygmt:main and the workflow was triggered when pushing in the main branch (https://github.com/seisman/pygmt/actions/workflows/benchmarks.yml).

seisman · 2024-01-07T04:17:49Z

It's unclear why I can't see the flame graphs.

weiji14

It's unclear why I can't see the flame graphs.

Hmm yeah, are you getting this Execution profile not available message too at https://codspeed.io/GenericMappingTools/pygmt/branches/slash-benchmark?

If I try another branch (e.g. https://codspeed.io/GenericMappingTools/pygmt/branches/alias/function), it looks ok, though sometimes the flame graphs can take some time to render.

.github/workflows/benchmarks.yml

weiji14 · 2024-01-07T07:06:34Z

.github/workflows/benchmarks.yml

      - '.github/workflows/benchmarks.yml'
-  # Uncomment the 'pull_request' line below to trigger the workflow in PR
-  # pull_request:
+  # Run in PRs but only if the PR has the 'run/benchmark' label


Should we document in .github/PULL_REQUEST_TEMPLATE.md or somewhere for contributors to request that the run/benchmark label gets added to PRs that might affect performance? Or just rely on the maintainers to set the label for now?

I prefer to leave the jobs to the maintainers. We probably will document it in the contributors guides when addressing #1113.

Ok, or we can put it in doc/maintenance.md somewhere. Not urgent though, since we aren't running the benchmarks that often.

Co-authored-by: Wei Ji <23487320+weiji14@users.noreply.github.com>

weiji14

Thanks @seisman! Have secretly wanted this label-based trigger mechanism in PyGMT for a while now to be honest 😀

weiji14 · 2024-01-07T08:15:50Z

Quick question: Do we want to remove the run/benchmark label after a PR is merged, or keep the label on?

seisman · 2024-01-07T08:23:53Z

I'm OK with either. Maybe we should keep the label so that we know which branches have performance reports.

seisman · 2024-01-07T08:29:02Z

Quick question: Do we want to remove the run/benchmark label after a PR is merged, or keep the label on?

Actually, we should remove the 'run/benchmark' label. We usually label a PR as "needs-review", then "final-review-call" and remove the "final-review-call" label before merging. All these actions will trigger the benchmarks workflow if the run/benchmark label is kept.

weiji14 · 2024-01-07T08:36:48Z

Quick question: Do we want to remove the run/benchmark label after a PR is merged, or keep the label on?

Actually, we should remove the 'run/benchmark' label. We usually label a PR as "needs-review", then "final-review-call" and remove the "final-review-call" label before merging. All these actions will trigger the benchmarks workflow if the run/benchmark label is kept.

Owh, but we would need to remove all those labels at the same time right? Maybe better to set an if-condition so that the benchmark workflow is not re-triggered after a PR is already merged?

seisman · 2024-01-07T08:46:49Z

Owh, but we would need to remove all those labels at the same time right?

We need to remove run/benchmark label as early as possible to avoid unnecessary runs.

Maybe better to set an if-condition so that the benchmark workflow is not re-triggered after a PR is already merged?

It may make the if-condition too complicated, which is already very long.

weiji14 · 2024-01-07T08:53:31Z

Owh, but we would need to remove all those labels at the same time right?

We need to remove run/benchmark label as early as possible to avoid unnecessary runs.

Ok, let's go with this. Remove the run/benchmark label before the other labels are removed. Same goes for the run/test-gmt-dev label.

…)" This reverts commit ecefd53.

Support slash command /benchmark in PRs

cbb94f4

seisman added 2 commits January 6, 2024 18:33

Revert "Support slash command /benchmark in PRs"

1c99d10

This reverts commit cbb94f4.

Run benchmarks if labeled with 'run-benchmark'

17c7bbe

seisman added the run/benchmark Trigger the benchmark workflow in PRs label Jan 6, 2024

seisman changed the title ~~Support slash command /benchmark in PRs~~ CI: Run benchmarks if PR is labeled with "run-benchmark" Jan 6, 2024

seisman added the maintenance Boring but important stuff for the core devs label Jan 6, 2024

seisman added this to the 0.11.0 milestone Jan 6, 2024

seisman added 2 commits January 6, 2024 19:04

Use the 'contains' syntax

328453c

Merge branch 'main' into slash-benchmark

5abc3a9

Change the label name to 'run/benchmark'

553e691

seisman added run/benchmark Trigger the benchmark workflow in PRs and removed run/benchmark Trigger the benchmark workflow in PRs labels Jan 6, 2024

seisman changed the title ~~CI: Run benchmarks if PR is labeled with "run-benchmark"~~ CI: Run benchmarks if PR is labeled with "run/benchmark" Jan 6, 2024

seisman marked this pull request as ready for review January 6, 2024 12:04

seisman commented Jan 6, 2024

View reviewed changes

seisman marked this pull request as draft January 6, 2024 15:21

Improve the job condition

6d42078

seisman marked this pull request as ready for review January 6, 2024 15:42

seisman mentioned this pull request Jan 6, 2024

CI: Run "GMT Dev Tests" if PR is labeled with 'run/test-gmt-dev' #2960

Merged

Run the workflow when labeled

0d0e17f

Run the workflow when synchronized

ada3aa5

seisman added the needs review This PR has higher priority and needs review. label Jan 7, 2024

weiji14 reviewed Jan 7, 2024

View reviewed changes

Update .github/workflows/benchmarks.yml

d59efb1

Co-authored-by: Wei Ji <23487320+weiji14@users.noreply.github.com>

weiji14 approved these changes Jan 7, 2024

View reviewed changes

seisman merged commit ecefd53 into main Jan 7, 2024

seisman deleted the slash-benchmark branch January 7, 2024 08:12

seisman removed needs review This PR has higher priority and needs review. run/benchmark Trigger the benchmark workflow in PRs labels Jan 7, 2024

seisman added the run/benchmark Trigger the benchmark workflow in PRs label Jan 7, 2024

seisman added a commit that referenced this pull request Mar 21, 2024

Revert "CI: Run benchmarks if PR is labeled with "run/benchmark" (#2958…

3464e48

…)" This reverts commit ecefd53.

Conversation

seisman commented Jan 6, 2024 • edited by weiji14 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

seisman commented Jan 6, 2024

Uh oh!

seisman commented Jan 6, 2024

Uh oh!

weiji14 commented Jan 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

seisman commented Jan 6, 2024

Uh oh!

weiji14 commented Jan 6, 2024

Uh oh!

seisman commented Jan 6, 2024

Uh oh!

seisman commented Jan 6, 2024

Uh oh!

seisman commented Jan 6, 2024

Uh oh!

seisman commented Jan 6, 2024

Uh oh!

weiji14 commented Jan 6, 2024

Uh oh!

seisman Jan 6, 2024

Choose a reason for hiding this comment

Uh oh!

seisman Jan 6, 2024

Choose a reason for hiding this comment

Uh oh!

seisman Jan 6, 2024

Choose a reason for hiding this comment

Uh oh!

seisman commented Jan 7, 2024

Uh oh!

weiji14 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

weiji14 Jan 7, 2024

Choose a reason for hiding this comment

Uh oh!

seisman Jan 7, 2024

Choose a reason for hiding this comment

Uh oh!

weiji14 Jan 7, 2024

Choose a reason for hiding this comment

Uh oh!

weiji14 left a comment

Choose a reason for hiding this comment

Uh oh!

weiji14 commented Jan 7, 2024

Uh oh!

seisman commented Jan 7, 2024

Uh oh!

seisman commented Jan 7, 2024

Uh oh!

weiji14 commented Jan 7, 2024

Uh oh!

seisman commented Jan 7, 2024

Uh oh!

weiji14 commented Jan 7, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

seisman commented Jan 6, 2024 •

edited by weiji14

Loading

weiji14 commented Jan 6, 2024 •

edited

Loading