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

chore: add codspeed for benchmark #4993

Closed
wants to merge 8 commits into from
Closed

Conversation

dqhl76
Copy link
Member

@dqhl76 dqhl76 commented Aug 10, 2024

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

uses: CodSpeedHQ/action@v3
with:
run: cargo codspeed run
token: ${{ secrets.CODSPEED_TOKEN }}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please load from 1password.

Copy link
Member Author

@dqhl76 dqhl76 Aug 10, 2024

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.

@Xuanwo
Copy link
Member

Xuanwo commented Aug 10, 2024

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.

@dqhl76
Copy link
Member Author

dqhl76 commented Aug 15, 2024

image

@adriencaccia
Copy link

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.
What I would recommend in the meantime, if relevant, is to manually change the name of the benchmarks depending on the service.
For example, for a benchmark myBench in the codebase, you could make its name depending on the service, so that multiple benchmarks are tracked on CodSpeed: service1-myBench, service2-myBench, service3-myBench...

@Xuanwo
Copy link
Member

Xuanwo commented Aug 29, 2024

Hi, @adriencaccia, there are some concerns from the ASF INFRA, do you have comments?

image

@adriencaccia
Copy link

Hi, @adriencaccia, there are some concerns from the ASF INFRA, do you have comments?

image

Hey, I just answered in apache/arrow-rs#6150 (comment) 😉

@Xuanwo
Copy link
Member

Xuanwo commented Aug 29, 2024

Hey, I just answered in apache/arrow-rs#6150 (comment) 😉

Thanks!

@Xuanwo
Copy link
Member

Xuanwo commented Oct 20, 2024

Hi, @dqhl76, this action has been approved! Please give it a try.

Also FYI @adriencaccia and @alamb

core/Cargo.toml Outdated Show resolved Hide resolved
core/Cargo.toml Outdated Show resolved Hide resolved
@Xuanwo
Copy link
Member

Xuanwo commented Oct 22, 2024

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.

@Xuanwo Xuanwo closed this Oct 22, 2024
Comment on lines 50 to 56
- 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 }}
Copy link

@adriencaccia adriencaccia Oct 22, 2024

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.

Suggested change
- 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?

Copy link
Member

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!

@Xuanwo Xuanwo reopened this Oct 22, 2024
@Xuanwo
Copy link
Member

Xuanwo commented Oct 22, 2024

Thank you for @adriencaccia's new idea. I re-open this PR to make it possible for @dqhl76 to continue.

@dqhl76
Copy link
Member Author

dqhl76 commented Oct 22, 2024

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.

@adriencaccia
Copy link

It seems we are only permitted to install the Codspeed GitHub app, but the workflow is not being allowed.

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

@dqhl76
Copy link
Member Author

dqhl76 commented Oct 22, 2024

This is where the action can be allowed:

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.

@dqhl76
Copy link
Member Author

dqhl76 commented Nov 3, 2024

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.

@dqhl76 dqhl76 closed this Nov 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

new feature: Add CodSpeed for benchmark
3 participants