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: Customize benchmark PR comment action #1071

Closed

Conversation

samuelburnham
Copy link
Contributor

PR benchmark comment customization

This PR enables customization of the issue_comment PR comment action from directly within the !gpu-benchmark comment. We can now run multiple benchmarks, with customizable features and env vars. The initial format is as follows:

!gpu-benchmark
Benches:
fibonacci,end2end
Features:
cuda,portable
Env vars:
LURK_RC=100,600,900
LURK_BENCH_NOISE_THRESHOLD=0.05

Implementation notes

  • Splits the benchmark into a setup action to parse the input parameters, then runs one GPU benchmark job per benchmark in a matrix job. This is due to the inherent limitation of boa-dev/criterion-compare-action only being able to run one benchmark or all of them (the latter with some hacks).
  • Removes reliance on the bench.env file used by just in favor of user specification in the comment
  • Removes the CPU-only benchmark as it's seldom used. Probably a better solution would be to turn off the cuda feature once implemented, as it's hardcoded here.

Limitations

  • Brittle parsing format due to rudimentary awk script, which should be improved in future work.
  • Benches and features must be provided on one line, separated by comma
  • Env vars must be listed one per line

Successful run

https://github.com/lurk-lab/ci-lab/actions/runs/7580695577

@samuelburnham samuelburnham requested a review from a team as a code owner January 19, 2024 07:40
Copy link
Contributor

@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.

I'm happy with exploring a richer set of benchmarks, but I really want to support a sensible default, as we do now.

There's a lot of friction brought by needing to learn what is the default benchmark, and exposure to janky parsing does not make the situation better (OTOH, for exotic needs, janky parsing is just fine).

Case in point: I'd like the PR to be backward compatible with the following "dumb" behavior:
argumentcomputer/ci-lab#59 (comment)
run:
https://github.com/lurk-lab/ci-lab/actions/runs/7670967389
error point:
https://github.com/lurk-lab/ci-lab/blob/main/.github/workflows/pr-comment.yml#L50

@samuelburnham
Copy link
Contributor Author

Superseded by #1114

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