Review training material episode 7

[annefou]

Going through episode 7, I have a few questions/remarks:

I would clone the repository to ~/ so everywhere we could cd ~/ESMValTool rather then cd ESMValTool. Or at least I would suggest to be consistent in all the episodes.

Review process

• Then, we create a new branch locally and start developing new codes. Here you assume that everyone has access to the ESMValTool repo. Shouldn't we ask to either request write access or fork the repo? Of course it does not matter for the tutorial itself as we never push back to github but ideally mentioning the "real" process may be a good approach. What do you think?

Using pre-commit

• I would rather use git checkout -b your_branch_name to create a new branch as it has not been done yet.
• Is there any conventions for naming branches? What I mean is that patch-1 may not be something you are willing to have as a branch name. Maybe a remark on how to name your branch may be useful.

2. flake8: the error message is about an unused local variable nx. We should check our codes regarding the usage of nx. For now, let’s assume that it is added by mistake and remove it.

Maybe we could add a sentence: Then rerun pre-commit and check that it passes. Mostly to have the same level of details everywhere.

Running tests

• Let’s checkout our local branch and add the recipe: rather than checkout, We just checkout earlier so it is not really necessary. Maybe we could rather ask to check the local branch to make sure we are in our "new" branch.
• Running pytest takes a very long time so maybe some information on the duration of these tests could help.
• Shouldn't we git add esmvaltool/recipes/warming_stripes.yml to be consistent with what we have done with the corresponding python script.
• I never managed to run pytest successfully even after fixing warming_stripes.yml:

  scripts:
   warming_stripes_script:
     script: warming_stripes.py

So maybe we need a bit more guidance in the solution (fixing warming_stripes.yml is probably not sufficient and it is obvious to everyone but me!).

[annefou]

The error I got is:

cat log.txt

ERROR: LoadError: ArgumentError: Package YAML [ddb6d928-2868-570f-bddf-ab3f9cf99eb6] is required but does not seem to be installed:
 - Run `Pkg.instantiate()` to install all recorded dependencies.

Stacktrace:
 [1] _require(::Base.PkgId) at ./loading.jl:999
 [2] require(::Base.PkgId) at ./loading.jl:928
 [3] require(::Module, ::Symbol) at ./loading.jl:923
 [4] include(::Function, ::Module, ::String) at ./Base.jl:380
 [5] include(::Module, ::String) at ./Base.jl:368
 [6] exec_options(::Base.JLOptions) at ./client.jl:296
 [7] _start() at ./client.jl:506
in expression starting at /data/dnb03/galaxy_db/job_working_directory/017/920/17920290/tmp/pytest-of-root/pytest-0/popen-gw35/test_diagnostic_run_diagnostic0/diagnostic.jl:2

[bouweandela]

That error happens because pytest is trying to run the test that checks if you are able to run a Julia diagnostic, but you may not have installed the Julia dependencies. To run only tests that do not require additional Julia or R dependencies, run

pytest -m 'not installation'

or alternatively, you can install the Julia dependencies with

esmvaltool install Julia

and the R dependencies with

esmvaltool install R

[annefou]

Cool! Thank you! I will try it out!

[SarahAlidoost]

@annefou Many thanks for reviewing the lesson and taking the time to submit the issue, very good points and detailed comments. :+1:

[SarahAlidoost]

To address this issue, a pull request should be made changing (only) episode 7 file and fixing these items:

• [ ] use cd ~/ESMValTool instead of cd ESMValTool in this episode
• [x] in section Review process, it is mentioned Then, we create a new ``branch`` locally and start developing new codes. After this line, add something like: To create a new branch: git checkout -b your_branch_name.
• [x] in solution part of section Using pre-commi, it is mentioned: 2. flake8: the error message is about an unused local variable nx. We should check our codes regarding the usage of nx. For now, let’s assume that it is added by mistake and remove it. After this line, add this: Then rerun pre-commit and check that it passes.
• [x] in section Running tests, it is mentioned: Let’s checkout our local branch and add the recipe.... Change this line to: make sure our local branch is checked out and add the recipe.
• [x] in section Running tests, it is mentioned: Run pytest and inspect the results. If a test is failed, try to fix it. After this line, add something like: this might takes a few minutes.
• [x] in section Review process, where it is mentioned: "Then, we create a new branch locally and start developing new codes...". Before this line, add something like: "We encourage you to join the ESMValTool development team. This way you can submit a pull request." It is already mentioned in the episode that for more information see ESMValTool documentation on GitHub workflow.

and we need to check:

• [x] if we follow the development installation explained in this episode, all dependencies should be installed. Find out whether it is correct to use the command pytest -m 'not installation'.

and finally providing best practices around branch names is beyond this tutorial.

[bouweandela]

pytest -m 'not installation' is what you would use to run the tests if NCL, R and Julia dependencies have not been installed (this happens if you only install the esmvaltool-python package from conda).

[SarahAlidoost]

pytest -m 'not installation' is what you would use to run the tests if NCL, R and Julia dependencies have not been installed (this happens if you only install the esmvaltool-python package from conda).

Many thanks for the explanations. In episode 7 of the tutorial, we run pip install --editable '.[develop]'. So, I think that all the dependencies should be installed.

[bouweandela]

Looking at the text of that episode, it looks like you're doing a Installation from source there, but skipping the steps for installing the R and Julia dependencies. That means that you need to use the command pytest -m 'not installation' to run the tests (or do a complete installation by installing Julia and then running esmvaltool install Julia and esmvaltool install R).

[SarahAlidoost]

Looking at the text of that episode, it looks like you're doing a Installation from source there, but skipping the steps for installing the R and Julia dependencies. That means that you need to use the command pytest -m 'not installation' to run the tests (or do a complete installation by installing Julia and then running esmvaltool install Julia and esmvaltool install R).

oh, good to know, many thanks :+1: