Move benchmark code from nox to bm_runner #361

stephenworsley · 2024-05-09T08:28:25Z

Closes #281

codecov · 2024-05-09T08:31:39Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.85%. Comparing base (fa3541b) to head (5e51f86).

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #361   +/-   ##
=======================================
  Coverage   98.85%   98.85%           
=======================================
  Files          36       36           
  Lines        3855     3855           
=======================================
  Hits         3811     3811           
  Misses         44       44

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

for more information, see https://pre-commit.ci

* origin/bm_runner: [pre-commit.ci] auto fixes from pre-commit.com hooks

pp-mo · 2024-05-13T14:26:20Z

@stephenworsley requested a review from @pp-mo

BTW I am actively reviewing this.
Just taking a while due to distractions + necessary learning..

for more information, see https://pre-commit.ci

* origin/bm_runner: [pre-commit.ci] auto fixes from pre-commit.com hooks

pp-mo

I've checked out the function of this now, and I'm happy it does function and contain what we need.
Just a few irrelevant references to fix I think (since you already ditched the unwanted Cperf code)

However, getting to grips with this has made me quite concerned about the future relationship of all this to the Iris benchmarking code, which it is similar to but significantly different.

for instance, I'm pleased that we didn't change noxfile.py here (i.e. in this PR), but I'm pretty concerned at how that relates to the Iris code : a lot in common but multiple significant differences
on the other hand, the change here to asv_delegated_conda.py just keeps it exactly the same as in Iris, so that is easy to track + could be done with simple "templating" as recently introduced into scitools/.github.

The task of generalising so these two implementations can "share and differ as required" seems attractive but is clearly already non-trivial. So, I'm hoping we can come up with better ways to do this in future, and apply it here, but not now !

pp-mo · 2024-05-16T15:47:25Z

benchmarks/bm_runner.py

+        _echo("Installing Mule into data generation environment ...")
+        mule_dir = data_gen_python.parents[1] / "resources" / "mule"
+        if not mule_dir.is_dir():
+            _subprocess_runner(
+                [
+                    "git",
+                    "clone",
+                    "https://github.com/metomi/mule.git",
+                    str(mule_dir),
+                ]
+            )
+        _subprocess_runner(
+            [
+                str(data_gen_python),
+                "-m",
+                "pip",
+                "install",
+                str(mule_dir / "mule"),
+            ]
+        )


I don't know if you actually care about this, but I suspect this bit is irrelevant to the iris-esmf-regrid benchmarking (I haven't checked it though).
IIUC it is needed in Iris because the "Cperf" benchmarks use mule.

pp-mo · 2024-05-16T15:48:52Z

benchmarks/bm_runner.py

+        _asv_compare(merge_base, head_sha)
+
+
+class _CSPerf(_SubParserGenerator, ABC):


Since we don't have a Cperf, this could maybe usefully all be merged into the 'Sperf' class instead.

I was aiming for similarity with the iris structure where possible, though I agree that it doesn't make much sense in tis instance.

pp-mo

Unfortunately I spotted a couple more problems which I think need fixing.
I did check local operation, and it does work 👍

pp-mo · 2024-05-20T13:44:38Z

benchmarks/bm_runner.py

+        "Run the on-demand Sperf suite of benchmarks (part of the UK Met "
+        "Office NG-VAT project) for the ``HEAD`` of ``upstream/main`` only, "


Whoops, I only just spotted that this description is out-of-date.
The scalability benchmarks are not really anything to do with NGVAT anymore, I think ?
Not sure how I would describe them. hopefully you can come up with something?

pp-mo · 2024-05-20T13:49:40Z

benchmarks/bm_runner.py

+            message = f"Input 'publish directory' is not a directory: {publish_dir}"
+            raise NotADirectoryError(message)
+        publish_subdir = (
+            publish_dir / f".*Scalability.*_{datetime.now().strftime('%Y%m%d_%H%M%S')}"


Suggested change

publish_dir / f".*Scalability.*_{datetime.now().strftime('%Y%m%d_%H%M%S')}"

publish_dir / f"sperf_{datetime.now().strftime('%Y%m%d_%H%M%S')}"

Probably cut+paste error ?

pp-mo · 2024-05-20T13:50:06Z

benchmarks/bm_runner.py

+        # Print completion message.
+        location = BENCHMARKS_DIR / ".asv"
+        _echo(
+            f'New ASV results for ".*Scalability.*".\n'


Suggested change

f'New ASV results for ".*Scalability.*".\n'

'New ASV results for "sperf".\n'

Probably cut+paste error ?

pp-mo · 2024-05-20T15:15:10Z

Great stuff @stephenworsley -- thanks for your patience !

* main: Move benchmark code from nox to bm_runner (SciTools-incubator#361) Bump scitools/workflows from 2024.01.0 to 2024.04.3 (SciTools-incubator#356) Bump peter-evans/create-pull-request from 5.0.2 to 6.0.5 (SciTools-incubator#355) [pre-commit.ci] pre-commit autoupdate (SciTools-incubator#336) Updated environment lockfiles (SciTools-incubator#344)

* main: Move benchmark code from nox to bm_runner (SciTools-incubator#361) Bump scitools/workflows from 2024.01.0 to 2024.04.3 (SciTools-incubator#356) Bump peter-evans/create-pull-request from 5.0.2 to 6.0.5 (SciTools-incubator#355) [pre-commit.ci] pre-commit autoupdate (SciTools-incubator#336) Updated environment lockfiles (SciTools-incubator#344) # Conflicts: # CHANGELOG.md

move benchmark code from nox to bm_runner

715c9e5

stephenworsley and others added 8 commits May 9, 2024 09:35

fix benchmark.yml

b2c93ea

move bm_runner.py

3336bba

keep benchmark python version consistent

6dcf83f

fix for asv with latest conda

eab39cc

flake 8, remove old code

7405b66

[pre-commit.ci] auto fixes from pre-commit.com hooks

646e481

for more information, see https://pre-commit.ci

docs changes

41277dc

Merge remote-tracking branch 'origin/bm_runner' into bm_runner

5743a44

* origin/bm_runner: [pre-commit.ci] auto fixes from pre-commit.com hooks

stephenworsley marked this pull request as ready for review May 9, 2024 13:27

stephenworsley requested a review from pp-mo May 13, 2024 14:23

pp-mo self-assigned this May 13, 2024

stephenworsley and others added 4 commits May 15, 2024 12:58

fix sperf

23413a9

[pre-commit.ci] auto fixes from pre-commit.com hooks

1e7651c

for more information, see https://pre-commit.ci

update benchmark README

3d9d75b

Merge remote-tracking branch 'origin/bm_runner' into bm_runner

58e407d

* origin/bm_runner: [pre-commit.ci] auto fixes from pre-commit.com hooks

pp-mo requested changes May 16, 2024

View reviewed changes

address review comments

e7cce70

pp-mo requested changes May 20, 2024

View reviewed changes

address review comments

5e51f86

pp-mo approved these changes May 20, 2024

View reviewed changes

pp-mo merged commit a50b2b8 into SciTools-incubator:main May 20, 2024
16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move benchmark code from nox to bm_runner #361

Move benchmark code from nox to bm_runner #361

stephenworsley commented May 9, 2024

codecov bot commented May 9, 2024 •

edited

pp-mo commented May 13, 2024 •

edited

pp-mo left a comment •

edited

pp-mo May 16, 2024

pp-mo May 16, 2024

stephenworsley May 17, 2024

pp-mo left a comment

pp-mo May 20, 2024

pp-mo May 20, 2024

pp-mo May 20, 2024

pp-mo commented May 20, 2024

		_asv_compare(merge_base, head_sha)


		class _CSPerf(_SubParserGenerator, ABC):

		"Run the on-demand Sperf suite of benchmarks (part of the UK Met "
		"Office NG-VAT project) for the ``HEAD`` of ``upstream/main`` only, "

	publish_dir / f".Scalability._{datetime.now().strftime('%Y%m%d_%H%M%S')}"
	publish_dir / f"sperf_{datetime.now().strftime('%Y%m%d_%H%M%S')}"

	f'New ASV results for ".Scalability.".\n'
	'New ASV results for "sperf".\n'

Move benchmark code from nox to bm_runner #361

Move benchmark code from nox to bm_runner #361

Conversation

stephenworsley commented May 9, 2024

codecov bot commented May 9, 2024 • edited

Codecov Report

pp-mo commented May 13, 2024 • edited

pp-mo left a comment • edited

Choose a reason for hiding this comment

pp-mo May 16, 2024

Choose a reason for hiding this comment

pp-mo May 16, 2024

Choose a reason for hiding this comment

stephenworsley May 17, 2024

Choose a reason for hiding this comment

pp-mo left a comment

Choose a reason for hiding this comment

pp-mo May 20, 2024

Choose a reason for hiding this comment

pp-mo May 20, 2024

Choose a reason for hiding this comment

pp-mo May 20, 2024

Choose a reason for hiding this comment

pp-mo commented May 20, 2024

codecov bot commented May 9, 2024 •

edited

pp-mo commented May 13, 2024 •

edited

pp-mo left a comment •

edited