-
Notifications
You must be signed in to change notification settings - Fork 2
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
Fixes #53: Avoid Iris 3.2.0.post0 #54
Conversation
Honestly, I'm very confused about the Travis builds. I have checked and Travis-CI finds master, the black+isort branch, and the 3d averages branch – but not this one? (see screenshot) I have tried whether the pattern of the branch name is a problem but it didn't lead to any problems on my fork. I'll have to check out later if this problem persists with every new branch we're creating here of if it's just this one that's causing problems. |
.travis.yml
Outdated
@@ -30,7 +30,7 @@ install: | |||
- conda activate test-environment | |||
- conda env update -n test-environment --quiet --file conda_environment.yml | |||
- conda install pip | |||
- conda install -c conda-forge iris | |||
- conda install -c conda-forge "iris>=3.1,!=3.2.0.post0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can actually just delete this line. line 31 already installs iris from the conda_environment file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can actually just delete this line. line 31 already installs iris from the conda_environment file.
Okay, I was wondering about this.
BUT: I think the conda env update
command comes with it's own issues. At least at some point, it upgraded my Python version (because it says python>=3.6
), which is not what we want, do we? So I wonder in general how reliable this recipe really is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For reference, this is what I found is a reliable way to install the test environment:
conda create -n testenv -c conda-forge python=3.x
conda activate testenv
conda install -c conda-forge "iris>=3.1,!=3.2.0.post0"
pip install -e . # source install from the cloned git repo
pip install [...] # whatever else is needed to run the tests (coverage, pytest, ...)
This can of course be adapted again to some of the helpful extras of the .travis
recipe and also using some environment file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, we can also remove the python>=3.6
since a too low Python version will lead to a failed installation anyhow. The problem with having it in .travis.yml
is simply that we have to update this in three places every time we change something about the dependencies (setup.py
, conda_environment.yml
, .travis.yml
)
conda_environment.yml
Outdated
- iris>=3.1,!=3.2.0.post0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really understand the decision for this yet: In #53 you mention that the actual errors leave when you install Iris version 3.1. Why not change this to iris==3.1
or iris>=3.1,<=3.2
, then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Especially if Iris drops support for Python 3.7 from now on, I think we should start thinking if we just keep it at the current version or require 3.8 or higher.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really understand the decision for this yet: In #53 you mention that the actual errors leave when you install Iris version 3.1. Why not change this to
iris==3.1
oriris>=3.1,<=3.2
, then?
We know that Iris 3.2.0.post0 is buggy, so a specific specification would be to exclude that. We do not want to exclude versions that are not known to be buggy. Specifically, we want to allow for newer Iris versions (which presumably have the bug fixed).
Also borrowing from the discussion in ESMValGroup/ESMValCore#1509.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But still, the tests are failing with Iris 3.2 (without the .post0
prefix). So maybe use !=3.2
instead of !=3.2.0.post0
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, I see that there are actually two buggy package candidates on conda-forge, I haven't seen this earlier (and it was working for me).
So !=3.2.0.post0
is not enough, but what about !=3.2.0
, which is relaxing the excluding rule as little as possible? It is also what the ESMValTool people seem to agree on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... should have read the whole discussion over there. Both !=3.2.0.post0
and !=3.2.0
need to be pinned in that case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that sounds a lot better! Also sounds pretty much like what has been discussed in the other thread :)
cube = iris.load_cube(src, var).collapsed("latitude", iris.analysis.MEAN) | ||
# print(cube) # Workaround for the Iris bug | ||
iris.save(cube, str(dst)) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice to add a test for this directly! 👍
Okay, at least it is not me being stupid! |
Just to make sure I understand: Is your conclusion also that Travis is not triggered to run the tests for this branch? |
Exactly! Seems not to get triggered, I have no idea why |
@uwefladrich now Travis found the branch 😄 |
No description provided.