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

Align ESMValTool to ESMValCore=2.2.0 (adopt iris3, fix environment for new Core release) #1874

Merged
merged 16 commits into from Feb 10, 2021

Conversation

stefsmeets
Copy link
Contributor

@stefsmeets stefsmeets commented Oct 20, 2020

This PR tracks the progress to use iris3 in esmvalcore/esmvaltool. See this issue: #1862
Using this branch of esmvalcore: https://github.com/ESMValGroup/ESMValCore/tree/iris3 (PR: ESMValGroup/ESMValCore#819)

To do list

We should keep this PR as is until ESMValGroup/ESMValCore#819 is merged and released(?). Before merging into master, we need to look at:

  • Merge master, and address conflicts with code using the updated syntax.
  • setup.py: Uncomment 'scitools-iris=3.0.0'

Tasks

  • Create an issue to discuss what you are going to do, if you haven't done so already (and add the link at the bottom)
  • Give this pull request a descriptive title that can be used as a one line summary in a changelog
  • Circle/CI tests pass. Status can be seen below your pull request. If the tests are failing, click the link to find out why.
  • Preferably Codacy code quality checks pass, however a few remaining hard to solve Codacy issues are still acceptable. Status can be seen below your pull request. If there is an error, click the link to find out why. If you suspect Codacy may be wrong, please ask by commenting.
  • Please use yamllint to check that your YAML files do not contain mistakes

Closes #1862 and #2006

@bouweandela
Copy link
Member

bouweandela commented Oct 30, 2020

I tested:

  • examples/recipe_python.yml
  • examples/recipe_preprocessor_test.yml

with this branch and compared the runtime and results with the master branch.

The runtime is similar and the resulting netcdf files are too, though there was some minor difference in the time axis of the multimodel statistics of diagnostic_1 of the preprocessor test recipe (different calendar chosen from input cubes), but I suspect that is related to the multi model statistics preprocessor and not an iris issue.

@valeriupredoi
Copy link
Contributor

something to be aware of #2006

@valeriupredoi
Copy link
Contributor

failing tests are dubious, here SciTools/iris#3967

@valeriupredoi
Copy link
Contributor

failing tests are dubious, here SciTools/iris#3967

@schlunma you mind fixing those tests in line with the change of iris.coords.Coord as @bjlittle tells us about in SciTools/iris#3967 🍺

@schlunma
Copy link
Contributor

Will do!

@schlunma
Copy link
Contributor

Haha, so apparently it was me who coded that 😂 No idea why I did that though...

@valeriupredoi
Copy link
Contributor

Haha, so apparently it was me who coded that joy No idea why I did that though...

shh, nobody know 🤣 We need to grep on the diags dir see if others shared your penchant for Coord() 😁

@schlunma
Copy link
Contributor

Just checked it, it's only me 😂

But thank god we had unit tests for this...I think we need to thoroughly test all diagnostics before releasing this, I'm pretty sure there are other issues waiting for us not tested by unit tests.

@schlunma
Copy link
Contributor

Just fixed the tests 👍

I just noticed that I get the following warning every time I import iris now:

Python 3.7.9 | packaged by conda-forge | (default, Dec  9 2020, 21:08:20) 
[GCC 9.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import iris
/mnt/lustre02/work/bd0854/b309141/miniconda3/envs/esm2/lib/python3.7/site-packages/iris/config.py:139: UserWarning: Ignoring config item 'Resources':'test_data_dir' (section:option) as '/home/h05/itwl/projects/git/iris-test-data/test_data' is not a valid directory path.
  warnings.warn(msg.format(section, option, c_path))

Anyone else experiencing this? If yes I will open an issue in the iris repo.

@jvegreg jvegreg added the requires new ESMValCore release A new release of ESMValCore is needed to solve this issue/merge this pull request.. label Feb 2, 2021
@schlunma
Copy link
Contributor

schlunma commented Feb 4, 2021

Would it be possible to get this merged as soon as possible? For a new PR I need the new release branch of ESMValCore (or its master), and the current master of ESMValTool does not work with that because of iris3!

@valeriupredoi valeriupredoi marked this pull request as ready for review February 4, 2021 19:24
@valeriupredoi
Copy link
Contributor

Would it be possible to get this merged as soon as possible? For a new PR I need the new release branch of ESMValCore (or its master), and the current master of ESMValTool does not work with that because of iris3!

yes, let me pin mpich from #2007 then test and will merge - will do this tomorrow morning 👍

@bouweandela
Copy link
Member

The changes in this pull request need ESMValCore v2.2, so this can only be merged once that has been released (next Monday).

@schlunma You can just branch off of this branch for your pull request that needs the development version of the ESMValCore.

@schlunma
Copy link
Contributor

schlunma commented Feb 5, 2021

Cool, thanks!! I already branched of this branch, just wanted to make sure that this PR has priority

@valeriupredoi
Copy link
Contributor

I just re-double-checked the situation with mpich that needs to be pinned to avoid library import issues, even though the Core from release _2.2.0 installs a newer version of mpich:

(release22x) valeriu@valeriu-PORTEGE-Z30-C:~/ESMValCore$ conda list mpich
# packages in environment at /home/valeriu/miniconda3/envs/release22x:
#
# Name                    Version                   Build  Channel
mpich                     3.4.1                h846660c_2    conda-forge

the updated env for Tool with this branch updates the mpich to the offending package:`

(release22x) valeriu@valeriu-PORTEGE-Z30-C:~/ESMValTool$ conda list mpich
# packages in environment at /home/valeriu/miniconda3/envs/release22x:
#
# Name                    Version                   Build  Channel
mpich                     3.4.1                external_2    conda-forge

that's causing us issues. So the pin needs to stay in for now. I will add a note to the env and meta file so we remove it as soon as we can. Gotta love conda

@valeriupredoi
Copy link
Contributor

OK I've also pinned to the new esmvalcore that will be released today by the magnificent @jvegasbsc 💃

@valeriupredoi valeriupredoi changed the title Switch to iris3 Align ESMValTool to ESMValCore=2.2.0 (adopt iris3, fix environment for new Core release) Feb 8, 2021
Copy link
Member

@bouweandela bouweandela left a comment

Choose a reason for hiding this comment

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

It's important that you keep the pin on the upper version of esmvalcore, or the esmvaltool package may end up being broken in the two weeks between the esmvalcore release and esmvaltool release.

package/meta.yaml Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
environment.yml Outdated Show resolved Hide resolved
valeriupredoi and others added 3 commits February 9, 2021 10:42
Co-authored-by: Bouwe Andela <b.andela@esciencecenter.nl>
Co-authored-by: Bouwe Andela <b.andela@esciencecenter.nl>
Co-authored-by: Bouwe Andela <b.andela@esciencecenter.nl>
@valeriupredoi
Copy link
Contributor

It's important that you keep the pin on the upper version of esmvalcore, or the esmvaltool package may end up being broken in the two weeks between the esmvalcore release and esmvaltool release.

very good point @bouweandela cheers 🍺

@bouweandela
Copy link
Member

Merged #1994 into this branch to see if it makes the tests pass.

@bouweandela
Copy link
Member

The conda build is failing, but it seems unrelated to this pull request, see #2016.

Because we really need this to move forward with testing of ESMValTool for the upcoming release and integrating final features, I will merge this.

@bouweandela bouweandela merged commit 121effe into master Feb 10, 2021
@bouweandela bouweandela deleted the iris3 branch February 10, 2021 12:17
@valeriupredoi
Copy link
Contributor

yes yes yes 🍺 And the install works a-ok with esmvalcore=2.2.0 👍

@bouweandela bouweandela mentioned this pull request Feb 10, 2021
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement in technical review iris release requires new ESMValCore release A new release of ESMValCore is needed to solve this issue/merge this pull request..
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Switch to iris3 testing
5 participants