-
Notifications
You must be signed in to change notification settings - Fork 498
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
chore: add codspeed for benchmark #4993
Conversation
.github/workflows/test_benchmark.yml
Outdated
uses: CodSpeedHQ/action@v3 | ||
with: | ||
run: cargo codspeed run | ||
token: ${{ secrets.CODSPEED_TOKEN }} |
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.
Please load from 1password.
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.
Will use 1password after I get the token. Thanks for the review.
Hi, @adriencaccia, OpenDAL has a benchmark suite that can run tests over different services based on the environment. Does Codspeed support our case? For example, running Cargo Codspeed multiple times can display their performance in groups based on service. |
Hey, at the moment there is no support for grouping benchmarks. |
Hi, @adriencaccia, there are some concerns from the ASF INFRA, do you have comments? |
Hey, I just answered in apache/arrow-rs#6150 (comment) 😉 |
Thanks! |
Hi, @dqhl76, this action has been approved! Please give it a try. Also FYI @adriencaccia and @alamb |
Hello, thank you @dqhl76 for your efforts and thank you @adriencaccia for the updates. However, the solution proposed in this PR is not what we are looking for. I'm going to close it now. Apologies for any inconvenience, and I hope we can achieve better integration in the future. |
.github/workflows/test_benchmark.yml
Outdated
- name: Build the benchmark targets | ||
run: cargo codspeed build --features tests | ||
- name: Run the benchmarks | ||
uses: CodSpeedHQ/action@v3 | ||
with: | ||
run: cargo codspeed run | ||
token: ${{ secrets.CODSPEED_TOKEN }} |
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.
@Xuanwo I have a new idea for not directly modifying files under core
:
Only modify the github action, and not directly modifying the source code. That way, the code would only be modified during the actual action run.
- name: Build the benchmark targets | |
run: cargo codspeed build --features tests | |
- name: Run the benchmarks | |
uses: CodSpeedHQ/action@v3 | |
with: | |
run: cargo codspeed run | |
token: ${{ secrets.CODSPEED_TOKEN }} | |
- name: Install codpseed-criterion-compat | |
run: cargo add --dev --rename criterion --features async,async_tokio [email protected] | |
- name: Build the benchmark targets | |
run: cargo codspeed build --features tests | |
- name: Run the benchmarks | |
uses: CodSpeedHQ/action@v3 | |
with: | |
run: cargo codspeed run | |
token: ${{ secrets.CODSPEED_TOKEN }} |
Would that meet your requirements?
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.
Hi, that looks good to me!
Thank you for @adriencaccia's new idea. I re-open this PR to make it possible for @dqhl76 to continue. |
It seems we are only permitted to install the Codspeed GitHub app, but the workflow is not being allowed. https://github.com/apache/opendal/actions/runs/11463180557 I think it still needs to communicate with infra team in jira ticket. |
This is where the action can be allowed: https://docs.github.com/en/organizations/managing-organization-settings/disabling-or-limiting-github-actions-for-your-organization#allowing-select-actions-and-reusable-workflows-to-run |
Thanks for the link. However, we don't have the organizations privilege😢, still need to seek for permission from infra team. I will try to do it tomorrow. |
Close this PR, I will reopen it on opendal repo's branch. Seems all the obstacles are removed. Thanks for all your work. This PR is from my forked repo's branch, we cannot run the action that need password. |
Which issue does this PR close?
Closes #4937 .
Rationale for this change
Trying to add a benchmark workflow to track and compare performance
What changes are included in this PR?
A new workflow that runs on every PR and the default main branch.
The criterion dependency is changed to codspeed-criterion-compat.
This should not affect
cargo bench
Are there any user-facing changes?
No
Wait for approval from infra team https://issues.apache.org/jira/projects/INFRA/issues/INFRA-26030?filter=allissues