-
Notifications
You must be signed in to change notification settings - Fork 258
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
ci: Track main branch benchmarks with Bencher #1441
Conversation
WalkthroughThe update introduces a step in the benchmarking workflow to set up the Bencher CLI tool, followed by executing benchmarks through Bencher with designated configurations and an authentication token. This enhancement aims to streamline benchmark tests, ensuring they are performed efficiently and securely. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
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.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- .github/workflows/benchmark.yml (1 hunks)
Additional comments: 1
.github/workflows/benchmark.yml (1)
- 86-100: The integration of Bencher CLI is a significant addition to the benchmarking workflow. Here are a few observations and recommendations:
- Use of
bencherdev/bencher@main
: It's generally recommended to pin actions to a specific version or commit to avoid unexpected changes. Consider using a tagged version ofbencherdev/bencher
if available.- Security of
BENCHER_API_TOKEN
: Ensure that theBENCHER_API_TOKEN
secret is properly set up in the repository settings. This is crucial for authentication with Bencher Cloud.- Clarity on
--testbed
parameter: The--testbed
parameter is set tobenchmarking-runner
. Ensure that this value accurately represents the testbed environment used by Bencher for benchmarking.- Documentation and Comments: Adding comments explaining the purpose of each parameter and step, especially for custom scripts like
json_to_md.rs
andcriterion_compare.rs
, would improve maintainability and clarity for future contributors.Overall, the integration seems well-executed, but consider the above points for refinement.
You can try using ubuntu-latest instead of benchmarking-runner just for testing purpose. |
Added the token on GitHub. Should I merge this PR? Can we also track macro benchmarks? |
I just sent you an invite to the
We could wait for bencherdev/bencher#347 to be implemented. |
Action required: PR inactive for 2 days. |
This reverts commit e0a7f6c.
Summary:
@tusharmath this PR adds tracking of the micro-benchmarks on the
main
branch.Once, things are able to run the results will be visible on this public perf page: https://bencher.dev/perf/tailcall
In order for things to work in the
tailcallhq/tailcall
repo, you will need to:tailcall
organization that I have created.BENCHER_API_TOKEN
as a Repository secret. (ex: Repo -> Settings -> Secrets and variables -> Actions -> New repository secret)How do you have
benchmarking-runner
setup?Due to the customer
benchmarking-runner
used, it is a bit difficult for me to show you things working pre-merge.The run just gets skipped: https://github.com/epompeii/tailcall/actions/runs/8281704757
If you have have any advice on how to make this work on my end, please let me know, and I will try to implement them. I don't want to break your build!
Future work would include transitioning from the custom script currently used to using Bencher for relative benchmarking and creating PR comments.
Issue Reference(s):
Relates to: #436
Completes: #1300
Build & Testing:
cargo test
successfully../lint.sh --mode=fix
to fix all linting issues raised by./lint.sh --mode=check
.Checklist:
<type>(<optional scope>): <title>
Summary by CodeRabbit