Compare two model development runs in a single run of CMEW #259

mo-johnmo · 2025-03-25T17:13:40Z

Closes #142.

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)

…el2.conf and remove its MODELS_OPTION from other files. Move configure_for rose-app.conf to a bash script.

…erence variant labels.

app/configure_for/update_recipe_file.py as they are no longer needed. Revert to using environment variables.

alistairsellar

Partial review...

alistairsellar · 2025-03-27T17:23:12Z

CMEW/app/configure_for/bin/update_recipe_file.py

+    first_dataset.update(
+        {
+            "start_year": start_year,
+            "ensemble": variant_label_reference,
+            "end_year": end_year,
+        }
+    )


first_dataset will need to have the same keys updated as second_dataset (below). so project, exp & activity need added. exp should be hard-coded to "historical" for first_dataset.

These changes and others as discussed in Teams, have been made in commit b04203b.

alistairsellar

Hi @mo-johnmo. It's looking good. I've got a couple of questions. Can we discuss online after 2:15?

alistairsellar · 2025-03-28T12:33:31Z

CMEW/app/configure_for/bin/test_update_recipe_file.py

+g_variant_label = "r1i1p1f1"  # = ensemble test
+g_variant_label_reference = "r1i1p1f3"  # = ensemble reference


What does the g_ prefix indicate in these variable names?

...which means these global variables are not needed.

alistairsellar · 2025-03-28T12:54:24Z

CMEW/app/configure_for/bin/update_recipe_file.py



-def main():
+def update(recipe_path, variant_label, variant_label_reference):


The additional layer of function seems unnecessary. Is there a reason not to handle the two new vars in the same way as recipe_path was treated before in main()?

Thanks for the explanation on our call. Since we don't call this intermediate function at the moment, please to the previous structure, with update_recipe called directly by main

alistairsellar · 2025-03-28T12:57:19Z

CMEW/app/configure_for/bin/test_update_recipe_file.py

 def mock_env_vars(monkeypatch):
    monkeypatch.setenv("START_YEAR", "1993")
    monkeypatch.setenv("NUMBER_OF_YEARS", "1")
+    monkeypatch.setenv("VARIANT_LABEL", g_variant_label)


Why set these with variables instead of hard-coding like the YEAR variables?

Now we've decided to feed these to update_recipe as environment variables, these can be specified in the same way as START_YEAR etc....

alistairsellar · 2025-03-28T13:31:23Z

CMEW/app/configure_standardise/bin/create_request_file.py

        json.dump(request, file, separators=(",\n", ": "))


+def make_request_file(


Similar to above, is there a strong reason to introduce an extra level to the function hierarchy?

Since we only want to test create_request, we don't need this additional layer. Please revert to calling create_request and write_request directly from main

… while retaining the variant_label and variant_label_reference additions.

alistairsellar

Hi @mo-johnmo. Here are the changes we discussed.

Plus confirmation on the question I said I'd get a second opinion on, and a couple of new requests about the argparse stuff.

alistairsellar · 2025-03-28T14:52:48Z

CMEW/app/configure_for/bin/update_recipe_file.py



-def update_recipe(recipe_path):
+def update_recipe(recipe_path, variant_label, variant_label_reference):


Thanks for discussion just now. We decided to feed these variables to update_recipe as environment variables, for consistency with START_YEAR etc.

alistairsellar · 2025-03-28T14:54:44Z

CMEW/app/configure_for/bin/update_recipe_file.py



-def main():
+def update(recipe_path, variant_label, variant_label_reference):


Thanks for the explanation on our call. Since we don't call this intermediate function at the moment, please to the previous structure, with update_recipe called directly by main

alistairsellar · 2025-03-28T14:57:31Z

CMEW/app/configure_for/bin/test_update_recipe_file.py

 def mock_env_vars(monkeypatch):
    monkeypatch.setenv("START_YEAR", "1993")
    monkeypatch.setenv("NUMBER_OF_YEARS", "1")
+    monkeypatch.setenv("VARIANT_LABEL", g_variant_label)


Now we've decided to feed these to update_recipe as environment variables, these can be specified in the same way as START_YEAR etc....

alistairsellar · 2025-03-28T14:57:53Z

CMEW/app/configure_for/bin/test_update_recipe_file.py

+g_variant_label = "r1i1p1f1"  # = ensemble test
+g_variant_label_reference = "r1i1p1f3"  # = ensemble reference


...which means these global variables are not needed.

alistairsellar · 2025-03-28T15:06:54Z

CMEW/app/configure_standardise/bin/test_create_request_file.py

+    # noqa : 36 : Ignore warning about monkey_env not being used; it is used
+    # silently but not referenced.
+
+    request = make_request_file(


Please just test the create_request function by comparing dicts as before. Second reviewer @ehogan agrees that we don't want to extend the scope of the test in this PR.

A bit more detail in case of interest... we have end-to-end tests to cover the interactions between functions including files. The unit test is to test the smallest sensible unit, and tests the logical content is correct.

alistairsellar · 2025-03-28T15:09:15Z

CMEW/app/configure_standardise/bin/create_request_file.py

        json.dump(request, file, separators=(",\n", ": "))


+def make_request_file(


Since we only want to test create_request, we don't need this additional layer. Please revert to calling create_request and write_request directly from main

alistairsellar · 2025-03-28T15:24:20Z

CMEW/app/configure_standardise/bin/create_request_file.py

+    # Get args from cmd line.  This needs to be told which of test or reference
+    # settings to use, it cannot decide itself.
+    # Mandatory flags or missing arguments for a flag are handled by the parser
+    # Optional flags must be handled explicitly unless given a default value.
+    parser = argparse.ArgumentParser(
+        prog="create-request-file",
+        description="Create a request file to pass to CDDS-convert",
    )
-    request = create_request()
-    write_request(request, target_path)
+    parser.add_argument("-p", help="Request-file path", required=True)
+    parser.add_argument("-m", help="Model_id", required=True)
+    parser.add_argument("-s", help="Suite_id", required=True)
+    parser.add_argument("-c", help="Calendar", required=True)
+    parser.add_argument("-v", help="Variant Label", required=True)
+    args = parser.parse_args()


A good convention to follow with argparse to keep main tidy is to put all the argparse commands in a single function called parse_arguments and call this from main with

args = parse_arguments()`

Please follow this approach here.

alistairsellar · 2025-03-28T15:25:19Z

CMEW/app/configure_standardise/bin/create_request_file.py

-    request = create_request()
-    write_request(request, target_path)
+    parser.add_argument("-p", help="Request-file path", required=True)
+    parser.add_argument("-m", help="Model_id", required=True)


Suggested change

parser.add_argument("-m", help="Model_id", required=True)

parser.add_argument("--model-id", "-m", help="Model_id", required=True)

Please include long flag names for all these arguments.

…s-env on same line.

#142: Two models in a run.

76eb059

mo-johnmo self-assigned this Mar 26, 2025

mo-johnmo added the enhancement New feature or request label Mar 26, 2025

mo-johnmo added this to the v0.1.0 milestone Mar 26, 2025

mo-johnmo added 2 commits March 26, 2025 15:30

#142: restructure_dirs.sh variables quoted. Delete opt/rose-suite-mod…

58e9b10

…el2.conf and remove its MODELS_OPTION from other files. Move configure_for rose-app.conf to a bash script.

#142: Edit app/configure_for/rose-app.conf .

ff58b2f

alistairsellar mentioned this pull request Mar 26, 2025

Feed CMEW's model_id through to ESMValTool recipe #260

Open

#142: Correct configure_for task, updating datasets with test and ref…

22d102c

…erence variant labels.

mo-johnmo marked this pull request as ready for review March 27, 2025 10:28

mo-johnmo requested a review from alistairsellar March 27, 2025 10:28

alistairsellar modified the milestones: v0.1.0, v0.2.0 Mar 27, 2025

#142: Remove command line parameters from

ab955a2

app/configure_for/update_recipe_file.py as they are no longer needed. Revert to using environment variables.

alistairsellar requested changes Mar 27, 2025

View reviewed changes

#142: Amend the recipe details for test and reference models.

b04203b

mo-johnmo requested a review from alistairsellar March 28, 2025 11:16

alistairsellar reviewed Mar 28, 2025

View reviewed changes

#142: Remove the update() method thus reverting the structural change…

dbb8cb5

… while retaining the variant_label and variant_label_reference additions.

alistairsellar requested changes Mar 28, 2025

View reviewed changes

mo-johnmo added 3 commits March 31, 2025 10:52

#142: Changes from review of configure_standardise task components.

899baf1

#142: configure_standardise.sh must have all arguments to cmew-proces…

0b608f1

…s-env on same line.

#142: Corrected a missing char from argument name.

1b9ef33

MGEX82 unassigned mo-johnmo Apr 1, 2025

alistairsellar self-assigned this Apr 7, 2025

		g_variant_label = "r1i1p1f1" # = ensemble test
		g_variant_label_reference = "r1i1p1f3" # = ensemble reference



		def main():
		def update(recipe_path, variant_label, variant_label_reference):

		json.dump(request, file, separators=(",\n", ": "))


		def make_request_file(



		def update_recipe(recipe_path):
		def update_recipe(recipe_path, variant_label, variant_label_reference):

	parser.add_argument("-m", help="Model_id", required=True)
	parser.add_argument("--model-id", "-m", help="Model_id", required=True)

Compare two model development runs in a single run of CMEW #259

Are you sure you want to change the base?

Compare two model development runs in a single run of CMEW #259

Uh oh!

Conversation

mo-johnmo commented Mar 25, 2025 • 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!

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

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

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

Uh oh!

mo-johnmo commented Mar 25, 2025 •

edited by alistairsellar

Loading