search

ESMValGroup / ESMValCore

ESMValCore: A community tool for pre-processing data from Earth system models in CMIP and running analysis scripts.
https://www.esmvaltool.org
Apache License 2.0
42 stars 38 forks source link

`iris` currently pinned to `iris>=3.1.0,!=3.2.0.post0,!=3.2.0` - rethink pin for future `iris` bugfix release #1509

Closed schlunma closed 2 years ago

schlunma commented 2 years ago

During the latest round of tests I found a pretty severe bug in iris: https://github.com/SciTools/iris/issues/4599

If a cube with a lazy aux coord is saved as netCDF file, this gives an error. The error does not appear in iris=3.1.0. I suggest pinning iris<3.2 for this release.

@ESMValGroup/esmvaltool-coreteam

valeriupredoi commented 2 years ago

that looks dodgy and a half, I can't believe it wasn't picked up by any of iris' tests - I commented on the issue, it'd be useful to look at dask and what version produces that behaviour

schlunma commented 2 years ago

Since no other package than iris changed when switching between iris=3.1.0 and iris=3.2.0 I guess it is related to iris. Maybe someone else can try to reproduce the issue? There is code for that in the iris issue. I did it on two independent machines.

I think we should pin iris quickly so that we are able to perform another round of tests for v2.5.

schlunma commented 2 years ago

One additional comment: for our tests with rc2 we still had iris=3.1.0, only since rc3 we use iris=3.2.0.

schlunma commented 2 years ago

I just had a discussion with @remi-kazeroni about this: our approach now is to wait for a response of the iris developers and postpone our release if necessary by 1-2 weeks so that we can include a fixed version of iris. It would be really a shame not to include the many new features that iris 3.2 offers.

In addition, we thought that it might be good to decouple our release from the iris releases. Having a new iris version in the middle of our testing phase really isn't optimal.

valeriupredoi commented 2 years ago

I just had a discussion with @remi-kazeroni about this: our approach now is to wait for a response of the iris developers and postpone our release if necessary by 1-2 weeks so that we can include a fixed version of iris. It would be really a shame not to include the many new features that iris 3.2 offers.

sounds reasonable to me, do we have any pressing IS-ENES3 matters?

In addition, we thought that it might be good to decouple our release from the iris releases. Having a new iris version in the middle of our testing phase really isn't optimal.

their release schedule is slightly more hectic than ours, and that'd mean we'll have to dance by their music, I am not sure this is something I'd like, we need to talk about it

schlunma commented 2 years ago

One more comment: I guess one of the reasons this doesn't break our entire preprocessing chain is that we use lots of logger.debug() statements which realize the coordinates...

schlunma commented 2 years ago

In addition, we thought that it might be good to decouple our release from the iris releases. Having a new iris version in the middle of our testing phase really isn't optimal.

their release schedule is slightly more hectic than ours, and that'd mean we'll have to dance by their music, I am not sure this is something I'd like, we need to talk about it

We didn't mean to speed up or slow down our release schedule, but rather be aware of iris' schedule.

valeriupredoi commented 2 years ago

We didn't mean to speed up or slow down our release schedule, but rather be aware of iris' schedule.

Yep, that's it! :+1:

valeriupredoi commented 2 years ago

note that it's half term here in the UK, and the MO as a gov institution is observing the week off (well, not all of them) so it may be that the issue gets delayed

bouweandela commented 2 years ago

The difficulty in trying to plan our releases around the iris releases is that from my experience they are quite unpredictable and can be delayed by months. I would propose to deal with collisions on a case by case basis rather than counting on them sticking to a particular schedule.

The alternative to pinning iris to <3.2 could be to pin to iris !=3.2, like that we will still get iris 3.2.1 one once it's released. But I agree that it is a very good idea to wait for the iris developers to respond to the issue before making any decisions about our own release.

schlunma commented 2 years ago

We had more discussion about this and thought that iris !=3.2 might be a good solution for that. I will open a PR with this soon. Do we need this for ESMValTool and ESMValCore?

zklaus commented 2 years ago

It seems the consensus here is to give the Iris folks a few more days to react, particularly in light of the holidays. Shall we wait till Tuesday or so before we make a decision?

schlunma commented 2 years ago

But what kind of action by the iris developers would make us not pin iris to !=3.2.0? We cannot use this version, regardless what they decide.

We will definitely delay the release, but pinning this rather sooner gives us more time for the tests.

valeriupredoi commented 2 years ago

they will tag a bugfix release of the rc they have, it is possible it will be 3.2.0.post1 so be careful with pinning strictly to != 3.2

schlunma commented 2 years ago

How about iris>=3.1.0,!=3.2.0.post0,!=3.2.0? This seems to work fine on my machine.

valeriupredoi commented 2 years ago

I suggest we pin extremely strictly eg !=3.2.0.post0=pyhd8ed1ab_0 since they may just up the build by 1 (dunno what they will do, to the very least pin !=3.2.0.post0 so we can get another 3.2 should it come by with the fix in it

zklaus commented 2 years ago

Pinning to !=3.2.0 sounds fine to me. I thought you only wanted to pin and then move forward with that. :+1: for pinning now for testing purposes and waiting (and keeping this issue open?) with a final decision.

valeriupredoi commented 2 years ago

for short term yes, for long term we should have a pin for 3.2.0.post0 just in case mamba gets drunk

schlunma commented 2 years ago

Cheers guys, will open the PR now! Note that we need both pins !=3.2.0.post0 and !=3.2.0 otherwise we pick up the version that is not excluded.

valeriupredoi commented 2 years ago

huh?

schlunma commented 2 years ago

huh?

Can you elaborate on that? :smile:

valeriupredoi commented 2 years ago

hahaha, sorry dude, had to dash out - yeah, why do we need to pin on the more general 3.2.0?

schlunma commented 2 years ago

If I don't do that the version 3.2.0 gets picked up. Didn't test if the bug is in there, but I would be really surprised if it wasn't.

valeriupredoi commented 2 years ago

let's keep this open since it's just a temporary pin, we want to revist this once all matters settle on the iris side

remi-kazeroni commented 2 years ago

The risk of pinning with iris>=3.1.0,!=3.2.0.post0,!=3.2.0 for the v2.5 release is that whenever a bugfix release of iris will be made, our users installing the tool (from conda) may get esmvaltool using a newer version of iris than what we used for the recipe testing. Either this will work smoothly or will require a bugfix release on our side to adjust the pinning. It is not quite clear to me if and when a bugfix release of iris will be made and how long we should put our release on hold.

My suggestion to move forward would be: pin iris<3.2 for our v2.5 release so that our users get a thoroughly tested version of the Tool. Once the release is done, we could remove the pin and those working with the development installation would get newer versions of iris and benefit from new features in iris3.2. If I understand correctly, ESMValTool v2.5 does not rely on recent additions in iris3.2 so I think it would make sense to only open iris3.2 features to developers instead of having it (untested) in the stable release.

sounds reasonable to me, do we have any pressing IS-ENES3 matters?

I don't think so. The project is currently under review and what matters is everything that was delivered until December 2021.

zklaus commented 2 years ago

Good points, @remi-kazeroni. In some sense, we want the core to be slightly ahead of the tool to facilitate the idea that new tool developments can benefit from new core features. The support for UGRID and irregular grids in Iris 3.2 would be a welcome addition.

Perhaps we could consider a 2.5.1 release? Or a work-around (since all we need to do is realize the data before saving)?

valeriupredoi commented 2 years ago

good points both from @remi-kazeroni and @zklaus - I too support a bugfix release v2.5.1 as soon as the iris bugfix comes online in the shape of a (post, or bug, or minor) release

The support for UGRID and irregular grids in Iris 3.2 would be a welcome addition.

Would that not be good enough for a v2.5.1 from us? I reckon we're not needing them asap

schlunma commented 2 years ago

I like this idea @remi-kazeroni !

As far as I can tell we are not using any of the new iris features in the core at the moment, so pinning it to <3.2 for the release and immediately remove this pin afterwards for new developments sounds fine to me.

The alternative approach would be to leave the pins as they are for the current release and hope that the bugfix release of iris doesn't break anything for us. If it does we would need a v2.5.1.

schlunma commented 2 years ago

We (the release managers) had another discussion about this. For us, pinning iris>=3.1.0,<3.2.0 feels like the optimal solution.

Our main arguments for this are:

I will open a PR with these changes now (see https://github.com/ESMValGroup/ESMValCore/pull/1525).

valeriupredoi commented 2 years ago

After running hundreds of recipes with iris=3.1.0, we are quite confident that our release is stable at the moment. We don't know if and how iris=3.2.x will change that.

Fully support this because :arrow_up: Cheers for all the work, guys!