Fix build #270
Fix build #270
Conversation
The Iris dev guide has moved - update the link to the new location
This reverts commit 3fddc50.
|
Removed the changes from the other PR I was working on that got caught in this. |
|
Last conflict seems to be a conflict at the dependency resolution stage. This probably explains why I'm struggling to get tests to run locally too (hence pushing them here to be tested.) It seems to be unable to find mo_pack and I can't find it either - the install instruction of |
|
On the plus side, the build using Iris from conda-forge rather than source is working. |
|
Okay, mo_pack I can't find because it doesn't support Windows (probably should have noticed that earlier.) Not sure why CI wasn't picking up the specified Black version while Iris's CI could. Re-running in the hope it's sorted itself out. |
There was a problem hiding this comment.
Essentially this is great.
Many many thanks for seeing this + taking the trouble to fix @TomDufall 🚀
I probably should have seen this coming with SciTools/iris#4141. But I didn't 😞
But two slight questions remain for me...
(1) whether it would be better to leave the 'test_empty_slice' test in place,
as a kind of "behaviour regression" test.
The problem that this older code addressed is essentialy one of performance, but performance testing isn't really practical. So instead, you can (continue to) check that the data is not fetched when creating the lazy wrapper.
(2) whether we should change the Iris version requirement
The newer approach in iris._lazy_data should fix the performace hit.
That is now in v3.0.2, but I think not yet back in master.
Of course, a 'tidied' iris-grib would now get a nasty performance hit if used with an older Iris.
| @@ -43,31 +43,5 @@ def test_bitmap__invalid_indicator(self): | |||
| data_proxy._bitmap(section_6) | |||
|
|
|||
|
|
|||
| class Test_emptyfetch(tests.IrisGribTest): | |||
There was a problem hiding this comment.
I'm just wondering if it would make sense to leave this in as a "regression" testcase ?
There was a problem hiding this comment.
A difficult question! Maybe side with caution and leave it in? We can remove it later on if it causes problems.
On a side note I think we should ASV (aka performance) testing for this sort of thing. I had started thinking about it last week after SciTools/iris#4141.
There was a problem hiding this comment.
if it would make sense to leave this in as a "regression" testcase ?
Ok sorry I didn't read this closely enough.
This test will now fail, as we no longer intercept zero-length slicing.
You could modify it to call iris._lazy_data._as_lazy_data + check that that didn't 'visit' the data, but that's really testing Iris (which we do elsewhere), so not really appropriate here.
So, let's scrap it, as you originally suggested. 👍
|
Okay sounds to me like we want to have a minimum requirement of Also, looking over the codebase, it might make sense to rationalise the I'm happy to do all that, as I'm spinning that plate for @TomDufall Are you working to a specific deadline or milestone? Otherwise, we could quickly iterate on this and bank it all in an |
|
My main motivation was getting tests to pass in master to mess about with Flake8/Pytest as CPD. |
Personally I'd prefer we move quickly on this. We have some users requiring the latest iris-grib soon, so I would lean more to a 0.17.1 release (and if we go for that, this PR should point at the v0.17.x branch) |
|
I think the bigger urgency is that this breaks existing releases unless users work out that they can pin Iris to an older version themselves. The shortest possible fix would be to specify Iris < 3.0.2, I don't have strong opinions on which of the various suggestions is best. |
| @@ -43,31 +43,5 @@ def test_bitmap__invalid_indicator(self): | |||
| data_proxy._bitmap(section_6) | |||
|
|
|||
|
|
|||
| class Test_emptyfetch(tests.IrisGribTest): | |||
There was a problem hiding this comment.
if it would make sense to leave this in as a "regression" testcase ?
Ok sorry I didn't read this closely enough.
This test will now fail, as we no longer intercept zero-length slicing.
You could modify it to call iris._lazy_data._as_lazy_data + check that that didn't 'visit' the data, but that's really testing Iris (which we do elsewhere), so not really appropriate here.
So, let's scrap it, as you originally suggested. 👍
|
It looks like everything has now been resolved, so I am going to merge this in, so we can cut and release 0.17.1 today |
|
Note: I will be adding the pin in a follow up PR that updates the version number |
|
Thanks @TomDufall ! |
As discovered in #269,
iris.util._array_slice_ifemptyhas been removed and this breaks iris-grib builds.The PR that removed it from Iris said that it was no longer necessary so, I assume, it is also no longer necessary in iris-grib for the same reasons and I've removed it following the style of the Iris PR.
While I have the skill to technically implement this change, I don't have the requisite knowledge to say whether or not this is the correct decision - this is the bit that needs reviewing most!