Skip to content

CI Performance Testing#4253

Merged
trexfeathers merged 41 commits intoSciTools:mainfrom
jamesp:asv-ci
Jul 23, 2021
Merged

CI Performance Testing#4253
trexfeathers merged 41 commits intoSciTools:mainfrom
jamesp:asv-ci

Conversation

@jamesp
Copy link
Member

@jamesp jamesp commented Jul 22, 2021

🚀 Pull Request

Description

An MVP for CI performance testing PRs.

Things in this PR:

benchmarks/benchmarks/*

These are directly lifted from the offline metrics repository. No edits made.

benchmarks/conda_lock_plugin.py

This is a low-touch change to asv.plugins.conda.Conda to use a lockfile as the environment source.
The trickiness here is that asv doesn't really have great support for per-commit environment changes.
It wants to make one environment and then run a whole load of commits in this same environment.
So this environment plugin moves the setting of environment packages to the build stage of testing
a specific commit.
This may have a poor interaction with parallel processing of environments.

.github/workflows/benchmark.yml

This workflow is configured to run on pull requests.
It runs asv on the head of the pull request and the head of the target branch.
The results are compared and output as an artifact from the run. i.e. you can download the .asv/results
folder and interrogate in more detail if needed.
If the comparison finds performance degradation that hits ASV's criteria (currently default criteria)
then the run is marked as failed and will show up in the pull request checks section as a failed check.
See jamesp#9 for an example of this.
If there are no changes above the threshold, the check is a success.


Consult Iris pull request check list

@jamesp jamesp requested a review from trexfeathers July 22, 2021 15:16
Copy link
Contributor

@trexfeathers trexfeathers left a comment

Choose a reason for hiding this comment

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

@jamesp thanks so much for taking this on. Makes me happy to see benchmarking moving forward. Some comments for you, and some stuff that won't fit as code annotations:

  • Also need to remove the tests/benchmarking directory. The single benchmark in there is part of our offline suite, although we will need to sort out file loading before it come into this setup.

  • Could we add .gitignore and pyproject.toml exclusions for benchmarks/.asv? I've been burned before!

* [ ] What do you think of making a Nox session for this? Too early? I would want to eventually - avoids everyone having to know the correct asv run ... incantation if they want to run benchmarks manually.

  • As discussed offline, it would be great if we could soon replicate the ASV environment plugin introduced in SciTools/iris-esmf-regrid#76, to truly defer environment management to a single common place. Something for me to capture in an issue once merged.

@jamesp
Copy link
Member Author

jamesp commented Jul 23, 2021

Ok I think the checkout logic is way too naive, it will fail with branches and other forks. I'll review it and come up with something better

@jamesp
Copy link
Member Author

jamesp commented Jul 23, 2021

ok @trexfeathers, I think this might be ready for final review.

@trexfeathers
Copy link
Contributor

trexfeathers commented Jul 23, 2021

I just took a look at compare.txt for the latest run. All the benchmarks for the 'previous' commit (main in this case) failed. Something to do with that checkout perhaps?

Benchmarks that have improved:

       before           after         ratio
     [149164ef]       [c1c7a17f]
           failed              n/a      n/a  aux_factory.FactoryCommon.time_create
           failed              n/a      n/a  aux_factory.FactoryCommon.time_return
           failed       14.6±0.1μs      n/a  aux_factory.HybridHeightFactory.time_create
           failed         81.4±2ns      n/a  aux_factory.HybridHeightFactory.time_return
           failed      16.2±0.05μs      n/a  coords.AncillaryVariable.time_create
           failed         82.8±5ns      n/a  coords.AncillaryVariable.time_return
           etc etc...

Clues in the stdout:

[  0.00%] ··· Error running /usr/bin/git checkout -f 149164efa13116ad2ce391f02ba6a4480fd816c8 (exit status 128)
              STDOUT -------->
              
              STDERR -------->
              fatal: reference is not a tree: 149164efa13116ad2ce391f02ba6a4480fd816c8

[  0.00%] ·· Skipping conda-lock-py3.8
[  0.17%] ··· aux_factory.FactoryCommon.time_create skipped
[  0.33%] ··· aux_factory.FactoryCommon.time_return skipped
[  0.50%] ··· aux_factory.HybridHeightFactory.time_create skipped
[  0.67%] ··· aux_factory.HybridHeightFactory.time_return skipped
[  0.83%] ··· coords.CoordCommon.time_create skipped
[  1.00%] ··· coords.CoordCommon.time_return skipped
etc etc...

@jamesp
Copy link
Member Author

jamesp commented Jul 23, 2021

Should be resolved

@trexfeathers
Copy link
Contributor

Fancy adding this to the README?
asv

@trexfeathers
Copy link
Contributor

Thanks for your persistence @jamesp

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.

ASV - Automate Iris dependencies ASV - Create Basic Benchmark Testing Suite

2 participants