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

Initial Commit for Recipe Test Workflow #2787

Merged
merged 21 commits into from
Sep 2, 2022

Conversation

mo-tgeddes
Copy link
Contributor

@mo-tgeddes mo-tgeddes commented Sep 1, 2022

Description

Add files from CMEW as a base for the recipe test workflow.
This PR will be merged to a feature branch and does not need a review from the ESMValTool community at this time


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.

New or updated recipe/diagnostic

New or updated data reformatting script


To help with the number of pull requests:

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.

Thanks @mo-tgeddes! :) I have a few requests:

  • There are still a number of instances referring to CAW and climate-assessment-workflow in this PR (try searching in this page or using, e.g. the command grep -ir caw esmvaltool/utils/recipe_test_workflow/ on the command line). Please ensure all references to CAW and climate-assessment-workflow are replaced.
  • Would it be possible to name this PR something like "Initial commit for the recipe test workflow" (there should be no reference to CAW in this PR, even in the title!) :)
  • Please remove the Met Office copyrights for introduction to the ESMValTool repository.

We may need to merge the documentation into the central ESMValTool documentation, but I think it makes sense to keep it separate for now, while we are developing.

esmvaltool/utils/recipe_test_workflow/doc/source/about.rst Outdated Show resolved Hide resolved
esmvaltool/utils/recipe_test_workflow/doc/source/conf.py Outdated Show resolved Hide resolved
setup.cfg Outdated Show resolved Hide resolved
setup.cfg Outdated Show resolved Hide resolved
@mo-tgeddes mo-tgeddes changed the title Add known CAW files to Recipe Test Workflow Initial Commit for Recipe Test Workflow Sep 2, 2022
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.

Thanks @mo-tgeddes! :)

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.

Thanks @mo-tgeddes! :)

The suggestion are to ensure the documentation builds and the workflow runs.

I note that the recipe fails to run after updating the paths, so we'll need to fix that :)

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.

Thanks @mo-tgeddes! 🥳

@mo-tgeddes mo-tgeddes marked this pull request as ready for review September 2, 2022 11:57
@mo-tgeddes mo-tgeddes merged commit 57bc0d2 into recipe_test_workflow_prototype Sep 2, 2022
@mo-tgeddes mo-tgeddes deleted the add_known_caw_files branch September 2, 2022 11:58
@valeriupredoi
Copy link
Contributor

hey guys, good to see work being done on this matter! Could I please suggest a couple of things:

  • it's good to have the PR description checklist being run through and boxes being ticked or removed if necessary - even if it's a PR to be merged into a feature branch, which brings me to the second point -
  • could you please not work on branches that will eventually merge into other branches, that in turn, are targeted to main? It's hard to keep track of changes that were reviewed by others while review is still needed for the feature branch that gets merged into main ie what if I am a reviewer of the feature branch and I have questions about changes that were brought in by the initial merge(s)? That'll re-trigger the review process of changes that were already approved and merged by others, without any notification that I could see.

Instead, if you think you need to change a lot, and the PR is large, then it's best to split it in successive smaller PRs that are directly targeted to main; also, you could use forks, that are meant exactly for this type of work - collective work on a larger issue that is somewhat separate from the regular/daily development work. Cheers 🍺

@ehogan
Copy link
Contributor

ehogan commented Sep 20, 2022

Hi V! 👋

Apologies for the delay in my reply, I got back from leave last week and I'm still playing catch up!

During the requirements gathering, we (the Met Office) offered to complete the development work, but we didn't discuss what that would look like!

We (@KatherineTomkins, @mo-tgeddes and myself) agreed to use a main feature branch (recipe_test_workflow_prototype) with sub-feature branches for each sub-task that contributed to the recipe test workflow prototype. This enables me (as someone with Rose and Cylc knowledge) to easily perform the reviews (in small chunks), while ensuring the development of the sub-tasks are transparent to the community (via the sub-feature branches). In addition, we would then be able to "deliver" the prototype to the community in one go (via a PR with the main feature branch targeted to main). Working with main feature branches and sub-feature branches is something I do regularly in other repositories; is there a technical / ESMValTool reason not to do this? I do feel that delivering the prototype in this way is a good way to do things, but as you point out, this could also be achieved via a fork.

One other thing to consider before making a decision about the development workflow is whether the community would like to get involved in reviewing the sub-feature branches as a knowledge sharing / learning activity. If this is the case, I would suggest the main feature branch approach that we are already taking would be preferred, since the community can get involved as and when they would like. However, if the community would prefer to receive the prototype in one go, then we can work on a fork (although this will be less transparent to the community).

Apologies for not managing the checklist appropriately in this PR. If you are happy for us to continue with the main feature branch approach, would it be possible for you to add protection to the recipe_test_workflow_prototype branch so it can be treated similarly to main? We will also manage the checklist appropriately in the future.

Thoughts? :)

@valeriupredoi
Copy link
Contributor

@ehogan welcome back, hope you had a good leave 🏖️ Sounds good to me, if the baby PRs/branches are (mostly) for internal use/review then it's perfectly fine for me, but it would be good if you guys included those in the big PR description (please add links to those, and a few descriptive words about them e.g. we implemented X in PR Y [with a link to it] - much clearer in the description then somewhere in the lower bellows of the PR). How's that sound?

@ehogan
Copy link
Contributor

ehogan commented Sep 22, 2022

Great idea @valeriupredoi! We will definitely do that when we do the final PR. I'll also plan to keep the summary issue updated with this information as well :) Thank you! 🥳

KatherineTomkins pushed a commit that referenced this pull request Oct 3, 2022
* #2787 Install ESMValTool and Core in install_cold task

* #2821: PR comments part 1

* #2821: PR comments part 2

* Update esmvaltool/utils/recipe_test_workflow/recipe_test_workflow/app/install_cold/bin/install_cold.sh

Co-authored-by: Emma Hogan <[email protected]>

* Update esmvaltool/utils/recipe_test_workflow/recipe_test_workflow/app/install_cold/bin/install_cold.sh

Co-authored-by: Emma Hogan <[email protected]>

* Update esmvaltool/utils/recipe_test_workflow/recipe_test_workflow/app/install_cold/bin/install_cold.sh

Co-authored-by: Emma Hogan <[email protected]>

* #2821: PR comments part 3

* #2821: PR comments part 4

* #2821: PR comments part 5

Co-authored-by: Emma Hogan <[email protected]>
@mo-tgeddes mo-tgeddes added the Recipe Test Workflow (RTW) Items relevant to the Recipe Test Workflow label Oct 13, 2022
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.

3 participants