Implement parametrisation for standardising model runs by mo-nikosbaltas · Pull Request #392 · MetOffice/CMEW

mo-nikosbaltas · 2026-03-02T10:47:37Z

Closes #338

PR creation checklist for the developer

The <issue_number> above ☝️ has been replaced with the issue number.
main has been selected as the base branch.
The feature branch name follows the format <issue_number>_<short_description_of_feature>.
The text of the PR title exactly matches with the text (not including the issue number) of the issue title.
Appropriate reviewers have been added to the PR (once it is ready for review).
The PR has been assigned to the developer(s).
The same labels as on the issue (except for the good first issue label) have been added to the PR.
The Climate Model Evaluation Workflow (CMEW) project has been added to the PR.
The appropriate milestone has been added to the PR.

Definition of Done for the developer

PR creation checklist for the reviewer

The <issue_number> above ☝️ has been replaced with the issue number.
main has been selected as the base branch.
The feature branch name follows the format <issue_number>_<short_description_of_feature>.
The text of the PR title exactly matches with the text (not including the issue number) of the issue title.
Appropriate reviewers have been added to the PR (once it is ready for review).
The PR has been assigned to the developer(s).
The same labels as on the issue (except for the good first issue label) have been added to the PR.
The Climate Model Evaluation Workflow (CMEW) project has been added to the PR.
The appropriate milestone has been added to the PR.

Definition of Done for the reviewer

Important

Remember to re-check the Definition of Done after making changes in response to a review.
The developer merges the PR.
Remember to use the format #<pull_request_number>: <pull_request_title> when writing the merge commit message for the pull request, so the pull request number is immediately visible on GitHub, regardless of the length of the pull request title.

CMEW/app/configure_standardise/bin/create_request_file.py

zmaalick

All good, nothing to add

NParsonsMO

I remember @ehogan saying that this PR should make a variables file per dataset, which is not currently happening.

I've also queried a couple of other bits.

NParsonsMO · 2026-03-05T14:56:07Z

CMEW/app/restructure_dirs/rose-app.conf

I can believe it was necessary to pull this step out into its own app (if it just needs running once overall rather than separately for each dataset), so if @ehogan is happy with the addition of a new app(?) then this seems fine.

The extra API (restrucre_dirs) is required because it acts as the fan-in api for synchronisation. We have two instances of standardise_model_data running in parallel and unless they both finish you cannot restructure the data. I had discussed this with Alistair as well.

There may be a way to run restructure_for_cmip6 in a way that can be done in parallel, but I propose that any investigation related to this is deferred. The name of the app should be changed, but I proposed that this is also deferred (to #318).

Following the agreement we made at the technical meeting this morning to revert configure_standardise.sh, restructure_dirs.sh should be reverted to this previous version.

Done. Although not different it had only some useful logging which have been removed. 7d8aef1

The version of restructure_dirs.sh in this PR has not been reverted to this previous version. Please revert the changes to restructure_dirs.sh so that it matches this previous version.

I removed all comments and checks and reverted back to the serial version. f889818

NParsonsMO · 2026-03-05T15:11:06Z

CMEW/app/standardise_model_data/rose-app.conf

@@ -1,4 +1,4 @@
-# (C) Crown Copyright 2024-2025, Met Office.
+# (C) Crown Copyright 2024-2026, Met Office.


It doesn't look like this file has changed, so the copyright shouldn't be updated.

This has now changed.

NParsonsMO · 2026-03-05T15:18:53Z

CMEW/app/configure_standardise/bin/configure_standardise.sh

+fi

 # ---------------------------------------------------------------------------
 # 1. Create variables.txt once (shared by both runs)


I think we are meant to make two now, not one shared one.

We had changed our minds!!! Perhaps you missed that. Not allowed to have a separate 'create_variables' app.

Would it not just be a case of making a variables_<dataset>.txt filepath akin to REQUEST_PATH = ${CYLC_WORKFLOW_SHARE_DIR}/etc/request_%(dataset)s.cfg while keeping it in the same (combined) app?

As discussed at the technical meeting this morning, the reason we didn't want to introduce a create_variables app that created a single variables file is because it's possible for the same variables to come from different streams depending on the model evaluation run (/ dataset) used, which means a variables file is required for each dataset. Keeping the variables file creation within configure_standardise is preferred, since it minimises the number of changes compared to the previous version of the code. Edited to add that since configure_standardise is running in parallel, each job should write its own variables file, rather than try to write a single file with the same name, which is not safe.

Keep it as it is.

All defensive programming and logging have been removed.

All these have been resolved 7d8aef1

Done it. 79e6324

NParsonsMO · 2026-03-05T15:27:26Z

CMEW/flow.cylc

+        [[[environment]]]
+            ROSE_TASK_APP = standardise_model_data
+            SITE = {{ SITE }}
+            RUN_LABEL = %(dataset)s


It doesn't seem good to define these things (RUN_LABEL and REQUEST_PATH) twice (once in configure_standardise<recipe,dataset> and once in standardise_model_data) - could you just define them once then inherit?

Also, please use the appropriate Cylc variable for the parameterisation variable, i.e. ${CYLC_TASK_PARAM_dataset} rather than %(dataset)s. An example of this for the recipe parameterised variable is available at https://github.com/MetOffice/CMEW/blob/main/CMEW/flow.cylc#L51.

Would it be possible to use only ${CYLC_TASK_PARAM_dataset} in favour of ${RUN_LABEL}, please? (Using ${RUN_LABEL} obfuscates the recognisable and understandable variable ${CYLC_TASK_PARAM_dataset})

Done f889818

NParsonsMO · 2026-03-05T15:53:43Z

CMEW/app/configure_standardise/bin/create_request_file.py


-def create_request():
-    """Retrieve CDDS request information from Rose suite configuration.
+def create_request() -> configparser.ConfigParser:


I note from an earlier comment of Emma's on another review that CMEW should not contain type hints:
#349 (comment)

I'm not sure why or if that's an absolute, again probably one for @ehogan.

Should there still be a docstring?

Python functions must have docstrings. Documentation requirements: Python documentation provides more information.

Type hinting has been removed and docstrings reinstated. docstrings were removed after previous changes were made.

All type hinting and the inclusion of docstrings had been done 7d8aef1

CMEW/app/configure_standardise/bin/create_request_file.py

ehogan · 2026-03-06T13:18:20Z

CMEW/app/configure_standardise/bin/configure_standardise.sh

 BASH_XTRACEFD=1
 set -xeuo pipefail

-echo "[INFO] Running configure_standardise for REF and EVAL runs"


At the technical meeting this morning, we agreed that configure_standardise.sh should be reverted to this previous version. Specifically:

-o pipefail is only needed in rose-app.conf files, as described in the Developer Guide: Rose requirements

The echoing and defensive programming is not required; the options provided to set at the top of the script do the following:

-x Print commands and their arguments as they are executed.

-e Exit immediately if a command exits with a non-zero status.

-u Treat unset variables as an error when substituting.

If we want to validate variables, this should be done in the flow.cylc file, e.g. https://github.com/MetOffice/CMEW/blob/main/CMEW/flow.cylc#L5, and not in an individual app. However, this is out of scope of this issue (a new issue should be opened should we want to validate variables)

Done. 7d8aef1

The version of configure_standardise.sh in this PR has not been reverted to this previous version. Please revert the changes to configure_standardise.sh so that it matches this previous version.

Basically removed some useful comments and checks. The only diffrence is that the create_variables_file.py script has to be executed before create_request_file.py otherwise it breaks. f889818

ehogan · 2026-03-06T13:26:32Z

I remember @ehogan saying that this PR should make a variables file per dataset, which is not currently happening.

As discussed in the technical meeting this morning, configure_standardise must produce a variables file for each dataset, i.e. variables_u-bv526.txt and variables_u-cw673.txt.

ehogan · 2026-03-06T13:44:02Z

CMEW/app/standardise_model_data/rose-app.conf

@@ -1,4 +1,4 @@
-# (C) Crown Copyright 2024-2025, Met Office.
+# (C) Crown Copyright 2024-2026, Met Office.


Following the agreement we made at the technical meeting this morning to revert configure_standardise.sh, the rose-app.conf file for standardise_model_data should be reverted to this previous version and the standardise_model_data.sh script removed.

It has 7d8aef1

The version of standardise_model_data/rose-app.conf in this PR has not been reverted to this previous version. Please revert the changes to standardise_model_data/rose-app.conf so that it matches this previous version, and then remove the standardise_model_data.sh script.

standardise_model_data tasks run in parallel and they are standalone tasks so rose-app.conf had to be changed. You cannot revert to the serial version.

CMEW/flow.cylc

mo-nikosbaltas

Making comment visible.

mo-nikosbaltas · 2026-03-06T14:58:06Z

CMEW/app/restructure_dirs/rose-app.conf

Done. Although not different it had only some useful logging which have been removed. 7d8aef1

CMEW/flow.cylc

mo-nikosbaltas · 2026-03-06T17:23:08Z

CMEW/app/configure_standardise/bin/configure_standardise.sh

+fi

 # ---------------------------------------------------------------------------
 # 1. Create variables.txt once (shared by both runs)


All these have been resolved 7d8aef1

mo-nikosbaltas · 2026-03-06T17:26:44Z

CMEW/app/configure_standardise/bin/create_request_file.py


-def create_request():
-    """Retrieve CDDS request information from Rose suite configuration.
+def create_request() -> configparser.ConfigParser:


All type hinting and the inclusion of docstrings had been done 7d8aef1

mo-nikosbaltas · 2026-03-06T17:31:22Z

CMEW/app/standardise_model_data/rose-app.conf

@@ -1,4 +1,4 @@
-# (C) Crown Copyright 2024-2025, Met Office.
+# (C) Crown Copyright 2024-2026, Met Office.


It has 7d8aef1

mo-nikosbaltas · 2026-03-06T18:42:43Z

CMEW/app/configure_standardise/bin/configure_standardise.sh

+fi

 # ---------------------------------------------------------------------------
 # 1. Create variables.txt once (shared by both runs)


Done it. 79e6324

mo-nikosbaltas · 2026-03-09T10:42:22Z

CMEW/app/standardise_model_data/rose-app.conf

@@ -1,4 +1,4 @@
-# (C) Crown Copyright 2024-2025, Met Office.
+# (C) Crown Copyright 2024-2026, Met Office.


This has now changed.

NParsonsMO

I'm happy that my concerns have now been addressed.

I've left one comment on a copyright that I think now needs updating.

NParsonsMO · 2026-03-10T08:37:10Z

CMEW/app/restructure_dirs/bin/restructure_dirs.sh

@@ -1,18 +1,11 @@
 #!/bin/bash
-# (C) Crown Copyright 2024-2026, Met Office.
+# (C) Crown Copyright 2024-2025, Met Office.


Oh, now it has changed... So maybe it should now be 2026?

Done. e21f4fe

NParsonsMO

All good!

Approved.

mo-nikosbaltas added 2 commits March 2, 2026 09:55

added parameterisation to the workflow

6bd818e

added parameterisation to the workflow

521541c

mo-nikosbaltas added this to the v0.2.0 (multiple model runs) milestone Mar 2, 2026

mo-nikosbaltas requested a review from zmaalick March 2, 2026 10:47

mo-nikosbaltas added enhancement New feature or request standardise Anything related to CDDS labels Mar 2, 2026

mo-nikosbaltas changed the title ~~implement parallel parameterised ref eval multi run standardisation~~ Implement parametrisation for standardising model runs Mar 2, 2026

added parameterisation to the workflow

e97b950

zmaalick assigned zmaalick and mo-nikosbaltas and unassigned zmaalick Mar 3, 2026

alistairsellar requested changes Mar 3, 2026

View reviewed changes

CMEW/app/configure_standardise/bin/create_request_file.py Outdated Show resolved Hide resolved

NParsonsMO mentioned this pull request Mar 3, 2026

Create datasets parameterisation list from rose-suite.conf[model_runs] sections #395

Open

mo-nikosbaltas added 3 commits March 3, 2026 17:35

some minor changes for parametrisation

2e0f9e8

changes strictly for parametrisation of two-model runs

274e9be

changes strictly for parametrisation of two-model runs

a27442c

zmaalick approved these changes Mar 5, 2026

View reviewed changes

NParsonsMO self-requested a review March 5, 2026 14:32

NParsonsMO requested changes Mar 5, 2026

View reviewed changes

ehogan requested changes Mar 6, 2026

View reviewed changes

ehogan reviewed Mar 6, 2026

View reviewed changes

CMEW/flow.cylc Outdated Show resolved Hide resolved

mo-nikosbaltas added 2 commits March 6, 2026 16:22

updates following PR reviews

7d8aef1

creates two (identical) variables files

79e6324

mo-nikosbaltas commented Mar 9, 2026

View reviewed changes

NParsonsMO requested changes Mar 10, 2026

View reviewed changes

changed copyright year

e21f4fe

NParsonsMO approved these changes Mar 10, 2026

View reviewed changes

merged with main and resolved conflicts

acc264d

alistairsellar approved these changes Mar 10, 2026

View reviewed changes

mo-nikosbaltas requested a review from ehogan March 10, 2026 16:51

NParsonsMO mentioned this pull request Mar 12, 2026

Make extracted raw data location configurable #378

Open

50 tasks

changes following PR review; not affecting the code

f889818

zmaalick requested review from ehogan and removed request for ehogan March 16, 2026 13:36

NParsonsMO mentioned this pull request Mar 16, 2026

Remove hard-coding of ESMValTool developer configuration file #401

Open

50 tasks

		@@ -1,4 +1,4 @@
		# (C) Crown Copyright 2024-2025, Met Office.
		# (C) Crown Copyright 2024-2026, Met Office.

Conversation

mo-nikosbaltas commented Mar 2, 2026 • edited by ehogan Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR creation checklist for the developer

Definition of Done for the developer

PR creation checklist for the reviewer

Definition of Done for the reviewer

Uh oh!

Uh oh!

zmaalick left a comment

Choose a reason for hiding this comment

Uh oh!

NParsonsMO left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ehogan Mar 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mo-nikosbaltas Mar 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mo-nikosbaltas commented Mar 2, 2026 •

edited by ehogan

Loading

ehogan Mar 6, 2026 •

edited

Loading

mo-nikosbaltas Mar 6, 2026 •

edited

Loading