Integrate ASV with Nox

[trexfeathers]

🚀 Pull Request

Description

• Creates a Nox session for typical benchmark runs, and uses this for the CI GitHub Action
• Defers environment preparation for any ASV commit to use the Nox environment preparation for the tests session.

---

Consult Iris pull request check list

[trexfeathers]

Closes #4255

[jamesp]

This looks great @trexfeathers, I've started to review but it'll take a bit of time to test the functionality. While I get going, could you confirm it's doing the right thing? Here's what I've done so far:

1. Checkout your PR branch
2. Run nox. This is the output:

 pipx run nox -s benchmarks
nox > Running session benchmarks(ci compare)
nox > Creating conda env in .nox/benchmarks-ci-compare with python=3.8
nox > python -m pip install asv nox
nox > cd benchmarks
nox > asv machine --yes
<<... output of asv machine elided ...>>
ram [7864964]: nox > asv continuous --factor=1.2 HEAD^1 HEAD
· The virtualenv name was hashed to avoid being too long.
· Creating environments...
· Discovering benchmarks
·· Running Nox environment update for: nox-conda-py3.8.
·· Uninstalling from nox-conda-py3.8
·· Building 415e90ad <pr/trexfeathers/4378> for nox-conda-py3.8..
·· Installing 415e90ad <pr/trexfeathers/4378> into nox-conda-py3.8.
· Running 300 total benchmarks (2 commits * 1 environments * 150 benchmarks)
[  0.00%] · For scitools-iris commit 75dc6a1c <pr/trexfeathers/4378~1> (round 1/2):
[  0.00%] ·· Building for nox-conda-py3.8....
[  0.00%] ·· Benchmarking nox-conda-py3.8
[  0.17%] ··· Running (aux_factory.FactoryCommon.time_create--).............................
[  5.00%] ··· Running (cube.CellMeasure.time_add--)..........................................
[ 12.83%] ··· Running (import_iris.Iris.time_fileformats__ff--)..........................................................................
[ 25.00%] · For scitools-iris commit 415e90ad <pr/trexfeathers/4378> (round 1/2):
[ 25.00%] ·· Building for nox-conda-py3.8...
[ 25.00%] ·· Benchmarking nox-conda-py3.8
[ 25.17%] ··· Running (aux_factory.FactoryCommon.time_create--).......................................
[ 31.67%] ··· Running (cube.Cube.time_basic--).................................................................
[ 42.50%] ··· Running (import_iris.Iris.time_quickplot--)..............................................
[ 50.00%] · For scitools-iris commit 415e90ad <pr/trexfeathers/4378> (round 2/2):
[ 50.00%] ·· Benchmarking nox-conda-py3.8
[ 50.17%] ··· aux_factory.FactoryCommon.time_create                                               n/a
[ 50.33%] ··· aux_factory.FactoryCommon.time_return                                               n/a
[ 50.50%] ··· aux_factory.HybridHeightFactory.time_create                                 6.80±0.07μs
[ 50.67%] ··· aux_factory.HybridHeightFactory.time_return                                  82.4±0.4ns
...

The lines I'm concerned about:

· The virtualenv name was hashed to avoid being too long.
Is this expected/a problem?

·· Building 415e90ad <pr/trexfeathers/4378> for nox-conda-py3.8..
·· Installing 415e90ad <pr/trexfeathers/4378> into nox-conda-py3.8.
· Running 300 total benchmarks (2 commits * 1 environments * 150 benchmarks)
[  0.00%] · For scitools-iris commit 75dc6a1c <pr/trexfeathers/4378~1> (round 1/2):

This appears to be installing the environment from one commit and running the benchmarks from another. Either the log messages are wrong, or the environment is wrong.

[trexfeathers]

· The virtualenv name was hashed to avoid being too long.
Is this expected/a problem?

This has so far not been a problem. From memory the ASV team are concerned about path length for certain file systems, but nothing has come to light in all my testing on my own linux machine, using Cirrus CI and using GHA.

[jamesp]

Comments inline. Looks like a nice addition although maybe a little complicated for new contributors to get stuck into. Not saying this is something you need to change, sometimes complicated things are complicated.

You'll need to also add documentation on how to use the two new nox commands (I think I did it wrong by just doing nox -s benchmarks). Also an explanation of how nox and asv relate would be helpful and perhaps reduce the barrier to entry described above.

[trexfeathers]

·· Building 415e90ad <pr/trexfeathers/4378> for nox-conda-py3.8..
·· Installing 415e90ad <pr/trexfeathers/4378> into nox-conda-py3.8.
· Running 300 total benchmarks (2 commits * 1 environments * 150 benchmarks)
[  0.00%] · For scitools-iris commit 75dc6a1c <pr/trexfeathers/4378~1> (round 1/2):

This appears to be installing the environment from one commit and running the benchmarks from another. Either the log messages are wrong, or the environment is wrong.

ASV needs to set up an environment before it gets started. It's quite verbose about this:

·· Building 415e90ad <pr/trexfeathers/4378> for nox-conda-py3.8..
·· Installing 415e90ad <pr/trexfeathers/4378> into nox-conda-py3.8.

But once this is done and the commit-to-commit benchmarking has started, the messages become briefer:

[  0.00%] · For scitools-iris commit 75dc6a1c <pr/trexfeathers/4378~1> (round 1/2):
[  0.00%] ·· Building for nox-conda-py3.8....

[ 25.00%] · For scitools-iris commit 415e90ad <pr/trexfeathers/4378> (round 1/2):
[ 25.00%] ·· Building for nox-conda-py3.8...

From working with ASV I've learned that it's still doing the same build+install steps after checking out a given commit but before benchmarking it.

I don't know why it bothers with building and installing 415e90ad first in this instance, since it immediately starts with 75dc6a1c. I expect it's because that would be the right thing to do for asv run, whereas asv continuous is a feature that was added later. Perhaps not all the necessary changes were made to optimise for asv continuous.

[jamesp]

Ah yes, that rings a bell. I think that was the reason for the log message on L65 of conda_lock_plugin.py

[trexfeathers]

@jamesp thanks for your thorough review 👍

You'll need to also add documentation on how to use the two new nox commands (I think I did it wrong by just doing nox -s benchmarks). Also an explanation of how nox and asv relate would be helpful and perhaps reduce the barrier to entry described above.

I'll have to come back to this later in the week. Some thought will need to be given to exactly where benchmarking should be mentioned in the documentation, with correct formatting, references etcetera, and that's not something I can do with the time I have remaining today!

[jamesp]

OK, benchmark CI is passing with the new code which gives me confidence this is working as expected. You can do either of the following:

1. Add the docs to this PR.
2. Add an issue to the backlog to add benchmark docs.

Once done I'll merge.
Then I guess we need to update our nightly runner to use nox?

[trexfeathers]

Add an issue to the backlog to add benchmark docs.

I'd prefer this option. Not because I'm lazy¹, but because I consider this PR just another step towards an acceptable solution. There are still other problems I want to solve before I'd be happy officially sharing with the outside world (e.g. what to do when we want to incorporate large files into benchmarking). #4385

Then I guess we need to update our nightly runner to use nox?

Similar to the above, this is moving us closer to not having the offline overnight runner, so I'd prefer not to invest any resources into improving that, since said resources could continue to improve the 'online' runner.

---

¹ well maybe I am, but it's not relevant here