Skip to content

Conversation

@pp-mo
Copy link
Member

@pp-mo pp-mo commented Feb 9, 2018

Should be at least a temporary fix for the current test problems on master.
I moved it from the "all" to the "extensions" dependency-group (latter not used by Travis).

For the future, I would really like to kick gdal out of our requirements altogether...

It is only used by iris.experimental.raster.export_geotiff.
We have managed without in the internal SSS environments for some time,
if you "import iris.experimental.raster" in these, it simply fails.

Gdal seems to be a very large package of which we're only using a tiny part.
The dependencies on conda-forge are an immensely complicated + over-constrained lot.
No wonder it has caused trouble
For instance, the latest problem is due to the move from 2.2.3_1 to 2.2.3_3 (that is, different build versions of libgdal 2.2.3) : We were ok with the build-1 version, but it has been removed from the channel.

@pp-mo pp-mo changed the title Remove gdal from 'all' to 'extensions' depedency group. Stop testing with gdal. Feb 9, 2018
@pp-mo pp-mo requested review from DPeterK and pelson February 12, 2018 15:20
@cpelley
Copy link

cpelley commented Feb 13, 2018

We have managed without in the internal SSS environments for some time,

We have worked around it for quite some time (having a gdal conda-forge build in our own environments). It was a great deal of effort for me and @marqh to get it into the SSS. Really appreciate the effort @marqh made to help us with it, getting it into experimental.

@cpelley
Copy link

cpelley commented Feb 13, 2018

Should be at least a temporary fix for the current test problems on master.

I think this means that iris gdal func. would no longer be tested no?
👎

@cpelley
Copy link

cpelley commented Feb 13, 2018

As an aside, the number of users making use of gdal is likely underestimated. A number of people I have seen using the old /usr/local/sci/bin/python, for that purpose.

@DPeterK
Copy link
Member

DPeterK commented Feb 13, 2018

iris gdal func. would no longer be tested

That's correct, although there's pretty much no gdal functionality in Iris, and what there is is experimental only and subject to change/removal without warning. As it stands though, the Iris tests simply will not pass on Travis unless we do something to mitigate the broken gdal build that the Travis environments are picking up, which is a very bad state to be in.

Note also that removal of testing glal elements in Iris implies neither that gdal functionality will be removed from Iris nor that gdal will be removed from software environments.

Copy link
Member

@DPeterK DPeterK left a comment

Choose a reason for hiding this comment

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

@pp-mo the changes here are both small and make the tests pass, which receives a big 👍 from me. One question though: I see that gdal tests are now skipped on Travis - has there been a code change made to skip glal-reliant tests or did we just get this for free?

@DPeterK DPeterK merged commit 8a327b4 into SciTools:master Feb 13, 2018
@pp-mo
Copy link
Member Author

pp-mo commented Feb 13, 2018

I see that gdal tests are now skipped on Travis - has there been a code change made to skip glal-reliant tests or did we just get this for free?

No, its standard : Iris tests should run with only the essential dependencies. As gdal is optional, the tests have a skipper (just like for pandas, iris-grib, stratify, nc_time_axis etc.)

@pp-mo
Copy link
Member Author

pp-mo commented Feb 13, 2018

It was a great deal of effort for me and @marqh to get it into the SSS.

Well that really is the whole point : the problem with these cases is that the work is never over once you just have something working : ongoing maintenance effort is always required, the only question is how much.
In this case, this particular package is a serial offender : we keep encountering serious difficulties in accommodating it alongside the other things we need.

So the current situation is :

  • we have no gdal in our existing default desktop environments ("current" or "next"), because it was a lot of work
  • there is a package in our current "experimental-current" environment, following some special effort, but it is an older version (2.1.0)
  • the latest releases (2.2) seem to introduce a real dependency headache, so we have now removed this from testing + will wait to see what happens

@cpelley
Copy link

cpelley commented Feb 13, 2018

merged?? 😱

I have been getting more and more concerned over the direction in which iris.experimental has been taking of late and of the potential impact that may have.

ping @jkettleb, we may have to consider what this means for ANTS both for experimental and the situation around the SSS.

@DPeterK
Copy link
Member

DPeterK commented Feb 13, 2018

@cpelley I assume then you haven't read any of the comments that @pp-mo or I have made. Without this Iris cannot move, which is a completely unacceptable situation to be in and is blocking the release of Iris v2.

The fact that one more bit of Iris experimental is no longer tested on Travis only is really neither here nor there, and the approach to experimental code in Iris has remained exactly the same as always it has been - it is subject to change and removal without warning. If you're relying on experimental code in downstream systems then preferably (a) - don't, or (b) - you have to accept that risk. If the experimental code is really valuable to you then perhaps it is time we started considering moving the code out of experimental, but so far as we knew, no-one was using it.

P.S. To reiterate, the changes here have no effect on the contents of software stack environments, nor does it imply any changes are upcoming. This is a pragmatic change on Travis only that affects Travis only.

@ajdawson
Copy link
Member

If the experimental functionality is proven valuable then the code ought not to live in an experimental namespace anymore. However, considering this particular code brings with it such a heavy dependency perhaps it would be a good candidate for a spin-out package? This solution would maintain testing and support for the component without forcing iris to add complexity to its dependencies and testing.

@DPeterK
Copy link
Member

DPeterK commented Feb 13, 2018

@ajdawson we're always 👍 for spin-out packages 😄

@cpelley
Copy link

cpelley commented Feb 13, 2018

I'm +1 on pulling it out into it's own project. We can put our raster import alongside it (something we couldn't get review time from iris devs with).

I don't agree however on experimental usage. Neither do I agree that is a case of "same as always it has been". Perhaps I'm missing something, but given these tests are optional and now we explicitly don't even care if travis runs them, then how can you know if the code works or not?
It assumes the developer/reviewer has run these optional tests (i.e. they have gdal installed).

I'm honestly happy if I'm wrong about this!

@hdyson
Copy link
Contributor

hdyson commented Feb 13, 2018

If the experimental code is really valuable to you then perhaps it is time we started considering moving the code out of experimental, but so far as we knew, no-one was using it.

Is there a documented path somewhere for end users to encourage code to be migrated from experimental to a more stable location (whether in iris proper or a spun out package)?

@dkillick I saw you give a presentation last year on logging all the pythons - did your logging go to the extent of identifying which parts of iris were being used? It might be valuable information for you, rather than relying on end user feedback. We tend to only moan about the things that break, rather than mention the things that just work.

@DPeterK
Copy link
Member

DPeterK commented Feb 13, 2018

Is there a documented path somewhere

Nope, but an Iris issue or a comment on the google groups might get some traction...

did your logging go to the extent of identifying which parts of iris were being used?

It didn't but it could, and it's part of the reason we put the logging together!

We tend to only moan about the things that break

And this is the sad frustration of situations like this. We're making a positive step forward that we need to make and that's unfortunately causing the potential for breaking someone's workflow. Regrettably, this is simply the way of things, but it isn't a reason to stop or not do something.

@hdyson
Copy link
Contributor

hdyson commented Feb 13, 2018

I don't want to distract from @cpelley's last comment: I'd be asking similar questions about testing too, if he hadn't already covered them.

Nope, but an Iris issue or a comment on the google groups might get some traction...

We currently rely on iris.experimental.um, and iris.experimental.raster. Absent a process for encouraging migrating these to stable locations, I'm sure you can understand why we're suddenly nervous. Can we be confident these will survive at least until iris v2.0 is released?

It didn't but it could, and it's part of the reason we put the logging together!

Sounds good - and forgive the poor manners on my part for not mentioning that it was a good presentation. The ratio of usage of old iris vs the stack environments was revelatory.

So the current situation is :
we have no gdal in our existing default desktop environments ("current" or "next"), because it was a lot of work
there is a package in our current "experimental-current" environment, following some special effort, but it is an older version (2.1.0)
the latest releases (2.2) seem to introduce a real dependency headache, so we have now removed this from testing + will wait to see what happens

@pp-mo I'm assuming the alternative of dropping back to the known good 2.1.0 wouldn't enable the testing to resume without investing too much effort?

@pelson
Copy link
Member

pelson commented Feb 14, 2018

Just to add my 2 cents - I'm completely supportive of this change. Iris development effort is a finite resource, and we appear to be pretty well over the limit (@cpelley's statement about failing to get a review is a case in point).

I can't imagine anybody would think that iris cube to GeoTIFF conversion was not a valuable piece of functionality, but does it belong in core iris? If cube <> grib doesn't, then I'm pretty confident that GeoTIFF doesn't either.

With that in mind, and the comments here regarding surprise that "experimental" is not considered stable, would anybody like to take ownership of a new repository in SciTools-incubator called iris-gdal / iris-raster / iris-something-else? Because we are so close to v2.0.0 iris.experimental.raster will remain in that version, but because it is experimental, it can readily be removed in favour of a 3rd party package in v2.1 should it be so desired.

To be clear, one of the key reasons for splitting functionality out of Iris is so that the spin-offs can control their own release cycle and CI as necessary. It means that core Iris can be leaner (with shorter CI), but it does mean that breaking changes in iris has more of a downstream impact, as it falls on the spin-off package maintainers to support the changes in the latest core-iris.

@cpelley
Copy link

cpelley commented Feb 14, 2018

Happy to take ownership of a new repo. We already own gdal import within ANTS and manage changes to support the latest release of iris there, so I see little problem to extending that responsibility to export too. I suppose the name iris-raster is more likely to be better encompassing :)

Thanks @ajdawson , @dkillick , @pp-mo , @hdyson and @pelson for sharing your thoughts on this, appreciated.
We'll take a look at our usage of experimental and consider what we need to do.

Thanks again.

@pelson
Copy link
Member

pelson commented Feb 14, 2018

@cpelley - I've created a repo at https://github.com/SciTools-incubator/iris-raster which you have admin rights on (that should allow you the freedom to do anything to that repository, including adding other members as appropriate).

@pp-mo
Copy link
Member Author

pp-mo commented Feb 14, 2018

The problem with gdal 2.1.0 is it actually contains a bug which is fixed in in 2.2, but which causes our test to fail. That is the immediate cause of the current problem.

Gdal 2.2 is quite different from 2.1 and has added a bunch of extra dependencies, including libgdal, boost, geotiff, libmkl, and poppler

Unfortunately there is a reason we can't use 2.2 at this point, whereas before we actually could : The old build (2.2.3_0) of libgdal which "used to work" for us has been deleted from the conda-forge channel. It's not usual to remove builds, possibly its dependencies were not correct (just guessing here really) ?
Meanwhile the remaining 2.2 build there (2.2.3_3) has additional dependency limitations which mean it is now incompatible with matplotlib<1.9. I think it is tighter limits on exact package versions, like
libpng>=1.6.32,<1.6.35 (was >=1.6.28,<1.7) and libtiff >=4.0.8,<4.0.10 (was 4.0.*)

When I tried to unpick this by speculatively removing version requirements for specific packages, I got results like this ...

Created a conda-requirements.txt from "matplotlib<1.9" plus all the deps from
(latest=2.2.3) libgdal (from conda-forge feedstock repo).

    $ conda create -n x --dry-run -y --file trial_conda_reqts.txt

Problems like ...

* initial version:
    UnsatisfiableError: The following specifications were found to be in conflict:
      - libpng >=1.6.32,<1.6.35
      - matplotlib <1.9
      - poppler 0.61.*

* remove poppler versioning ==>
    UnsatisfiableError: The following specifications were found to be in conflict:
      - libpng >=1.6.32,<1.6.35
      - matplotlib <1.9
      - openjpeg 2.3.*
      - poppler

* remove openjpeg versioning ==> OK

* revert to original, remove libpng versioning ==>
    UnsatisfiableError: The following specifications were found to be in conflict:
      - libtiff >=4.0.8,<4.0.10
      - matplotlib <1.9
      - poppler 0.61.*

This was just speculative stuff trying to find out what the basic is : the conclusion is "it's hopelessly complicated".

If you want a separate repo to handle this, you still are going to have the same problems getting version compatibility. I don't see any easy win here.

@cpelley
Copy link

cpelley commented Feb 14, 2018

Thanks for additional context @pp-mo.

@pelson
Copy link
Member

pelson commented Feb 14, 2018

Cherry-picked into v2.0.x in 7e7bfab.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants