-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Disable flamegraph uploads for walltime benchmarks #20419
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
Disable flamegraph uploads for walltime benchmarks #20419
Conversation
This reverts commit 25c13ea.
|
|
Maybe try running with environment variable |
| uses: CodSpeedHQ/action@653fdc30e6c40ffd9739e40c8a0576f4f4523ca1 # v4.0.1 | ||
| uses: CodSpeedHQ/action@76578c2a7ddd928664caa737f0e962e3085d4e7c # v3.8.1 | ||
| with: | ||
| mode: instrumentation |
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.
Can we keep the mode lines, it makes upgrading easier (a rennovate PR rather than having to do the upgrade manually)
|
I pinged the codspeed folks. I'd suggest to maybe give them a few hours to fix this on their side. |
|
I think @silamon is on to something. Codspeed now enables perf profiles for wallspeed benchmarks (you now see a flamegraph for walltime benchmarks too). I can't say just yet if they'll be useful or if our flamegraphs are too large but it's certainly nice to have more than just: It got slower or faster |
MichaReiser
left a comment
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.
We should try CODSPEED_PERF_ENABLED=false or accept the regression in return for now having flamegraphs
|
https://codspeed.io/astral-sh/ruff/branches/alex%2Fsubtyping-between-protocols?runnerMode=WallTime has some flamegraphs for #20368 (comment) but I don't find them useful personally. The regression in CI time is very significant, however; I'm finding it very frustrating waiting for CI to finish on #20368!
I've pushed a commit trying this out |
|
Nice, we're back to the walltime benchmarks completing in 10-11 minutes. Though there are now a lot of lines like this in the logs that I don't know the meaning of: |
MichaReiser
left a comment
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.
Thank you. I'm fine disabling this for now. It might be worth adding a comment explaining why we disable the perf uploads (because they would be nice)
Reverts #20409Prior to 25c13ea, the walltime benchmarks CI job took around 10-11 minutes. It now seems to be taking around 17-18 minutes to complete, which is a pretty major slowdown and somewhat annoying if you're waiting for benchmark results on a PR! See e.g. https://github.com/astral-sh/ruff/actions/runs/17738649543/job/50411336184