Skip to content

Minimise duplication of the CDDS default request items #349

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
zmaalick wants to merge 11 commits into
base: main
Choose a base branch
from
+103 −74
Open

Minimise duplication of the CDDS default request items #349

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
cd9177c
add a cfg file for default values
zmaalick Jan 20, 2026
87286c5
update copywrite
zmaalick Jan 20, 2026
521d817
update config list with sorting metadata and adding copywrite
zmaalick Jan 22, 2026
6d7edff
update copywrite
zmaalick Jan 22, 2026
7bcab81
fix the unittest runs
zmaalick Jan 22, 2026
8526711
update with precommit checks
zmaalick Jan 22, 2026
b947af1
Merge branch 'main' into 209_config_cdds_dafault_request_items
zmaalick Jan 29, 2026
76a5730
update miptable version
zmaalick Jan 29, 2026
ca8d9a0
fix the unittest and copywrite
zmaalick Jan 30, 2026
eac559f
sort the list
zmaalick Jan 30, 2026
231b16c
create global variable and remvoe the condition
zmaalick Feb 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 33 additions & 26 deletions CMEW/app/configure_standardise/bin/create_request_file.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,28 @@
import os
from pathlib import Path

DEFAULT_REQUEST_DEFAULTS_PATH = (
Copy link
Member

@ehogan ehogan Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minimise the use of DEFAULT in the name of this global variable:

Suggested change
DEFAULT_REQUEST_DEFAULTS_PATH = (
REQUEST_DEFAULTS_PATH = (

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


def load_defaults():
Copy link
Member

@ehogan ehogan Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Name the function to better reflect what the function is doing:

Suggested change
def load_defaults():
def load_request_defaults():

cfg = configparser.ConfigParser()

# Explicit override (tests or power users)
cfg_path = Path(
os.environ.get(
"REQUEST_DEFAULTS_CFG",
str(DEFAULT_REQUEST_DEFAULTS_PATH),
)
)

if not cfg_path.exists():
raise FileNotFoundError(f"Defaults file not found: {cfg_path}")

Comment on lines +18 to +29
Copy link
Member

@ehogan ehogan Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These lines aren't needed, this function should just load the request_defaults.cfg file:

Suggested change
# Explicit override (tests or power users)
cfg_path = Path(
os.environ.get(
"REQUEST_DEFAULTS_CFG",
str(DEFAULT_REQUEST_DEFAULTS_PATH),
)
)
if not cfg_path.exists():
raise FileNotFoundError(f"Defaults file not found: {cfg_path}")

cfg.read(cfg_path)
return cfg


def create_request():
"""Retrieve CDDS request information from Rose suite configuration.
Expand All @@ -17,54 +39,39 @@ def create_request():
configparser.ConfigParser()
CDDS request configuration.
"""

defaults = load_defaults()
start_year = int(os.environ["START_YEAR"])
end_year = int(os.environ["START_YEAR"]) + int(
os.environ["NUMBER_OF_YEARS"]
)
request = configparser.ConfigParser()
request["metadata"] = {
"base_date": "1850-01-01T00:00:00",
"branch_method": "no parent",
**defaults["metadata"],
"calendar": os.environ["CALENDAR"],
"experiment_id": "amip",
"institution_id": os.environ["INSTITUTION_ID"],
"license": "GCModelDev model data is licensed under the Open Government License v3 (https://www.nationalarchives.gov.uk/doc/open-government-licence/version/3/)", # noqa: E501
"mip": "ESMVal",
"mip_era": "GCModelDev",
"model_id": os.environ["MODEL_ID"],
"model_type": "AGCM AER",
"sub_experiment_id": "none",
"variant_label": os.environ["VARIANT_LABEL"],
}

request["common"] = {
"external_plugin": "",
"external_plugin_location": "",
**defaults["common"],
"mip_table_dir": os.path.expanduser(
"~cdds/etc/mip_tables/GCModelDev/0.0.25"
defaults["common"]["mip_table_dir"]
),
"mode": "relaxed",
"package": "round-1",
"root_proc_dir": os.environ["ROOT_PROC_DIR"],
"root_data_dir": os.environ["ROOT_DATA_DIR"],
"workflow_basename": os.environ["SUITE_ID"],
}
request["data"] = {
**defaults["data"],
"start_date": f"{start_year}-01-01T00:00:00",
"end_date": f"{end_year}-01-01T00:00:00",
"mass_data_class": "crum",
"model_workflow_branch": "trunk",
"model_workflow_id": os.environ["SUITE_ID"],
"model_workflow_revision": "not used except with data request",
"start_date": f"{os.environ['START_YEAR']}-01-01T00:00:00",
"streams": "apm",
"variable_list_file": os.environ["VARIABLES_PATH"],
}
request["misc"] = {
"atmos_timestep": "1200",
}
request["conversion"] = {
"mip_convert_plugin": "UKESM1",
"skip_archive": "True",
"cylc_args": "--no-detach -v",
}
request["misc"] = dict(defaults["misc"])
request["conversion"] = dict(defaults["conversion"])
return request


Expand Down
69 changes: 22 additions & 47 deletions CMEW/app/configure_standardise/bin/test_create_request_file.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,27 @@
# (C) Crown Copyright 2024-2026, Met Office.
# The LICENSE.md file contains full licensing details.
from create_request_file import DEFAULT_REQUEST_DEFAULTS_PATH
import os

from create_request_file import create_request
import configparser


def load_expected_cfg() -> dict:
Copy link
Member

@ehogan ehogan Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The following name would be more explicit (also, we're not using type hints in CMEW):

Suggested change
def load_expected_cfg() -> dict:
def expected_request():

"""Load expected values from default_request.cfg into a dict."""
Copy link
Member

@ehogan ehogan Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is doing more than loading values from the default_request.cfg file; it's returning a dictionary with the expected request:

Suggested change
"""Load expected values from default_request.cfg into a dict."""
"""Return the expected request."""


parser = configparser.ConfigParser()
parser.read(DEFAULT_REQUEST_DEFAULTS_PATH)
Comment on lines +12 to +13
Copy link
Member

@ehogan ehogan Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Call load_request_defaults here.


expected = {}
for section in parser.sections():
Copy link
Member

@ehogan ehogan Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use a more descriptive variable name:

Suggested change
for section in parser.sections():
for section in request_defaults.sections():

expected[section] = dict(parser.items(section))

# Match runtime behaviour
expected["common"]["mip_table_dir"] = os.path.expanduser(
expected["common"]["mip_table_dir"]
)

return expected


def test_create_request(monkeypatch):
Expand All @@ -22,50 +41,6 @@ def test_create_request(monkeypatch):
actual = {
section: dict(config.items(section)) for section in config.sections()
}
expected = {
"metadata": {
"branch_method": "no parent",
"calendar": "360_day",
"base_date": "1850-01-01T00:00:00",
"experiment_id": "amip",
"institution_id": "MOHC",
"license": "GCModelDev model data is licensed under the Open Government License v3 (https://www.nationalarchives.gov.uk/doc/open-government-licence/version/3/)", # noqa: E501
"mip": "ESMVal",
"mip_era": "GCModelDev",
"model_id": "UKESM1-0-LL",
"model_type": "AGCM AER",
"sub_experiment_id": "none",
"variant_label": "r1i1p1f1",
},
"common": {
"external_plugin": "",
"external_plugin_location": "",
"mip_table_dir": os.path.expanduser(
"~cdds/etc/mip_tables/GCModelDev/0.0.25"
),
"mode": "relaxed",
"package": "round-1",
"root_proc_dir": "/path/to/proc/dir/",
"root_data_dir": "/path/to/data/dir/",
"workflow_basename": "u-az513",
},
"data": {
"end_date": "1994-01-01T00:00:00",
"mass_data_class": "crum",
"model_workflow_branch": "trunk",
"model_workflow_id": "u-az513",
"model_workflow_revision": "not used except with data request",
"start_date": "1993-01-01T00:00:00",
"streams": "apm",
"variable_list_file": "/path/to/variables.txt",
},
"misc": {
"atmos_timestep": "1200",
},
"conversion": {
"mip_convert_plugin": "UKESM1",
"skip_archive": "True",
"cylc_args": "--no-detach -v",
},
}
expected = load_expected_cfg()

assert actual == expected
44 changes: 44 additions & 0 deletions CMEW/app/configure_standardise/etc/request_defaults.cfg
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
# (C) Crown Copyright 2026, Met Office.
# The LICENSE.md file contains full licensing details.
# request_defaults.cfg
Copy link
Member

@ehogan ehogan Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is not necessary:

Suggested change
# request_defaults.cfg

[metadata]
base_date = 1850-01-01T00:00:00
branch_method = no parent
calendar = 360_day
experiment_id = amip
institution_id = MOHC
license = GCModelDev model data is licensed under the Open Government License v3 (https://www.nationalarchives.gov.uk/doc/open-government-licence/version/3/)
mip = ESMVal
mip_era = GCModelDev
model_id = UKESM1-0-LL
model_type = AGCM AER
sub_experiment_id = none
variant_label = r1i1p1f1

[common]
external_plugin =
external_plugin_location =
Comment on lines +19 to +20
Copy link
Member

@ehogan ehogan Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to remove these lines, please (since they have no values)?

Suggested change
external_plugin =
external_plugin_location =

mip_table_dir = ~cdds/etc/mip_tables/GCModelDev/0.0.25
mode = relaxed
package = round-1
root_proc_dir = /path/to/proc/dir/
root_data_dir = /path/to/data/dir/
workflow_basename = u-az513

[data]
start_date = 1993-01-01T00:00:00
end_date = 1994-01-01T00:00:00
mass_data_class = crum
model_workflow_branch = trunk
model_workflow_id = u-az513
model_workflow_revision = not used except with data request
streams = apm
variable_list_file = /path/to/variables.txt

[misc]
atmos_timestep = 1200

[conversion]
mip_convert_plugin = UKESM1
skip_archive = True
cylc_args = --no-detach -v
5 changes: 4 additions & 1 deletion CMEW/app/configure_standardise/rose-app.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
# (C) Crown Copyright 2024-2025, Met Office.
# (C) Crown Copyright 2024-2026, Met Office.
# The LICENSE.md file contains full licensing details.

[command]
default=configure_standardise.sh

[file:$CYLC_WORKFLOW_SHARE_DIR/etc]
mode=mkdir

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

@NParsonsMO NParsonsMO Jan 21, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@zmaalick zmaalick Jan 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix the unittests: 7bcab81

Comment on lines +10 to +11
Copy link
Member

@ehogan ehogan Feb 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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? 🤔

Copy link
Collaborator Author

@zmaalick zmaalick Feb 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@ehogan ehogan Feb 6, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.