Skip to content

Lift and shift monsoon assessment area#361

Open
Paul Earnshaw (pdearnshaw) wants to merge 12 commits intomainfrom
343_autoassess_monsoon
Open

Lift and shift monsoon assessment area#361
Paul Earnshaw (pdearnshaw) wants to merge 12 commits intomainfrom
343_autoassess_monsoon

Conversation

@pdearnshaw
Copy link
Copy Markdown
Collaborator

@pdearnshaw Paul Earnshaw (pdearnshaw) commented Feb 9, 2026

This is a draft Pull Request to get a pre-review to make sure the direction of travel is as expected.

Closes #343 .

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)

@pdearnshaw Paul Earnshaw (pdearnshaw) changed the title 343 autoassess monsoon Lift and shift monsoon assessment area Feb 9, 2026
Comment thread CMEW/inc/autoassess.cylc

[scheduling]
[[graph]]
R1 = """
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Noddy question: does this overwrite, or append to, the R1 = ... in line 16 of the main flow.cylc file?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

It appends to. Use this quite a bit in our model workflows.

Copy link
Copy Markdown
Member

@ehogan Emma Hogan (ehogan) left a comment

Choose a reason for hiding this comment

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

This looks great Paul Earnshaw (@pdearnshaw), thank you! 🥳

I have some minor comments to add when you are ready for them, but my biggest question is whether we want to utilise Rose (i.e. use optional configuration based on the autoassess_area and use a single command in the rose-app.conf file) rather than defining the script within the autoassess.cylc file, for consistency with the rest of CMEW? 🤔 If this feels like a lot of work we can defer this, so we can showcase what works at the demo on Wednesday? 🤔

I have made some suggestions on what I needed to change to get CMEW to run. With these changes, CMEW fails for me as follows:

[FAIL] file:~/cylc-run/CMEW/run55/share/src/AutoAssess=source=git:https://github.com/MetOffice/AutoAssess.git::./::45_update_for_cmew: ls-remote: could not locate 'https://github.com/MetOffice/AutoAssess.git':
[FAIL]     fatal: could not read Username for 'https://github.com/': No such device or address
2026-02-16T09:10:12Z CRITICAL - failed/ERR

I assume this is because the AutoAssess repository is private and I don't have access. We might need to consider how best to solve this (edited to add: we could use a mirror?) / add something to the prerequisites.

Comment thread CMEW/inc/autoassess.cylc Outdated
Comment thread CMEW/inc/autoassess.cylc Outdated
Comment thread CMEW/inc/autoassess.cylc Outdated
Comment thread CMEW/inc/autoassess.cylc
@pdearnshaw
Copy link
Copy Markdown
Collaborator Author

Paul Earnshaw (pdearnshaw) commented Mar 3, 2026

In response to Emma's review I have the following comments, and have tried to reply to specific issues in the code review comments.

The issue over accessing AutoAssess from the GitHub repository has potentially been resolved. The URL used was the HTTPS version due to a misconfigured user authentication key (not authorised for SSO). Once this was resolved the SSH version of the repository URL worked. The Met Office's local mirror bot has also been added to the collaborator list so the AutoAssess repository is also available via local mirror. I have tested both of these and they "work for me"! These fixes are in commits fddeda2 (implement SSH URL) and 2032246 (use local mirror).

I have implemented the AutoAssess scripts as rose apps (all prefixed aa_) in commit 552c7be and bugfix 38d57b7. As suggested these can make use of optional configurations where appropriate, and this also tidies up the inc/autoassess.cylc file by removing the [[run_area<autoassess_area=some_area>]] sections. By using an "optional" optional configuration (use of brackets to define the optional configuration) we can supply an area assessment's alternative calling command if it exists and leave the default if it doesn't.

A couple more commits were required, fa9d32c fixed an issue raised by the code standards checker to include a blank line, and b28e82d makes sure the mass_retrieval cylc queue is acting on the correct family of tasks.

I think this PR is ready for another look if and when you are ready Emma?

@pdearnshaw
Copy link
Copy Markdown
Collaborator Author

Paul Earnshaw (pdearnshaw) commented Mar 3, 2026

One thing that has cropped up is that the AutoAssess code will not work with the esmvaltool community version of scitools due to the missing mo_pack library. Once this is included in version 2.13 then it should work fine. In the interim, the best choice is to switch to use scitools/default-current.

Copy link
Copy Markdown
Member

@ehogan Emma Hogan (ehogan) left a comment

Choose a reason for hiding this comment

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

Thanks Paul Earnshaw (@pdearnshaw)! 🥳

I have added some comments following a walkthrough with Naomi Parsons (@NParsonsMO).

Naomi Parsons (@NParsonsMO) ran CMEW, but it failed due to the lack of mo-pack. This is being dealt with via #399 👍

Comment thread CMEW/inc/autoassess.cylc
# autoassess.cylc

[task parameters]
autoassess_area = {{ AUTOASSESS_AREAS | join(",") }}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

One of the visions I had was when a scientist converted an AutoAssess metric to an ESMValTool recipe, that they would move the metric from the "autoassess_area" list to the "recipe" list. Therefore, would it be possible to add this to the flow.cylc file, and define (by hand) the AutoAssess areas (which should be monsoon for this PR) 👍

Comment thread CMEW/rose-suite.conf
Comment on lines +5 to +7
AUTOASSESS=false
!!AUTOASSESS_AREAS=["monsoon"]
!!AUTOASSESS_TITLE="AUTOASSESS TITLE"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I believe we agreed that AutoAssess would only be run at the MO. It would be good if AutoAssess ran by default at the MO. Would it be possible to add AUTOASSESS=true to opt/rose-suite-metoffice.conf, please?

Also, should the AUTOASSESS_TITLE be specific to the autoassess_area? Can the e.g. label_for_plots be used instead?

Comment on lines +7 to +8
[file:$CYLC_WORKFLOW_SHARE_DIR/src/AutoAssess]
source=git:git@github.com:MetOffice/AutoAssess.git::./::45_update_for_cmew
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If the statement I made earlier is true ("I believe we agreed that AutoAssess would only be run at the MO"), then the contents from app/install_autoassess/opt/rose-app-localmirror.conf can be used here 👍

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Also, the doc/source/user_guide/prerequisites_metoffice.rst file needs updating to include setting up the mirrors at the MO. I would prefer not to include the specific instructions here (since they contain paths), but there will (soon!) be a link to the Git and GitHub documentation that we can add (which is visible to all members of the Met Office organisation on GitHub).

Comment thread CMEW/rose-suite.conf

[template variables]
AUTOASSESS=false
!!AUTOASSESS_AREAS=["monsoon"]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The first time CMEW was run, it failed stating that a minimum of 2 years of data was required. It would be good to document this somewhere. (Is this true for all AutoAssess metrics?)

@@ -0,0 +1,8 @@
[command]
Copy link
Copy Markdown
Member

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 document all the new apps in doc/source/user_guide/workflow.rst, please? 😊

Comment thread CMEW/inc/autoassess.cylc
Comment on lines +67 to +70
REF_SUITE_ID = {{ REF_SUITE_ID }}
REF_SUITE_TITLE = {{ REF_LABEL_FOR_PLOTS }}
SUITE_ID = {{ SUITE_ID }}
SUITE_TITLE = {{ LABEL_FOR_PLOTS }}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not sure how this will work once we get rid of SUITE_ID and REF_SUITE_ID 😕

--mem=250MB

[[SITE_AA_HTML_PAGE]]
inherit = LOCAL
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This shouldn't be needed, since platform = localhost is already set in [[root]] 👍

[[LOCAL]]
platform = localhost

[[SPICE]]
Copy link
Copy Markdown
Member

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 instead use [[COMPUTE]]?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

And have the inheritance be added more globally via inc/autoassess.cylc?

inherit = SPICE
execution time limit = PT5M
[[[directives]]]
--job-name=AutoAssess_install
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What does job-name do? Is this needed? Can the default job name be used?

execution time limit = PT5H
[[[directives]]]
--job-name=AutoAssess_MOOSE_retrieval
--mem=2GB
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This file should be used to specify time and memory usage for a given AutoAssess area, rather than family (assuming the requirements will differ depending on the area?).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Lift and shift monsoon assessment area

3 participants