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

feat: add CodSpeed to the project #6150

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

adriencaccia
Copy link

Which issue does this PR close?

Closes #6149

@samuelcolvin, I let you take it from here if you want :)
In order for it to work, https://codspeed.io/ needs to be installed on the organization and the repo.

What changes are included in this PR?

A new workflow benchmark.yml that runs on every PR and the default master branch.
The criterion dependency is changed to codspeed-criterion-compat.
The eq scalar StringViewArray bench is commented out, because it fails on the runner.

Are there any user-facing changes?

No.

@adriencaccia adriencaccia marked this pull request as draft July 29, 2024 09:53
@github-actions github-actions bot added parquet Changes to the parquet crate arrow Changes to the arrow crate labels Jul 29, 2024
@adriencaccia adriencaccia force-pushed the feat/add-codspeed branch 2 times, most recently from ccb4cf3 to 081fa1f Compare July 29, 2024 10:01
@samuelcolvin
Copy link
Contributor

I'm 👍 👍 👍 for this, but I don't have permissions on the repo. I think installing this will require @alamb.

@alamb
Copy link
Contributor

alamb commented Jul 29, 2024

I triggered the workflow on this PR but I think codspeed won't run until we merge it

One way to test is to make some PRs against your own fork of arrow-rs and run the actions there to test -- that is how we have tested github actions in the past

@Xuanwo
Copy link
Member

Xuanwo commented Jul 29, 2024

I'm 👍 👍 👍 for this, but I don't have permissions on the repo. I think installing this will require @alamb.

I double checked the permission that needed by CodSpeed. It only needs read permission which should be ok for the ASF INFRA team.

image

@adriencaccia adriencaccia force-pushed the feat/add-codspeed branch 2 times, most recently from 333a86c to abc71d2 Compare July 29, 2024 12:35
@adriencaccia
Copy link
Author

The codspeed-benchmarks fails with the following message: moonrepo/setup-rust@v1 and codspeedhq/action@v3 are not allowed to be used in apache/arrow-rs

I triggered the workflow on this PR but I think codspeed won't run until we merge it

If I understand correctly, the way to allow those actions is to first merge this PR?

@alamb
Copy link
Contributor

alamb commented Jul 31, 2024

If I understand correctly, the way to allow those actions is to first merge this PR?

I think this may be due to ASF security policy: https://infra.apache.org/github-actions-policy.html

Looks like maybe we have to pin the action to a specific SHA?

@adriencaccia
Copy link
Author

Looks like maybe we have to pin the action to a specific SHA?

I pinned the two actions to specific SHAs, the workflow still fails.
It seems they will still need to be manually approved somehow.

@Xuanwo
Copy link
Member

Xuanwo commented Jul 31, 2024

Looks like maybe we have to pin the action to a specific SHA?

Hi, it doesn't work in this way. We need the INFRA team to approve those actions via jira tickets. Here is an example: https://issues.apache.org/jira/projects/INFRA/issues/INFRA-25997

I'm willing to help do this if you want.

@Xuanwo
Copy link
Member

Xuanwo commented Aug 11, 2024

Hi, we have submited a ticket to enable codspeed: https://issues.apache.org/jira/browse/INFRA-26030. Maybe @alamb want to comment your same request too?

@Xuanwo
Copy link
Member

Xuanwo commented Aug 15, 2024

Hi, @adriencaccia the ASF INFRA team has question about the github app's permission request:

Please explain why it needs access to email addresses. This is likely not allowed under the European GDPR.

Do you have any response to this?

@adriencaccia
Copy link
Author

adriencaccia commented Aug 29, 2024

Hi, @adriencaccia the ASF INFRA team has question about the github app's permission request:

Please explain why it needs access to email addresses. This is likely not allowed under the European GDPR.

Do you have any response to this?

Hey @Xuanwo, we only access the email addresses of the members of a GitHub organization to ensure that a logged user on CodSpeed is indeed a member of this organization.

@adriencaccia
Copy link
Author

Hey @Xuanwo, is there anything I can do to help merge this?

@Xuanwo
Copy link
Member

Xuanwo commented Oct 9, 2024

Hey @Xuanwo, is there anything I can do to help merge this?

Hi, we are waiting for the ASF INFRA team to review this request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrow Changes to the arrow crate parquet Changes to the parquet crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Track performance changes in CI
4 participants