Skip to content

Conversation

@psfinaki
Copy link
Contributor

@psfinaki psfinaki commented Jan 19, 2024

This revives and builds on the smoke tests for benchmarks introduced by @safesparrow here. It doesn't check anything new in FCS, it just verifies that the benchmarks themselves don't explode.

A few notes:

  • I don't want to bring ALL the benchmark projects to ALL the solutions, hence I keep the dotnet build FSharp.Benchmarks.sln to see that everything compiles and then do build.cmd so that we can execute FCS bench smoke tests. This is not a big waste of time
  • Since some benchmarks are very long even for single execution - and we don't want this job to be the longest - I classify benchmarks into short and long categories and run only short ones (that's the majority though)
  • I'd rather just have long category and run everything else, but this doesn't seem to be possible yet (and filtering by name would be too much hardcoding)
  • The fsharp category is removed since it wasn't universal and wasn't used
  • The script itself is renamed, moved and improved (since the BDN CLI is improved, e.g. job Dry supersedes hand-specifying all the job parameters)
  • We also stop on the first broken benchmark

Running this immediately detected a broken benchmark (assuming Giraffe presence on the machine) so it's disabled.

The smoke testing now takes about 18 minutes, the whole job around 35 minutes. So it's not the longest - and we don't want it to be, hence if needed we can mark more benchmarks as "slow" and not smoke test them.

closes #16407
closes #16408

@github-actions
Copy link
Contributor

✅ No release notes required

Copy link
Member

@vzarytovskii vzarytovskii left a comment

Choose a reason for hiding this comment

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

We shouldn't be running benchmarks in CI, since it's pointless and will be only burning cpu cycles for nothing.
Building them is enough to make sure they account for api changes.
Running won't add any useful info to CI.

@psfinaki psfinaki marked this pull request as ready for review January 19, 2024 13:53
@psfinaki psfinaki requested a review from a team as a code owner January 19, 2024 13:53
@psfinaki psfinaki requested a review from vzarytovskii January 19, 2024 13:54
@psfinaki
Copy link
Contributor Author

We shouldn't be running benchmarks in CI, since it's pointless and will be only burning cpu cycles for nothing. Building them is enough to make sure they account for api changes. Running won't add any useful info to CI.

Discussed offline - this just checks that the benchmark tests themselves are sane.

We can give this a try:

  • if our CI becomes faster and the smoke tests start taking the longest time (not the case now) - we can mark more benchmarks as "slow" and not run them at all
  • if this somehow proves to be flaky, we can remove the script execution from CI, keeping only the build

@psfinaki psfinaki enabled auto-merge (squash) January 24, 2024 17:15
@psfinaki psfinaki merged commit 544a1c9 into dotnet:main Jan 24, 2024
@psfinaki psfinaki deleted the bench/11 branch January 25, 2024 10:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

Run benchmarks in CI Facelift the existing benchmarks

4 participants