Skip to content

ci: run benchmarks on demand#26

Merged
mccutchen merged 7 commits intomainfrom
bench-on-demand
Jan 22, 2025
Merged

ci: run benchmarks on demand#26
mccutchen merged 7 commits intomainfrom
bench-on-demand

Conversation

@mccutchen
Copy link
Owner

The bench workflow is both very slow (~4 minutes per run) and flaky (see example failure getting in the way of #25).

Here we attempt to transform it into an on-demand workflow, made easier by an automated comment with instructions for kicking it off. The initial transformation was made primarily by copy/pasting the old workflow into claude1 and following up with some minor cosmetic tweaks.

Let's see what happens, I guess?

Footnotes

  1. Er, TIL that you can't share conversations from claude dot ai??

@github-actions
Copy link

github-actions bot commented Jan 22, 2025

🔥 Run benchmarks comparing 8396dc9 against main:

gh workflow run bench.yaml -f comparison_sha=8396dc9 -f pr_number=26

Note: this comment will update with each new commit.

@codecov
Copy link

codecov bot commented Jan 22, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.62%. Comparing base (50a630b) to head (8396dc9).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #26   +/-   ##
=======================================
  Coverage   79.62%   79.62%           
=======================================
  Files           3        3           
  Lines         432      432           
=======================================
  Hits          344      344           
  Misses         64       64           
  Partials       24       24           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

mccutchen added a commit that referenced this pull request Jan 22, 2025
Apparently workflow_dispatch jobs are only recognized in the main
branch, which is probably critical for security, but that prevents us
from testing the rewrite of the bench action in
#26.

Note: this will temporarily disable the bench action.
@mccutchen
Copy link
Owner Author

Turns out my initial UX idea won't work: It's not currently possible to link directly to a pre-filled form to trigger a workflow_dispatch action.

So maybe just a comment with a copy/paste-able command to run is enough for now?

Here's what it looks like to use:

$ gh workflow run bench.yaml -f comparison_commit=6fabf77 -f pr_number=26
✓ Created workflow_dispatch event for bench.yaml at main

To see runs for this workflow, try: gh run list --workflow=bench.yaml

@mccutchen
Copy link
Owner Author

Oh no, it looks like worfklow_dispatch events can only trigger worfklows as they defined in main, which I think means every tweak to this action will require merging the change into main untested, first. Again, I'm sure this is critical for security in ways that are non-obvious to me, but boy is it gonna be a pain in the ass to work on!

@mccutchen
Copy link
Owner Author

… but also, I just realized, that means the workflow_dispatch event triggered an action that was written to be triggered on pushes to a pull request, which means some stuff definitely won't work (e.g. I think the way it chooses the correct HEAD commit to test against is based on event context not available via workflow dispatch) … but something is running here:
https://github.com/mccutchen/websocket/actions/runs/12901790920/job/35974402206

… and it turns out that what is running there is some weird subset of the action as defined in main???

… I think maybe it was a bad idea to change an existing pull request workflow into a workflow_dispatch one without renaming the workflow or any of the steps. Oops!

@mccutchen
Copy link
Owner Author

Okay, gotta merge to iterate

@mccutchen mccutchen merged commit 9f05ba1 into main Jan 22, 2025
8 checks passed
@mccutchen mccutchen deleted the bench-on-demand branch January 22, 2025 06:07
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.

1 participant