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.

Zubair Maalick (zmaalick)

All good, nothing to add

Naomi Parsons (NParsonsMO)

I remember Emma Hogan (@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.

Naomi Parsons (NParsonsMO) · 2026-03-05T14:56:07Z

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 Emma Hogan (@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

Naomi Parsons (NParsonsMO) · 2026-03-05T15:18:53Z

+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

Naomi Parsons (NParsonsMO) · 2026-03-05T15:27:26Z

+        [[[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

Naomi Parsons (NParsonsMO) · 2026-03-05T15:53:43Z


-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 Emma Hogan (@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

Emma Hogan (ehogan) · 2026-03-06T13:18:20Z

 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

Emma Hogan (ehogan) · 2026-03-06T13:26:32Z

I remember Emma Hogan (@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.

Emma Hogan (ehogan) · 2026-03-06T13:44:02Z

@@ -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.

mo-nikosbaltas

Making comment visible.

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

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

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

+fi

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


All these have been resolved 7d8aef1

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


-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

@@ -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

+fi

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


Done it. 79e6324

Naomi Parsons (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.

Naomi Parsons (NParsonsMO)

All good!

Approved.

Swaps create_variables and create_request, relative to last commit

And remove script

(to retain the file history)

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 Zubair Maalick (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

Zubair Maalick (zmaalick) assigned Zubair Maalick (zmaalick) and mo-nikosbaltas and unassigned Zubair Maalick (zmaalick) Mar 3, 2026

Alistair Sellar (alistairsellar) requested changes Mar 3, 2026

View reviewed changes

Comment thread CMEW/app/configure_standardise/bin/create_request_file.py Outdated

Naomi Parsons (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

Zubair Maalick (zmaalick) approved these changes Mar 5, 2026

View reviewed changes

Naomi Parsons (NParsonsMO) self-requested a review March 5, 2026 14:32

Naomi Parsons (NParsonsMO) requested changes Mar 5, 2026

View reviewed changes

Emma Hogan (ehogan) requested changes Mar 6, 2026

View reviewed changes

Emma Hogan (ehogan) reviewed Mar 6, 2026

View reviewed changes

Comment thread CMEW/flow.cylc Outdated

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

Naomi Parsons (NParsonsMO) requested changes Mar 10, 2026

View reviewed changes

Comment thread CMEW/app/restructure_dirs/bin/restructure_dirs.sh Outdated

changed copyright year

e21f4fe

Naomi Parsons (NParsonsMO) approved these changes Mar 10, 2026

View reviewed changes

merged with main and resolved conflicts

acc264d

Alistair Sellar (alistairsellar) approved these changes Mar 10, 2026

View reviewed changes

mo-nikosbaltas requested a review from Emma Hogan (ehogan) March 10, 2026 16:51

Naomi Parsons (NParsonsMO) mentioned this pull request Mar 12, 2026

Make extracted raw data location configurable #378

Merged

50 tasks

changes following PR review; not affecting the code

f889818

Zubair Maalick (zmaalick) requested review from Emma Hogan (ehogan) and removed request for Emma Hogan (ehogan) March 16, 2026 13:36

Naomi Parsons (NParsonsMO) mentioned this pull request Mar 16, 2026

Remove hard-coding of ESMValTool developer configuration file #401

Merged

50 tasks

Alistair Sellar (alistairsellar) added 5 commits April 17, 2026 09:58

Return configure_standardisse.sh to previous version

64cef42

Swaps create_variables and create_request, relative to last commit

Move cdds_convert from script back into rose-app.conf

116bdc0

And remove script

Delete new file to facilitate move of old one

9cfc2c7

(to retain the file history)

Move restructure_dirs.sh to new app

3681c1b

Remove pipefail

3c5ee7b

Alistair Sellar (alistairsellar) assigned Alistair Sellar (alistairsellar) and unassigned mo-nikosbaltas Apr 17, 2026

Alistair Sellar (alistairsellar) added 3 commits April 17, 2026 12:10

Tidy docstrings and comments

54a3f00

Change python variable to be closer to env var

48821b5

Back to original ConfigParser (change out of scope)

093ebe7

		@@ -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!

Zubair Maalick (zmaalick) left a comment

Choose a reason for hiding this comment

Uh oh!

Naomi Parsons (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!

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!

Emma Hogan (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!

Emma Hogan (ehogan) commented Mar 6, 2026

Uh oh!

Choose a reason for hiding this comment

mo-nikosbaltas commented Mar 2, 2026 •

edited by ehogan

Loading

Emma Hogan (ehogan) Mar 6, 2026 •

edited

Loading

mo-nikosbaltas Mar 6, 2026 •

edited

Loading

Naomi Parsons (NParsonsMO) left a comment •

edited

Loading