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

feat(CI): add summary of benchmarking output #16488

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

adomani
Copy link
Collaborator

@adomani adomani commented Sep 4, 2024

Add a GitHub action triggered on the !bench comments.

Reports a summary of the changes by at least 10^9 instructions. This PR contains various examples of reports generated by this script.

Zulip


Open in Gitpod

Copy link

github-actions bot commented Sep 4, 2024

PR summary e27dfa7992

Import changes for modified files

No significant changes to the import graph

Import changes for all files
Files Import difference

Declarations diff

+ Bench
+ addBenchSummaryComment
+ benchOutput
+ formatDiff
+ formatFile
+ formatPercent
+ intDecs
+ summary
+ toCollapsibleTable
+ toTable

You can run this locally as follows
## summary with just the declaration names:
./scripts/declarations_diff.sh <optional_commit>

## more verbose report:
./scripts/declarations_diff.sh long <optional_commit>

The doc-module for script/declarations_diff.sh contains some details about this script.

Copy link
Collaborator

@grunweg grunweg left a comment

Choose a reason for hiding this comment

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

Thanks for writing this and slowly automating Michael Stoll's manual comments! This looks very useful!

I took a look up to line 100 of bench_summary. Here's my usual bunch of persnickety comments.

.github/workflows/bench_summary_comment.yml Outdated Show resolved Hide resolved
.github/workflows/bench_summary_comment.yml Outdated Show resolved Hide resolved
scripts/bench_summary.lean Show resolved Hide resolved
scripts/bench_summary.lean Outdated Show resolved Hide resolved
scripts/bench_summary.lean Outdated Show resolved Hide resolved
scripts/bench_summary.lean Outdated Show resolved Hide resolved
scripts/bench_summary.lean Outdated Show resolved Hide resolved
scripts/bench_summary.lean Outdated Show resolved Hide resolved
scripts/bench_summary.lean Outdated Show resolved Hide resolved
@grunweg grunweg added the CI Modifies the continuous integration / deployment setup label Sep 5, 2024
@grunweg grunweg changed the title CI: add summary for bench feat(CI): add summary of benchmarking output Sep 5, 2024
adomani and others added 3 commits September 5, 2024 09:50
Co-authored-by: grunweg <rothgami@math.hu-berlin.de>
…community/mathlib4 into adomani/bench_summary_action
Copy link
Collaborator Author

@adomani adomani left a comment

Choose a reason for hiding this comment

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

Michael, thank you for the review: I implemented all your comments!

.github/workflows/bench_summary_comment.yml Outdated Show resolved Hide resolved
.github/workflows/bench_summary_comment.yml Outdated Show resolved Hide resolved
scripts/bench_summary.lean Show resolved Hide resolved
scripts/bench_summary.lean Outdated Show resolved Hide resolved
scripts/bench_summary.lean Outdated Show resolved Hide resolved
scripts/bench_summary.lean Outdated Show resolved Hide resolved
Copy link
Collaborator

@grunweg grunweg left a comment

Choose a reason for hiding this comment

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

I randomly stumbled across this PR. I still find it useful, so left a few tweaks and a review. Enjoy! I pushed a commit with some changes directly. Please revert any you disagree with (or let's discuss)!
In any case: the current summary is much better than none --- I can happily postpone many comments to a follow-up!

I also wondered if the output can be clarified slightly, by printing the range of instructions for each collapsible table. WDYT?


- name: Summarize bench output
run: |
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

One of my first thoughts was: "surely this can be rewritten as a script, callable via lake exe, right"? Right now, the answer is no (because the script uses IO.Log, which requires a stronger monad than just IO) --- but this is just for debugging, right? (You could sneakily use dbg_trace, I presume...?)

Put differently: the current set-up is clever, but I wonder if something more standard is possible.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'll happily push a commit rewriting this as a script, if you think that is good.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A script would be nice, but maybe I would wait to see if there is interest in actually running this script outside of CI. Btw, using IO.print/IO.println may be better than dbg_trace, assuming that they are in the right monad.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good point. I was thinking about this mainly from a maintainance point of view: adding a script is well-understood - but this can easily be a follow-up.

Sure, IO.println would work (you just need an output file...).

scripts/bench_summary.lean Show resolved Hide resolved
scripts/bench_summary.lean Show resolved Hide resolved
scripts/bench_summary.lean Show resolved Hide resolved
scripts/bench_summary.lean Show resolved Hide resolved
scripts/bench_summary.lean Show resolved Hide resolved
@grunweg grunweg added the awaiting-author A reviewer has asked the author a question or requested changes label Oct 21, 2024
Copy link
Collaborator Author

@adomani adomani left a comment

Choose a reason for hiding this comment

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

Michael, thank you so much for reviewing this further!

You labelled the PR as awaiting-author, but... what is left for me to do? 😄

scripts/bench_summary.lean Show resolved Hide resolved
scripts/bench_summary.lean Show resolved Hide resolved
scripts/bench_summary.lean Show resolved Hide resolved
scripts/bench_summary.lean Show resolved Hide resolved
scripts/bench_summary.lean Show resolved Hide resolved

- name: Summarize bench output
run: |
{
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A script would be nice, but maybe I would wait to see if there is interest in actually running this script outside of CI. Btw, using IO.print/IO.println may be better than dbg_trace, assuming that they are in the right monad.

@adomani adomani removed the awaiting-author A reviewer has asked the author a question or requested changes label Oct 22, 2024
Copy link
Collaborator

@grunweg grunweg left a comment

Choose a reason for hiding this comment

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

The main thing was to agree if these changes are acceptable to you as well. I've pushed a tiny tweak, and am happy with this PR. (There are several directions for extending it, but none of these need to happen in this PR.) Thanks for writing it!
maintainer merge


- name: Summarize bench output
run: |
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good point. I was thinking about this mainly from a maintainance point of view: adding a script is well-understood - but this can easily be a follow-up.

Sure, IO.println would work (you just need an output file...).

scripts/bench_summary.lean Show resolved Hide resolved
scripts/bench_summary.lean Show resolved Hide resolved
Copy link

🚀 Pull request has been placed on the maintainer queue by grunweg.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Modifies the continuous integration / deployment setup maintainer-merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants