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

Fold output of each benchmark #10914

Merged
merged 8 commits into from
Sep 9, 2024
Merged

Conversation

JaroslavTulach
Copy link
Member

@JaroslavTulach JaroslavTulach commented Aug 29, 2024

Pull Request Description

This PR improves output of benchmarks run on GitHub Actions CI to group and fold output for each benchmark.

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

@JaroslavTulach JaroslavTulach added the CI: No changelog needed Do not require a changelog entry for this PR. label Aug 29, 2024
@JaroslavTulach JaroslavTulach self-assigned this Aug 29, 2024

@Override
public void startBenchmark(BenchmarkParams benchParams) {
output.println("::group::" + benchParams.getBenchmark());
Copy link
Member Author

Choose a reason for hiding this comment

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

The ::group:: format seems to be mentioned at this discussion.

Copy link
Member

Choose a reason for hiding this comment

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

@JaroslavTulach JaroslavTulach force-pushed the wip/jtulach/GithubActionsOutput branch from 37d0385 to b1ef6fb Compare August 29, 2024 10:56
@JaroslavTulach
Copy link
Member Author

JaroslavTulach commented Aug 29, 2024

Update: solved in 9ba4428

I don't know how to print plain line without any prefix in our Rust build system. If I use info! like in 37d0385 the line gets prefixed and looses its :: command prefix. If I use println! like in b1ef6fb the text isn't displayed in the log at all! Any advice from Rust experts?

Akirathan
Akirathan previously approved these changes Aug 29, 2024
Copy link
Member

@Akirathan Akirathan 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 a good idea. I am interested in how the end result will look like. I had no idea that there is some output grouping for GH actions. We could probably integrate this functionality even for other actions.

@JaroslavTulach JaroslavTulach marked this pull request as draft August 29, 2024 11:40
@JaroslavTulach
Copy link
Member Author

JaroslavTulach commented Aug 30, 2024

Locally the command lines seem to be determined fine. If I execute, I see:

enso$ ./run backend benchmark runtime --minimal-run
 INFO ide_ci::program::command: sbt ℹ️ [info] # *** WARNING: Use non-forked runs only for debugging purposes, not for actual performance runs. ***
 INFO ide_ci::program::command: sbt ℹ️ [info] Iteration   1: 102.137 ms/op
::endgroup::
::group::org.enso.interpreter.bench.benchmarks.semantic.ArrayProxyBenchmarks.sumOverComputingProxy
 INFO ide_ci::program::command: sbt ℹ️ [info] # Run progress: 12.68% complete, ETA 00:01:29
 INFO ide_ci::program::command: sbt ℹ️ [info] # Fork: N/A, test runs in the host VM

e.g. both ::endgroup and ::group are properly detected and the "INFO..." prefix isn't printed. Let's see what does the CI print?

folds

Finally it does something reasonable!

@JaroslavTulach JaroslavTulach marked this pull request as ready for review August 30, 2024 05:56
@JaroslavTulach
Copy link
Member Author

JaroslavTulach commented Aug 30, 2024

Certain parts of the stdlibs bench run output look nice as well.

Some messages "escape" the grouping. Not sure why and whether that matters that much to prevent integration. Hypothesis:

Let's try to switch to stdout - stdlib benchmark is running.

Update: The latest stdlibs output looks pretty decent. Looks like c698ef7 helped.

@JaroslavTulach JaroslavTulach requested a review from 4e6 as a code owner August 30, 2024 07:41
@Akirathan Akirathan dismissed their stale review August 30, 2024 08:14

No longer convinced this is needed at all

Copy link
Member

@Akirathan Akirathan left a comment

Choose a reason for hiding this comment

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

After some thinking, I believe that your changes are unnecessary, and seeing the current folded output, it is very messy - The group for benchGenerateList obviously contains output even from some other benchmark for example.

May I ask you again to explain your use-case that this folding will solve, and why do you think it is needed?

Note that at the end of each benchmark run (the very end of the whole job output) there is table-like structure with all the scores. In your case it is at https://github.com/enso-org/enso/actions/runs/10627416248/job/29460495368#step:7:1886. If you want to see the output for a single benchmark, you can just type its name in the search field. You would need to do that even if the output was folded.

EDIT: If you still think the folding will be beneficial, I will revoke my PR reject after I see that the folding works and that no output is assigned to a wrong group.

@JaroslavTulach
Copy link
Member Author

JaroslavTulach commented Aug 30, 2024

... changes are unnecessary...

I am sad you have changed your mind. I believe that with few more homey touches, we can make this folding work well and increase overall satisfaction.

May I ask you again to explain your use-case that this folding will solve, and why do you think it is needed?

  • I want to hide non-interesting crap - folding would achieve that
  • I want to organize things per interest - using title of a benchmark for navigation seems human friendly
  • I was hoping we tackle the size of the log (didn't work trying more in 2344fac) as we are reaching a limit on stdlibs benchmarks

Note that at the end of each benchmark run (the very end of the whole job output) there is table-like structure with all the scores. In your case it is at https://github.com/enso-org/enso/actions/runs/10627416248/job/29460495368#step:7:1886.

Right now the log stops at 22% of stdlibs benchmark run so the table isn't really visible.

If you want to see the output for a single benchmark, you can just type its name in the search field. You would need to do that even if the output was folded.

I've just tried and folding has no negative effect on searching. These ::group: folds are automatically expanded when the found text is in a collapsed fold.

If you still think the folding will be beneficial

The latest stdlibs output looks pretty decent. Looks like c698ef7 helped.

@JaroslavTulach
Copy link
Member Author

The newest idea c36fdad to solve the too huge log issue is to differentiate between execution of all benchmarks and execution of a selected benchmark(s):

  • running bench limits the output to ten lines per benchmark - let's see if that helps
  • running benchOnly Xyz dumps full output

As a result the CI run will be less verbose, but one will have a chance to re-run individual benchmark locally and see whole output.

@JaroslavTulach
Copy link
Member Author

at the end of each benchmark run (the very end of the whole job output) there is table-like structure with all the scores.

Yes, with the latest run the table is finally visible.

you can just type its name in the search field.

That is still possible even the output is folded.

If you still think the folding will be beneficial, I will revoke my PR reject

Yes, consider revoking your reject.

Copy link
Member

@Akirathan Akirathan left a comment

Choose a reason for hiding this comment

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

The latest output seems reasonable. The fact that when all the benchmarks are run, the output is less verbose is good - it helps to be able to interactively browse the output on the CI.

@JaroslavTulach JaroslavTulach merged commit 89c5b31 into develop Sep 9, 2024
42 checks passed
@JaroslavTulach JaroslavTulach deleted the wip/jtulach/GithubActionsOutput branch September 9, 2024 10:30
jdunkerley pushed a commit that referenced this pull request Sep 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: No changelog needed Do not require a changelog entry for this PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants