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

[CN] Add benchmarking for PRs #537

Merged
merged 6 commits into from
Sep 25, 2024
Merged

Conversation

jprider63
Copy link
Collaborator

This PR adds a CI workflow that adds regression benchmarks to PRs. It builds upon #530 so that should be merged first (ignore the first three commits).

Every time a PR is updated, a comment is posted to the PR that displays the old runtimes and the new runtimes. It highlights any regressions and if any benchmarks were removed. An example comment is here.

Implements part of rems-project/cn-tutorial#54.

@jprider63 jprider63 force-pushed the pr-benchmarking branch 4 times, most recently from 5a0b6ab to c2cf86f Compare August 29, 2024 14:22
@jprider63
Copy link
Collaborator Author

@dc-mak The CI for benchmarking regressions fails due to a permission error when attempting to post a comment to the PR with the results. This is succeeding on our fork. I've added permissions to the workflow, but that does not seem to make a difference. Can you check if there are repository settings that are preventing comments from being posted? This post describes how to modify workflow permissions.

@dc-mak
Copy link
Contributor

dc-mak commented Sep 3, 2024

@kmemarian can you check that post? The logs error-report also shows this link: https://docs.github.com/en/rest/issues/comments?apiVersion=2022-11-28#create-an-issue-comment

@dc-mak dc-mak changed the title Pr benchmarking [CN] Add benchmarking for PRs Sep 3, 2024
@jprider63
Copy link
Collaborator Author

@kmemarian, @dc-mak, @cp526 perhaps I can sit down with one of you and we can debug this together?

@cp526
Copy link
Collaborator

cp526 commented Sep 23, 2024

@jprider63 That looks great! Happy to go over it together to get it to work, I'll email you.

@cp526 cp526 merged commit 8db0cb6 into rems-project:master Sep 25, 2024
2 of 3 checks passed
@jprider63
Copy link
Collaborator Author

FYI I posted about the permission issues here. Hopefully we get a response!

@cp526
Copy link
Collaborator

cp526 commented Sep 25, 2024

Thanks!

@jprider63 jprider63 mentioned this pull request Sep 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants