ESMValGroup / ESMValTool

ESMValTool: A community diagnostic and performance metrics tool for routine evaluation of Earth system models in CMIP
https://www.esmvaltool.org
Apache License 2.0
210 stars 122 forks source link

Update cylc version in rtw.lock file for recipe_test_workflow #3596

Closed mo-gill closed 1 month ago

mo-gill commented 1 month ago

Description

This PR is intended to update the cylc version of the recipe test workflow prototype to match the more recent version8.2.5

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:

valeriupredoi commented 1 month ago

hey guys, the failed GA tests (looks to be a transitory HTTPS error, most defo will go away once you rerun them) prompted me to have a look at your GA workflow, and if you don't mind me, I could suggest a few improvements: have a look at the GA workflow we use to create the env from a lock file: https://github.com/ESMValGroup/ESMValCore/blob/main/.github/workflows/install-from-condalock-file.yml - it' s better if you use a dedicated action that sets up miniconda, ideally with Miniforge (or just conda-forge, if you are restricted to a certain set of packages), rather than using a system conda in the OS container. Also, in the long run, it'd be good if you talked to the MetO sysadmins to set up an eg met-office conda channel that is aligned to conda-forge, so that installation is a lot more flexible :+1: If you want me to, I can help with improving the GA workflow you got :beer:

ehogan commented 1 month ago

hey guys, the failed GA tests (looks to be a transitory HTTPS error, most defo will go away once you rerun them) prompted me to have a look at your GA workflow, and if you don't mind me, I could suggest a few improvements: have a look at the GA workflow we use to create the env from a lock file: https://github.com/ESMValGroup/ESMValCore/blob/main/.github/workflows/install-from-condalock-file.yml - it' s better if you use a dedicated action that sets up miniconda, ideally with Miniforge (or just conda-forge, if you are restricted to a certain set of packages), rather than using a system conda in the OS container. Also, in the long run, it'd be good if you talked to the MetO sysadmins to set up an eg met-office conda channel that is aligned to conda-forge, so that installation is a lot more flexible ๐Ÿ‘ If you want me to, I can help with improving the GA workflow you got ๐Ÿบ

Thanks @valeriupredoi! I was hoping to have a conversation with you and others in the TLT about how best to integrate the GitHub workflow and the documentation we have in our RTW branch into ESMValTool proper, in preparation for getting our main feature branch for the RTW merged into main. I was thinking that the workshop might be a good place to do this? ๐Ÿค”

ehogan commented 1 month ago

hey guys, the failed GA tests (looks to be a transitory HTTPS error, most defo will go away once you rerun them) prompted me to have a look at your GA workflow, and if you don't mind me, I could suggest a few improvements: have a look at the GA workflow we use to create the env from a lock file: https://github.com/ESMValGroup/ESMValCore/blob/main/.github/workflows/install-from-condalock-file.yml - it' s better if you use a dedicated action that sets up miniconda, ideally with Miniforge (or just conda-forge, if you are restricted to a certain set of packages), rather than using a system conda in the OS container. Also, in the long run, it'd be good if you talked to the MetO sysadmins to set up an eg met-office conda channel that is aligned to conda-forge, so that installation is a lot more flexible ๐Ÿ‘ If you want me to, I can help with improving the GA workflow you got ๐Ÿบ

Thanks @valeriupredoi! I was hoping to have a conversation with you and others in the TLT about how best to integrate the GitHub workflow and the documentation we have in our RTW branch into ESMValTool proper, in preparation for getting our main feature branch for the RTW merged into main. I was thinking that the workshop might be a good place to do this? ๐Ÿค”

Also, just to add, there was a required step that was missed before the lock file was committed, so we will resolve this shortly ๐Ÿ‘

valeriupredoi commented 1 month ago

sounds good and sounds good, Emma :beer:

mo-gill commented 1 month ago

Thanks @mo-gill! ๐Ÿ˜Š

A few requests; please would you:

* fix the end of file issue in `rtw-env.yml`

* update the GitHub workflow for the RTW to use the new `rtw-env.lock` file rather than the old `rtw.lock` file

* remove the old `rtw.lock` file

* update the link mentioned on L40 at https://github.com/ESMValGroup/ESMValTool/actions/runs/9079097479/job/24947565289?pr=3596#step:9:41 to use the new link, so that it no longer re-directs (update the link in [esmvaltool/utils/recipe_test_workflow/doc/source/common.txt](https://github.com/ESMValGroup/ESMValTool/blob/3593_update_cylc_version_in_lock_file/esmvaltool/utils/recipe_test_workflow/doc/source/common.txt#L19))

I also just noticed that the "Run Cylc configuration linter" task is returning issues but not failing, see https://github.com/ESMValGroup/ESMValTool/actions/runs/9079097479/job/24947565289?pr=3596#step:5:5. Would it be possible for you to make changes so that the task fails if cylc lint returns issues, please? (It may be that updating to the new version of Cylc will solve the problem, so do that first!) ๐Ÿคช

The rtw-env.yml whitespace should be fixed now though i can't work out which commit it was in :fearful:

Correction of cylc version is in commit https://github.com/ESMValGroup/ESMValTool/pull/3596/commits/cab9cdbf05c6743a3e780cf711de96258c8567ba

Removal of the rtw.lock file is in commit https://github.com/ESMValGroup/ESMValTool/pull/3596/commits/41b0abb8b4096738d1b3a4b803fd933b1c0e5da1

ESMValTool URL was updated in commit https://github.com/ESMValGroup/ESMValTool/pull/3596/commits/41b0abb8b4096738d1b3a4b803fd933b1c0e5da1

The linting issues should all now be sorted in commit 197aa519460d7aae3f86449f421658beb3fae3e1