Minimise duplication of the CDDS default request items by zmaalick · Pull Request #349 · MetOffice/CMEW

zmaalick · 2026-01-20T09:57:46Z

Closes #209 .

PR creation checklist for the developer

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

Definition of Done for the developer

Does the change in this PR address the above issue / have all acceptance criteria been met?
Does the change in this PR follow the requirements in the wiki: Developer Guide (including copyrights)?
Have new tests related to the change been added?
Do all the GitHub workflow checks pass?
Do all the tests run locally and pass? (Note: the tests are not run by the GitHub workflow, see wiki: Run the tests locally)
Has the API documentation (e.g. docstrings in Python modules) related to the change been updated appropriately?
Has the user documentation (i.e. everything in the doc directory) related to the change been updated appropriately, including the Quick Start section?
Do the HTML pages render correctly? (See wiki: Build the documentation locally)

PR creation checklist for the reviewer

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

Definition of Done for the reviewer

Does the change in this PR address the above issue / have all acceptance criteria been met?
Does the change in this PR follow the requirements in the wiki: Developer Guide (including copyrights)?
Have new tests related to the change been added?
Do all the GitHub workflow checks pass?
Do all the tests run locally and pass? (Note: the tests are not run by the GitHub workflow, see wiki: Run the tests locally)
Has the API documentation (e.g. docstrings in Python modules) related to the change been updated appropriately?
Has the user documentation (i.e. everything in the doc directory) related to the change been updated appropriately, including the Quick Start section?
Do the HTML pages render correctly? (See wiki: Build the documentation locally)

NParsonsMO

The unit test for configure_standardise/bin/test_create_request_file.py::test_create_request fails.

I also think that the defaults file is not supposed to contain user-configurable variables.

CMEW/app/configure_standardise/etc/request_defaults.cfg

CMEW/app/configure_standardise/rose-app.conf

NParsonsMO · 2026-01-21T17:28:21Z

CMEW/app/configure_standardise/rose-app.conf

 mode=mkdir
+
+[file:$CYLC_WORKFLOW_SHARE_DIR/etc/request_defaults.cfg]
+source=${CYLC_WORKFLOW_RUN_DIR}/app/configure_standardise/etc/request_defaults.cfg


I hoped on seeing this that it might be a way round the unit test failures that I encountered in PR #324, but unfortunately it isn't, and it has a very similar error itself.
When I run pytest myself, all the tests pass fine. But when I do cylc vip -O metoffice -O unittest, I get a file not found error for the file above, ending as below:

cfg_path = base_dir / "etc" / "request_defaults.cfg" if not cfg_path.exists(): > raise FileNotFoundError(f"Defaults file not found: {cfg_path}") E FileNotFoundError: Defaults file not found: [MY_DIR]/CMEW/run3/share/etc/request_defaults.cfg configure_standardise/bin/create_request_file.py:25: FileNotFoundError

However here, the unit test also does not pass with the command cylc vip -O metoffice -O test (as opposed to unittest) either.

Fix the unittests: 7bcab81

NParsonsMO · 2026-01-21T17:34:41Z

CMEW/app/configure_standardise/bin/create_request_file.py

+        # Running inside Cylc
+        base_dir = Path(os.environ["CYLC_WORKFLOW_SHARE_DIR"])
+    else:
+        # Running under pytest / unittest / local execution


This doesn't seem to catch everything, see comment on rose-app.conf

Fixed it: 7bcab81

CMEW/app/configure_standardise/etc/request_defaults.cfg

NParsonsMO

There are merge conflicts that need to be sorted,

and I assume that this needs to be pre-review (although I'm not sure it's come up yet)?

I've left two comments open to check those lines on merging with main.

All unit tests now passing in all methods.

One copyright change requested.

CMEW/app/configure_standardise/etc/request_defaults.cfg

CMEW/app/configure_standardise/bin/create_request_file.py

CMEW/app/configure_standardise/etc/request_defaults.cfg

NParsonsMO · 2026-01-22T12:55:38Z

CMEW/app/configure_standardise/etc/request_defaults.cfg

@@ -0,0 +1,44 @@
+# (C) Crown Copyright 2022-2026, Met Office.


It's only 2026, not 2022-2026.
The year of first publication of the specific file:
https://github.com/MetOffice/CMEW/wiki/Detailed-Working-Practices#coding-requirements

This hasn't been addressed

NParsonsMO

One unit test is failing.

Two prior review comments have not been responded to.

NParsonsMO · 2026-01-29T13:35:04Z

CMEW/app/configure_standardise/etc/request_defaults.cfg

@@ -0,0 +1,44 @@
+# (C) Crown Copyright 2022-2026, Met Office.


This hasn't been addressed

CMEW/app/configure_standardise/etc/request_defaults.cfg

zmaalick · 2026-01-30T08:57:31Z

One unit test is failing.

Two prior review comments have not been responded to.

Fix the unittest: ca8d9a0

NParsonsMO

Thanks @zmaalick!

alistairsellar

Thanks @zmaalick!

ehogan

Thanks @zmaalick 🥳

ehogan · 2026-02-02T08:24:06Z

CMEW/app/configure_standardise/rose-app.conf

+[file:$CYLC_WORKFLOW_SHARE_DIR/etc/request_defaults.cfg]
+source=${CYLC_WORKFLOW_RUN_DIR}/app/configure_standardise/etc/request_defaults.cfg


Would it be possible to explain why making a copy of request_defaults.cfg is needed here? Is it not possible to access the contents directly from the original file? 🤔

Cylc runs workflows from a run directory, not the source tree. Copying request_defaults.cfg into share/etc makes the run self-contained, reproducible, and immune to source changes. Reading the original file directly would break provenance and reproducibility.

The location where the request_defaults.cfg file is being copied from ($CYLC_WORKFLOW_SHARE_DIR) is already located in the run directory, so there's no need to copy the file.

It should be possible to remove these lines following the proposed changes in my comment above 👍

CMEW/app/configure_standardise/bin/test_create_request_file.py

CMEW/app/configure_standardise/bin/create_request_file.py

ehogan

Thanks @zmaalick 👍

CMEW/app/configure_standardise/bin/create_request_file.py

CMEW/app/configure_standardise/bin/test_create_request_file.py

CMEW/app/configure_standardise/etc/request_defaults.cfg

CMEW/app/configure_standardise/bin/test_create_request_file.py

ehogan · 2026-02-09T16:14:21Z

CMEW/app/configure_standardise/bin/create_request_file.py

+REQUEST_DEFAULTS_PATH = (
+    Path(__file__).resolve().parents[1] / "etc" / "request_defaults.cfg"
+)


Thinking a bit more about this, and given much of the Python code in CMEW accesses information via environment variables defined in the Cylc workflow, would it be possible to define REQUEST_DEFAULTS_PATH in the root environment in the flow.cylc file, then use that below?

I acknowledge this goes against my previous comment, but I forgot we are already doing this in other places in CMEW 😞

I know there are multiple issues open to review environment variables, and I'm going to open another one to consider environment variables within Python scripts.

#363 opened 😊

ehogan · 2026-02-09T16:16:54Z

CMEW/app/configure_standardise/rose-app.conf

+[file:$CYLC_WORKFLOW_SHARE_DIR/etc/request_defaults.cfg]
+source=${CYLC_WORKFLOW_RUN_DIR}/app/configure_standardise/etc/request_defaults.cfg


It should be possible to remove these lines following the proposed changes in my comment above 👍

add a cfg file for default values

cd9177c

zmaalick self-assigned this Jan 20, 2026

zmaalick changed the title ~~add a cfg file for default values~~ Minimise duplication of the CDDS default request items Jan 20, 2026

zmaalick added enhancement New feature or request good first issue Good for newcomers technical debt Technical debt in CMEW standardise Anything related to CDDS quality assurance Anything related to Quality Assurance (QA) labels Jan 20, 2026

zmaalick added this to the v0.2.0 (multiple model runs) milestone Jan 20, 2026

update copywrite

87286c5

NParsonsMO self-requested a review January 21, 2026 16:29

NParsonsMO requested changes Jan 21, 2026

View reviewed changes

zmaalick added 4 commits January 22, 2026 08:33

update config list with sorting metadata and adding copywrite

521d817

update copywrite

6d7edff

fix the unittest runs

7bcab81

update with precommit checks

8526711

NParsonsMO requested changes Jan 22, 2026

View reviewed changes

zmaalick requested a review from NParsonsMO January 29, 2026 10:51

zmaalick and others added 2 commits January 29, 2026 13:31

Merge branch 'main' into 209_config_cdds_dafault_request_items

b947af1

update miptable version

76a5730

NParsonsMO requested changes Jan 29, 2026

View reviewed changes

fix the unittest and copywrite

ca8d9a0

sort the list

eac559f

zmaalick requested a review from NParsonsMO January 30, 2026 09:01

NParsonsMO removed the good first issue Good for newcomers label Jan 30, 2026

NParsonsMO approved these changes Jan 30, 2026

View reviewed changes

zmaalick requested a review from alistairsellar January 30, 2026 10:09

alistairsellar approved these changes Jan 30, 2026

View reviewed changes

ehogan requested changes Feb 2, 2026

View reviewed changes

create global variable and remvoe the condition

231b16c

zmaalick requested a review from ehogan February 2, 2026 11:20

ehogan requested changes Feb 6, 2026

View reviewed changes

updates

7320416

zmaalick requested a review from ehogan February 9, 2026 11:58

ehogan requested changes Feb 9, 2026

View reviewed changes

ehogan mentioned this pull request Feb 9, 2026

Use the default request items across CMEW #365

Open

NParsonsMO mentioned this pull request Feb 10, 2026

Enable user to specify CMIP runs in addition to model development runs #358

Open

34 tasks

add path info in flow.cylc

fc1a2e0

zmaalick requested a review from ehogan February 13, 2026 08:57

		@@ -0,0 +1,44 @@
		# (C) Crown Copyright 2022-2026, Met Office.

		[file:$CYLC_WORKFLOW_SHARE_DIR/etc/request_defaults.cfg]
		source=${CYLC_WORKFLOW_RUN_DIR}/app/configure_standardise/etc/request_defaults.cfg

Conversation

zmaalick commented Jan 20, 2026 • edited by alistairsellar 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!

NParsonsMO left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

NParsonsMO Jan 21, 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!

Uh oh!

NParsonsMO left a comment • 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!

Uh oh!

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!

NParsonsMO left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zmaalick commented Jan 30, 2026

Uh oh!

NParsonsMO left a comment

Choose a reason for hiding this comment

Uh oh!

alistairsellar left a comment

Choose a reason for hiding this comment

Uh oh!

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

ehogan Feb 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!

Uh oh!

Uh oh!

ehogan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zmaalick commented Jan 20, 2026 •

edited by alistairsellar

Loading

NParsonsMO Jan 21, 2026 •

edited

Loading

NParsonsMO left a comment •

edited

Loading

ehogan Feb 6, 2026 •

edited

Loading