-
Notifications
You must be signed in to change notification settings - Fork 6
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
Run benchmarks in the correct context and compare main vs PR #28
Conversation
dd8e457
to
aaabe5f
Compare
The original version, which was force-pushed away, was generated with the "help" of AI and didn't actually work. The version present now is based on the Bencher docs. |
aaabe5f
to
511fe12
Compare
511fe12
to
90e4002
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! I don't see any problems here, but I've left a few comments with just some notes about different ways I would approach some things and such. Up to you if you feel like including any of those :)
Definitely appreciate the review @bo0tzz! |
…-Co#28) * Split benchmarks into a run and a comment workflow * Use explicit paths rather than glob for benchmark results Co-authored-by: bo0tzz <[email protected]> * Benchmark: Use default pull_request events --------- Co-authored-by: bo0tzz <[email protected]>
Overview
This PR attempts to fix two issues:
main
and the PR to ease comparisons.main
, not the PR.Details
This PR splits the benchmarking workflow into two pieces:
pull_request
context (i.e., from the fork) that executes the code and uploads the results.workflow_run
context (i.e., from CADmium-Co/ISOtope) that downloads the results and prints them in a comment.Running untrusted code is recommended in the
pull_request
context since it runs in the context of the fork and, thus, would have no access to any secrets or other branches. Theworkflow_run
piece is necessary to post the comment since the action running in thepull_request
context has read-only permissions and is unable to post/edit comments on the PR.