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

[LUR-27] CI: Reject large perf regressions #1028

Closed
huitseeker opened this issue Jan 8, 2024 · 4 comments · Fixed by argumentcomputer/gh-actions-runner#25
Closed

[LUR-27] CI: Reject large perf regressions #1028

huitseeker opened this issue Jan 8, 2024 · 4 comments · Fixed by argumentcomputer/gh-actions-runner#25
Labels
A-1 Proof Scaling CI Related to our CI pipelines

Comments

@huitseeker
Copy link
Contributor

huitseeker commented Jan 8, 2024

#996 is a textbook case where the bench CI pipeline (which may well be buggy) reported a characterised regression. This is not a case of the CI failing to run or finding no suitable referential.

The regression factor is > 2 for all tests.

When that happens we should at least file an issue (if not fail the merge tests) and not count on folks reading the bench comments.

LUR-27

@huitseeker huitseeker changed the title Reject large perf regressions [LUR-27] Reject large perf regressions Jan 8, 2024
@huitseeker huitseeker changed the title [LUR-27] Reject large perf regressions [LUR-27] CI: Reject large perf regressions Jan 8, 2024
@huitseeker huitseeker changed the title [LUR-27] CI: Reject large perf regressions [LUR-27] CI: Reject large perf regressions Jan 8, 2024
@huitseeker huitseeker added CI Related to our CI pipelines A-1 Proof Scaling labels Jan 8, 2024
@huitseeker
Copy link
Contributor Author

from @samuelburnham :

It appears that the bc command is not found in https://github.com/lurk-lab/lurk-rs/actions/runs/7452711363/job/20276565702, which I think is preinstalled on GitHub runners (which I tested) but not our self-hosted machine

@porcuquine
Copy link
Contributor

Although I know sorting through these mechanics is tedious, having this in place will be a big step forward. Given how performance-sensitive we are, and how many interacting systems can lead to regressions, it's important that we have an automated way of preventing us from accidentally veering down paths that will be expensive or difficult to recover from later.

@huitseeker
Copy link
Contributor Author

The bc fix is there, but it's not tested, so I'm gonna insist this remains open.

Further, #1029 shows we have trouble locating the regression, which may have been in a dependency, because no job outside of the merge-test one runs through a differential benchmark.
The job we have under workflow-dispatch runs a single bench:
https://github.com/lurk-lab/lurk-rs/blob/main/.github/workflows/gpu-bench-workflow-dispatch.yml

It would simplify forensics to change that.

@samuelburnham
Copy link
Contributor

Agreed, sorry I didn't realize "Should fix X" actually closes the issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-1 Proof Scaling CI Related to our CI pipelines
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants