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

ci: Add GPU benchmarks and configure with just script #790

Merged

Conversation

samuelburnham
Copy link
Member

@samuelburnham samuelburnham commented Oct 24, 2023

Pull Request Benchmarking

This PR enables two methods for testing GPU performance improvement/regression on future pull requests:

  • PR issue_comment: Write !gpu-benchmark in a PR comment to output a comparative fibonacci_lem benchmark with the base branch using boa-dev/criterion-compare. This gives an early signal when performance changes seem likely, but isn't required.
  • Merge Queue (required): Queue a PR to run a comparative benchmark with the base branch, which will check for fibonacci_lem performance regressions using a configurable noise threshold (e.g. 5%) and dequeue the PR if a regression is found. This action outputs the PR bench result as a nu11ptr/criterion-table, which is saved as a persistent comment on the commit (find it by clicking on a commit and scrolling to the bottom of the page).

Benchmark Reports

When a PR is merged to master, a benchmark report for the new commit is deployed to lurk-lab.github.io for historical comparison and user-friendly viewing.

Configuration

The GPU benchmarks are run via a just script, which allows for benchmark configuration with environment variables in CI. This PR adds support for the LURK_RC and LURK_BENCH_NOISE_THRESHOLD variables, which control the reduction counts and noise threshold used in the fibonacci_lem benchmark.

@samuelburnham samuelburnham requested a review from a team as a code owner October 24, 2023 18:33
@huitseeker
Copy link
Member

We should switch to fibonacci with a canned config, or more specifically pushing a justfile to the repo:
https://github.com/casey/just

@samuelburnham samuelburnham requested review from a team as code owners October 25, 2023 14:28
@samuelburnham samuelburnham marked this pull request as draft October 25, 2023 14:29
@samuelburnham samuelburnham changed the title chore: Upgrade bench runner hardware [WIP] ci: Configure GPU benchmarks and deployment Oct 25, 2023
Copy link
Member

@huitseeker huitseeker left a comment

Choose a reason for hiding this comment

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

This is going in the right direction. Hope the comments help!

justfile Outdated Show resolved Hide resolved
.github/workflows/merge-group.yml Outdated Show resolved Hide resolved
.github/workflows/merge-group.yml Outdated Show resolved Hide resolved
.github/workflows/bench-deploy.yml Outdated Show resolved Hide resolved
@samuelburnham
Copy link
Member Author

just is amazing, but thought I'd mention a few issues I've had from a first pass with it:

  • No explicitly supported lists or for loops, have to use shell script
  • Unclear what the difference is between a recipe, script, and variable
  • Comments aren't always comments, e.g. the line before a shebang script
  • Conflicting syntax/patterns for global set shell vs shebang script
  • No actively maintained VSCode integration
  • Beware of hidden tabs when copy-pasting, causing whitespace errors
  • Unhelpful error messages make it tricky to debug
  • Not great support for nushell
  • Doesn't support interpolation of strings and backticks (yet)

None of these are disqualifying, but it will need some additional effort if we want the whole team to use it for bench/CI configuration.

@samuelburnham samuelburnham marked this pull request as ready for review October 27, 2023 05:53
@samuelburnham samuelburnham changed the title [WIP] ci: Configure GPU benchmarks and deployment ci: Add just configuration for GPU benchmarks and deployment Oct 27, 2023
Copy link
Member

@huitseeker huitseeker left a comment

Choose a reason for hiding this comment

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

This will need a rebase on #808.

For the record, the thing I'm attached to is bench command being separate (in a different file) from github actions / CI workflows. If you would prefer a python script, a shell script, or anything else to host the benchmark command instead of a justfile @samuelburnham, I will be happy to go with your decision: you own this one.

@samuelburnham
Copy link
Member Author

If you would prefer a python script, a shell script, or anything else to host the benchmark command instead of a justfile

Nothing better immediately comes to mind, pardon the late-night rant 😄 In the interests of time LGTM, and advocate for justfile usage mainly in GH actions rather than general-purpose bench scripting for now.

@samuelburnham samuelburnham force-pushed the upgrade-bench-runner branch 14 times, most recently from ff960f4 to a4bba3e Compare October 27, 2023 21:13
@samuelburnham samuelburnham force-pushed the upgrade-bench-runner branch 4 times, most recently from 6d0e4ed to 511ac76 Compare October 30, 2023 17:50
@samuelburnham samuelburnham force-pushed the upgrade-bench-runner branch 3 times, most recently from f448a79 to 8bff5d5 Compare October 30, 2023 20:35
@samuelburnham samuelburnham force-pushed the upgrade-bench-runner branch 2 times, most recently from f8c6d7e to 13592b9 Compare October 31, 2023 00:09
@samuelburnham samuelburnham changed the title ci: Add just configuration for GPU benchmarks and deployment ci: Add GPU benchmarks and configure with just script Oct 31, 2023
Copy link
Member

@huitseeker huitseeker left a comment

Choose a reason for hiding this comment

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

Thank you so much, that's a huge step!

@huitseeker huitseeker mentioned this pull request Oct 31, 2023
1 task
@samuelburnham samuelburnham added this pull request to the merge queue Oct 31, 2023
Merged via the queue into argumentcomputer:master with commit 2771b3d Oct 31, 2023
15 checks passed
@samuelburnham samuelburnham deleted the upgrade-bench-runner branch October 31, 2023 13:52
samuelburnham added a commit to samuelburnham/lurk-rs that referenced this pull request Oct 31, 2023
github-merge-queue bot pushed a commit that referenced this pull request Oct 31, 2023
* Fix bugs in #790

* Address feedback & cleanup
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.

2 participants