search

conda-forge / cartopy-feedstock

A conda-smithy repository for cartopy.
BSD 3-Clause "New" or "Revised" License
4 stars 16 forks source link

Remove gdal and other optional dependency #94

Closed xylar closed 4 years ago

xylar commented 4 years ago

It should be optional.

Checklist

conda-forge-linter commented 4 years ago

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

I do have some suggestions for making it better though...

For recipe:

Documentation on acceptable licenses can be found here.

xylar commented 4 years ago

I saw mentioned here: https://github.com/SciTools/iris/pull/3762#issuecomment-685573007 that gdal should be an optional dependency, and that's clearly true: https://github.com/SciTools/cartopy/blob/v0.18.0/requirements/plotting.txt

I'm not sure why I added it as required in #88. Maybe I was just confused about which were required and which not.

xylar commented 4 years ago

@conda-forge-admin, please rerender

xylar commented 4 years ago

@conda-forge-admin, please rerender

xylar commented 4 years ago

@conda-forge-admin, please rerender

xylar commented 4 years ago

I added not only gdal but several other dependencies in #88 that should have been optional. Sorry! 😭😭😭

xylar commented 4 years ago

@conda-forge-admin, please rerender

xylar commented 4 years ago

@conda-forge/cartopy, sorry for the mess in #88. I think this is ready for review if anyone cares to take a look. Based on that experience, another pair of eyes would be a good idea.

bjlittle commented 4 years ago

@xylar Thanks for doing this. The gdal package is a monster and ultimately would have caused dependency issues further down the line for cartopy in a conda environment - particularly as gdal pins the version of libnetcdf, which can restrict other packages from installing their latest versions 👍

xylar commented 4 years ago

I'm going ahead with the merge. Presumably, this won't make things worse than they currently are...

danschef commented 4 years ago

@xylar @bjlittle This PR removes pyepsg from the run requirements although it is still imported in _epsg.py (see here).

This leads to breaking code, e.g., when using the ccrs_from_epsg() function.

xylar commented 4 years ago

@danschef, I'm pretty sure you need to explicitly include pyepsg as a package in your environment if you want to use ccrs_from_epsg(). It is an optional dependency for those not using that code. Similarly, a lot of plotting won't work without matplotlib but it is still technically optional.

xylar commented 4 years ago

@danschef, however, feel free to open an issue and we can discuss this further.

danschef commented 4 years ago

I see, it is listed in the optional dependencies in the documentation. However, it would be nice if cartopy would raise a helpful exception in case optional deps are not installed - instead of just raising an ImportError. I will open an issue for that.

xylar commented 4 years ago

@danschef, that would be an issue for the cartopy repo, this is just a recipe for building cartopy for the conda-forge channel.

danschef commented 4 years ago

Yes, I know.

mathause commented 4 years ago

I find it a bit unfortunate that you did this without a version bump. Because stuff* that used to work in version 0.18 now doesn't work in 0.18...

Yes, I know I can easily add the dependencies. But my package doesn't, so I may have to issue a new release there (of course a new version wouldn't have helped my package). Anyway, sorry for the rant & thanks for all your work on cartopy.

* By stuff I mean ploting a plot.

xylar commented 4 years ago

@mathause, you need to install some of the optional dependencies for your work. We don't determine the version number at conda-forge, it's determined upstream.

xylar commented 4 years ago

@mathause, sorry for the terse answer earlier, and sorry that this change caused you trouble. The original build of 0.18.0 incorrectly included a lot of optional packages, and several folks were complaining about that. I have tried to contain the damage but clearly it's causing trouble for you the other direction.

Would it be possible in the context you're working in to just add the optional dependencies along with the cartopy package?

mathause commented 4 years ago

Thanks, appreciate it. I also didn't mean to be rude, apologies!

I did not know matplotlib & scipy were optional dependencies of cartopy - they were always included via conda (e.g. conda search --info cartopy=0.13). So I was surprised. I'll be able to work around it & think that this can be beneficial in the long term.

ocefpaf commented 3 years ago

@xylar and @bjlittle looking into this I'm not sure removing matplotlib makes sense. After all cartopy's third point on its overview is:

integration to expose advanced mapping in Matplotlib with a simple and intuitive interface
xylar commented 3 years ago

@ocefpaf, and yet, they have gone out of their way to make matplotlib an optional dependency...

dopplershift commented 3 years ago

While it is an optional dependency, I'm also not at all clear what utility CartoPy has in that case--or whether anyone is actually using it in that way.

xylar commented 3 years ago

I don't have a problem with adding matplotlib-base back as a dependency. I think it was gdal that folks were wanting to be optional. But I figured once I took that out I should follow the lead of the developers upstream...

ocefpaf commented 3 years ago

@dopplershift what do you think is appropriate for the conda package?

@xylar indeed, gdal was too much! That one must remain an optional dep.

PS:I'd love to heat what @bjlittle thinks about this too.

dopplershift commented 3 years ago

I think a dependency on matplotlib-base is appropriate and what most users would expect. We never heard any complaints in the first 4 years. :wink:

xylar commented 3 years ago

@ocefpaf and @dopplershift, I don't mind making a PR to add back matplotlib-base. From upstream, the plotting requirements are:

matplotlib>=1.5.1
GDAL>=1.10.0
pillow>=1.7.8
scipy>=0.10

We've already said that gdal is out because it's too big. What about pillow and scipy? There's not indication I can see from upstream of how these requirements can be mixed and matched. I would have assumed you need all 4 or plotting wouldn't work. But I certainly haven't experimented with that.

ocefpaf commented 3 years ago

I would add only matplotlib-base. scipy is for the KDTree stuff, pillow and gdal to handle images and raster transformation stuff. People doing those will actively install them regardless of cartopy. However, people wanting to make a map may try to install only cartopy just to find out they also need matplotlib. (Optional deps are hard!)

xylar commented 3 years ago

@ocefpaf, thanks for following up on this and for understanding the dilemma I was facing.

ocefpaf commented 3 years ago

@ocefpaf, thanks for following up on this and for understanding the dilemma I was facing.

Don't worry. I'm still on not 100% sure what is the best course for us here. I'm 80% on the site of leaving mpl b/c that is what all the users I know expect. Still, that is a subset of all the users.

xylar commented 3 years ago

I 100% agree that it's hard for me to imagine why anyone would install cartopy without matplotlib and I don't know of anyone who would want that. So I don't see any downside to adding the requirement back in, especially because no one as complained about it before and folks did complain when it was gone.

I'm curious why it's not a default requirement upstream but not curious enough to follow up there.