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

Benchmark downloader (script + CI job) #12818

Merged
merged 2 commits into from
May 23, 2022
Merged

Benchmark downloader (script + CI job) #12818

merged 2 commits into from
May 23, 2022

Conversation

cameel
Copy link
Member

@cameel cameel commented Mar 18, 2022

Depends on #12804. Merged.

The script makes several CirecleCI and Github API requests to get the JSON with benchmark results for a specific PR or branch. I think it will be very handy for manual use, though my primary goal here is to use it in c_ext_benchmarks job to get benchmarks from the base PR and provide the diff. The PR extends that job to generate such diffs and attach them as artifacts.

After the experiences with #12181 and ethereum/solc-bin#35 I decided to use Python to make this more maintainable. This way I'll be able to cover it with tests, and, compared to my earlier Bash scripts, it was easy to make a module with helpers that we'll be able to reuse in other situations that require API access. While it isn't that short as a whole, it's more concise compared to the previous Bash scripts - the main logic specific to this task is concentrated in one function and the rest are reusable helpers and command-line argument definitions (which is more like docs than real code).

I'm on purpose not trying to do too much error handling to keep things simple. I.e. it either works or you get an exception. Only the cases that would not just crash on unexpected response (like finding no matching results) are explicitly checked.

This is still work in progress. I still need to Everything is done:

  • Add some unit tests.
  • Handle the case where the job has not finished yet.
  • Test some corner cases to make sure things fail properly on errors.
  • Getting artifacts for the specific revision of base branch that the requested PR/branch is based on. This would be helpful in cases where base branch has moved on since the PR/branch was created and has extra commits that affect benchmark results.

Usage of the script

To get benchmarks from your branch and from develop, you just run this:

scripts/externalTests/download_benchmarks.py --branch develop
scripts/externalTests/download_benchmarks.py

This stores summarized-benchmarks-<branch>-<commit hash>.json and all-benchmarks-<branch>-<commit hash>.json in the current directory. Now you can run the script from #12804 on the files.

If you want to compare someone else's PR you probably have the PR number handy - you can pass that to the script using the --pr option instead of the branch.

You can also use --base-of-pr option to get artifacts from the branch your PR is based on (instead of develop). By default the script finds a CI run matching not only the base branch name but also the exact commit. This guarantees that even if the base branch has moved on and has extra commits, your comparison will not be influenced by these commits.

Sometimes you might still want to get the benchmark from the latest state of the base branch, e.g. if there was no successful run on the commit you're based on and you know that any extra commits are not going to distort results all that much - in that case you can use --any-commit flag to bypass the commit check.

@cameel cameel self-assigned this Mar 18, 2022
@cameel cameel force-pushed the benchmark-downloader branch 8 times, most recently from fa1ddc6 to f41e05c Compare March 23, 2022 12:34
@cameel cameel added the has dependencies The PR depends on other PRs that must be merged first label Mar 23, 2022
@cameel
Copy link
Member Author

cameel commented Mar 23, 2022

This is now done and ready for review.

@cameel cameel marked this pull request as ready for review March 23, 2022 12:37
@cameel cameel changed the title Benchmark downloader Benchmark downloader (script + CI job) Mar 23, 2022
Comment on lines +1265 to +1286
"${scripts_dir}/externalTests/benchmark_diff.py" table \
--output-format markdown \
--style humanized \
base-branch/summarized-benchmarks-*.json \
summarized-benchmarks.json > diff/benchmark-diff-summarized-table-markdown-humanized.md
"${scripts_dir}/externalTests/benchmark_diff.py" table \
--output-format markdown \
--style absolute \
base-branch/summarized-benchmarks-*.json \
summarized-benchmarks.json > diff/benchmark-diff-summarized-table-markdown-absolute.md
"${scripts_dir}/externalTests/benchmark_diff.py" inplace \
--style absolute \
base-branch/summarized-benchmarks-*.json \
summarized-benchmarks.json > diff/benchmark-diff-summarized-inplace-absolute.md
"${scripts_dir}/externalTests/benchmark_diff.py" inplace \
--style absolute \
base-branch/all-benchmarks-*.json \
all-benchmarks.json > diff/benchmark-diff-all-table-inplace-absolute.md
Copy link
Member Author

@cameel cameel Mar 23, 2022

Choose a reason for hiding this comment

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

We might not need all of these. benchmark-diff-summarized-table-markdown-humanized.md will probably be the most useful one in practice. It will soon be possible to include extra columns with absolute values in it as well.

On the other hand the overhead of having all of them is negligible and also this acts as an end to end test for the script so I included them here anyway.

@cameel
Copy link
Member Author

cameel commented Apr 4, 2022

Note, about the c_ext_benchmarks failure:

Looking for pipelines that ran on branch benchmark-differ, commit fd0ec2e30a8d9c60bfbb15a984cf90bb0f571908.
[ERROR] Job c_ext_benchmarks has failed or is still running. Current status: blocked.

This is a limitation of the script with regard to PRs that are based one on the other. The job tries to access artifacts from the same job on the base PR. If both were pushed at the same time, it's likely that that base job has not finished yet. In that case if just needs to be reran after the base job finishes.

Base automatically changed from benchmark-differ to develop April 7, 2022 07:24
@cameel cameel removed the has dependencies The PR depends on other PRs that must be merged first label Apr 7, 2022
ekpyron
ekpyron previously approved these changes Apr 11, 2022
Copy link
Member

@ekpyron ekpyron 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 lot of infrastructure for just downloading some files :-). But probably not much one can do to avoid that...

I haven't reviewed the python scripts at all, but - as usual for these PRs - I'd say it's fine to merge based on them working properly in CI - and it being nice to have them, but them being non-critical.

If anyone is up for going through the python scripts once, though, it wouldn't hurt.

@cameel
Copy link
Member Author

cameel commented Apr 11, 2022

This is a lot of infrastructure for just downloading some files :-). But probably not much one can do to avoid that...

Unfortunately, dealing with APIs reliably is just verbose. This is not the first time I need to access their APIs and I always end up with a really ugly Bash script. We really needed some wrappers for that that we can later reuse as needed.

CircleCI actually has its own CLI utility called circleci and I was initially hoping I'd get away just using that instead of writing it in Python. But it does not really have the same use case. It does not have commands for fetching info from the API, only for controlling the pipelines from CLI.

Also, to be fair, it adds more value than just "downloading some files". It automates finding the exact runs that you need which is more tedious than it seems at first. E.g. if your branch is based on develop and something got merged in the meantime, you have to dig into history find the old commit ID, then locate the right run via this horrible list of pipelines in CircleCI's UI with infinite scroll. It's painful to use, especially If what you're looking for is not immediately on the first page. With the script you just give it a PR number and it either gives you the right benchmark or fails. Or you go to the artifacts in your PR and find the diff attached there. You don't have to think about it, which is how it should work.

@chriseth chriseth merged commit a8dc762 into develop May 23, 2022
@chriseth chriseth deleted the benchmark-downloader branch May 23, 2022 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants