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

Validate RTW user config file #3615

Merged

Conversation

chrisbillowsMO
Copy link

@chrisbillowsMO chrisbillowsMO commented May 23, 2024

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 enhancement Recipe Test Workflow (RTW) Items relevant to the Recipe Test Workflow labels May 23, 2024
@chrisbillowsMO chrisbillowsMO self-assigned this May 23, 2024
@chrisbillowsMO chrisbillowsMO marked this pull request as ready for review May 24, 2024 08:42
Copy link
Contributor

@alistairsellar alistairsellar left a comment

Choose a reason for hiding this comment

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

Wow - great stuff @chrisbillowsMO! Clearly written and a nice choice of tests.

I've tested on Jasmin and it runs cleanly.

As an aside, I've warmed to @ehogan's suggestion of an esmvaltool config validate command: #3392 (comment). But makes sense I think to merge this to RTW now, and then discuss moving the functions to Core for a new command.

@chrisbillowsMO
Copy link
Author

chrisbillowsMO commented May 24, 2024

Wow - great stuff @chrisbillowsMO! Clearly written and a nice choice of tests.

I've tested on Jasmin and it runs cleanly.

Hi @alistairsellar - great! 😁 I'm so pleased you're pleased. @ehogan didn't want to miss out on all the fun so I'll await her final feedback and merge once she bequeaths her seal.

As an aside, I've warmed to @ehogan's suggestion of an esmvaltool config validate command: #3392 (comment). But makes sense I think to merge this to RTW now, and then discuss moving the functions to Core for a new command.

Great! I was working with this in mind with a view to having a crack at the issue raised against it in ESMValCore:

One point to consider: it may be a bigger job to implement this in core.

CFG/Configure() objects are specifically designed to throw an error on the first invalid value. I had to write a workaround to collect multiple errors (i.e. to save the user multiple runs/failures).

I wrote it up a little here: #3392 (comment) but the tldr; for core we'd probably want a different, more holistic solution than what I've written. Indeed, it might mean re-shaping a lot of the validation code in ESMValCore - probably not ideal! A decorator might offer a solution but I need to fully understand the validation code first. I'm hoping to continue exploring when/if time allows.

Also, you're probably already aware, but possibly to bear in mind are the broader changes afoot in the user config file arena:

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.

Lovely, thanks @chrisbillowsMO! 🥳

@chrisbillowsMO chrisbillowsMO linked an issue May 24, 2024 that may be closed by this pull request
@chrisbillowsMO chrisbillowsMO merged commit 5d262ef into recipe_test_workflow_prototype May 24, 2024
10 checks passed
@chrisbillowsMO chrisbillowsMO deleted the 3392_validate_rtw_usr_config branch May 24, 2024 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Recipe Test Workflow (RTW) Items relevant to the Recipe Test Workflow
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Validate RTW user config file
3 participants