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

Improve the benchmarking workflow to generate a commit comment on PRs #64

Merged
merged 5 commits into from
Oct 12, 2023

Conversation

msschwartz21
Copy link
Collaborator

@msschwartz21 msschwartz21 commented Oct 12, 2023

I couldn't get the github-actions-benchmark to perform the way I wanted for PRs (although it's working great for generating the charts on github pages). Instead I wrote a slightly more manual action that I think gives us what we want.

The benchmarking action triggered for each PR now does the following:

  • Checks if benchmarks have been cached for the base commit and computes and saves them if not
  • Runs benchmarking for the current commit
  • Generates a table comparing the mean runtime for head and base with the percent change
  • Comments on the commit with the comparison table

@bentaculum are there any other statistics that you would want to see in that table?

@msschwartz21 msschwartz21 marked this pull request as ready for review October 12, 2023 03:19
@bentaculum
Copy link
Contributor

Looks great, just added some small changes to the autogenerated comment table, and added the tabulate dep.

@bentaculum bentaculum merged commit ecbf250 into main Oct 12, 2023
14 checks passed
@bentaculum bentaculum deleted the pr-benchmark branch October 12, 2023 18:24
@msschwartz21
Copy link
Collaborator Author

Based on this workflow run, I discovered that the commit comment action does not work on PRs opened from forked repos. This is documented in the action (see the paragraph below the linked table), but I didn't notice when I was setting it up yesterday.

I'll look into the option for configuring pull_request_target to see if that fixes it, but I'm not confident that it will. In the meantime, a short term fix would be to not use forks 😬 I could also change the action to just log the table on PRs coming from forks so at least the action wouldn't fail.

@bentaculum
Copy link
Contributor

Oh, thanks for figuring this out, I was quite baffled why this didn't go through.

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