Skip to content
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.

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

Use site-specific lists of recipes in the RTW #3856

Open
wants to merge 35 commits into
base: main
Choose a base branch
from

Conversation

chrisbillowsMO
Copy link
Contributor

@chrisbillowsMO chrisbillowsMO commented Jan 9, 2025

Description


Before you get started

Checklist

It is the responsibility of the author to make sure the pull request is ready to review. The icons indicate whether the item will be subject to the 🛠 Technical or 🧪 Scientific review.


To help with the number of pull requests:

@chrisbillowsMO chrisbillowsMO added the Recipe Test Workflow (RTW) Items relevant to the Recipe Test Workflow label Jan 9, 2025
@chrisbillowsMO chrisbillowsMO self-assigned this Jan 9, 2025
@chrisbillowsMO chrisbillowsMO added this to the v2.12.0 milestone Jan 9, 2025
@chrisbillowsMO chrisbillowsMO marked this pull request as ready for review January 9, 2025 11:21
@chrisbillowsMO chrisbillowsMO requested review from ehogan and removed request for ehogan January 9, 2025 11:21
Copy link
Contributor

@ehogan ehogan left a comment

Choose a reason for hiding this comment

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

Great work @chrisbillowsMO 🥳

I haven't yet reviewed the documentation, but I have run out of time today, so I'm sharing the review comments I have made so far. It's unlikely I'll have any time tomorrow to complete the review, so I'll do it first thing on Monday 👍

doc/sphinx/source/utils/RTW/tested_recipes.rst Outdated Show resolved Hide resolved
Comment on lines 19 to 21
- The files use the Jinja2 templating language (see this
:ref:`note <jinja2_templating_language>`, the `Cylc Jinja2`_ documentation
or the `Jinja2`_ documentation for more information).
Copy link
Contributor

Choose a reason for hiding this comment

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

Adding a link to more infomation (the first link) is great! 🥳 I wonder whether this second link should be included somewhere within the first link (if that's where more information about the files is located)? 🤔

Copy link
Contributor Author

@chrisbillowsMO chrisbillowsMO Jan 10, 2025

Choose a reason for hiding this comment

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

The second and third links were in the first link AS WELL...! Which is how I prefer it - but I merrily defer to the single source of truth doctrine. Despicable duplicate links removed here: 3445729 😉

Comment on lines 10 to 11
"is undefined and must be set, usually in '" ~ SITE_RECIPES_FILE ~ " - see 'How to "
"add a recipe to the RTW' documentation for more information."
Copy link
Contributor

Choose a reason for hiding this comment

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

Clarify the request. Also, I found out recently that, when writing documentation: use inclusive language that doesn't always refer to "seeing" an element. Unfortunately this is something I used to do a lot and I keep finding occurrences of it! 😱 I have made a suggestion below, but feel free to adjust 😊

Suggested change
"is undefined and must be set, usually in '" ~ SITE_RECIPES_FILE ~ " - see 'How to "
"add a recipe to the RTW' documentation for more information."
"is undefined and must be set in '" ~ SITE_RECIPES_FILE ~ ". The 'How to "
"add a recipe to the RTW' documentation provides more information."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I commited your suggestion but have reverted the . to a - (keeping your text) here: 2bb79fc

Jinja2 renders . as a line break.
E.g.

Jinja2Error: Jinja2 Assertion Error: FAST_RECIPES is undefined and must be set in 'site/metoffice-recipes.cylc'.
The 'How to add a recipe to the RTW' documentation provides more
information.

I did experiment with getting a . (e.g. using \., {% raw %} from here, using an utf-8 full stop) - but it made me feel like too much of a failure. I could live with the dash...!

(I didn't comment it in the file; it didn't feel a big enough issue. Let me know if we should have a comment)

Comment on lines 4 to 5
# Add recipes to the RTW in your site's `<site>-recipes.cylc` file in the 'site'
# directory. See "How to add a recipe to the RTW" documentation for more information.
Copy link
Contributor

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 this comment? It's almost identical to the error message below 😊

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just wanted them to REALLY know! 🤣

Removed here: 6054822

@@ -78,17 +81,27 @@

# Bash variable substitution below replaces `--` with `/` to obtain path to
# recipe.
[[process<fast>]]
{% for RECIPE in FAST_RECIPES %}
Copy link
Contributor

Choose a reason for hiding this comment

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

Jinja2 code should be indented from the left margin on its own terms, as recommended in Cylc Documentation: Style Guidelines

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 79ac7af (but yuck! 😆)

[
{
'recipe_path': 'recipe_radiation_budget',
'actual': 'na',
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be possible for you to get these values from the logs from the latest successful run (using your environment), please? 😊

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added here: 9c65801 I increased memory for two recipes based on the actuals - per the docs. They've obviously been running fine but I thought, if we're the "example" (at least for now) consistency with the documentation was most important.

And I alphabetised the recipes here: 6a61034

{% set FAST_RECIPES =
[
{
'recipe_path': 'recipe_radiation_budget',
Copy link
Contributor

Choose a reason for hiding this comment

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

Are single quotes required, or can double quotes be used? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replaced all singles with doubles: 7d7f19c

Comment on lines 66 to 76
# `recipe_ensclus` should be in fast: is in medium temporarily to avoid empty variable.
{% set MEDIUM_RECIPES =
[
{
'recipe_path': 'recipe_ensclus',
'actual': '2m10s, 2.4 GB on 2024-03-29',
'max_time': 'PT4M',
'max_memory': '3G',
},
]
%}
Copy link
Contributor

Choose a reason for hiding this comment

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

Now you have added checks (thank you!), if this was moved to FAST_RECIPES (where it belongs), would the workflow run without errors? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, I don't think so. If MEDIUM_RECIPES is empty (list or string) we still get:

ParamExpandError: parameter medium is not defined in compare<medium>

esmvaltool/utils/recipe_test_workflow/site/metoffice.cylc Outdated Show resolved Hide resolved
Comment on lines 6 to 7
{% set SITE_RECIPES_FILE = 'site/' ~ SITE ~ '-recipes.cylc' %}
{% from SITE_RECIPES_FILE import FAST_RECIPES, MEDIUM_RECIPES %}
Copy link
Contributor

Choose a reason for hiding this comment

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

I know you spent a lot of time investigating this, and apologies if you already told me the answer to this question, but if the file is included (e.g. {% include 'site/' ~ SITE ~ '-recipes.cylc' %}) are the FAST_RECIPES and MEDIUM_RECIPES variables not recognised? 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Recipe Test Workflow (RTW) Items relevant to the Recipe Test Workflow
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use site-specific lists of recipes in the RTW
2 participants