Skip to content

Remove gdal and other optional dependency#94

Merged
xylar merged 6 commits intoconda-forge:masterfrom
xylar:remove_gdal_dependency
Oct 19, 2020
Merged

Remove gdal and other optional dependency#94
xylar merged 6 commits intoconda-forge:masterfrom
xylar:remove_gdal_dependency

Conversation

@xylar
Copy link
Copy Markdown
Contributor

@xylar xylar commented Oct 16, 2020

It should be optional.

Checklist

  • Used a personal fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Reset the build number to 0 (if the version changed)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

It should be optional.
@conda-forge-linter
Copy link
Copy Markdown

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:

  • License is not an SPDX identifier (or a custom LicenseRef) nor an SPDX license expression.

Documentation on acceptable licenses can be found here.

@xylar
Copy link
Copy Markdown
Contributor Author

xylar commented Oct 16, 2020

I saw mentioned here: SciTools/iris#3762 (comment)
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
Copy link
Copy Markdown
Contributor Author

xylar commented Oct 16, 2020

@conda-forge-admin, please rerender

@xylar xylar force-pushed the remove_gdal_dependency branch from a312624 to bb4906b Compare October 16, 2020 21:11
@xylar
Copy link
Copy Markdown
Contributor Author

xylar commented Oct 16, 2020

@conda-forge-admin, please rerender

@xylar xylar force-pushed the remove_gdal_dependency branch from 74e0009 to eda9786 Compare October 16, 2020 21:18
@xylar xylar changed the title Remove gdal as a dependency Remove gdal and other optional dependency Oct 16, 2020
@xylar
Copy link
Copy Markdown
Contributor Author

xylar commented Oct 16, 2020

@conda-forge-admin, please rerender

@xylar
Copy link
Copy Markdown
Contributor Author

xylar commented Oct 16, 2020

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

@xylar xylar force-pushed the remove_gdal_dependency branch from 7471a86 to 463cb4d Compare October 16, 2020 21:33
@xylar
Copy link
Copy Markdown
Contributor Author

xylar commented Oct 16, 2020

@conda-forge-admin, please rerender

conda-forge-linter and others added 2 commits October 16, 2020 21:35
@xylar xylar force-pushed the remove_gdal_dependency branch from 62efd27 to 5a9cd30 Compare October 16, 2020 21:55
@xylar
Copy link
Copy Markdown
Contributor Author

xylar commented Oct 16, 2020

@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.

Comment thread recipe/meta.yaml
Co-authored-by: Isuru Fernando <isuruf@gmail.com>
@bjlittle
Copy link
Copy Markdown

@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
Copy link
Copy Markdown
Contributor Author

xylar commented Oct 19, 2020

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

@xylar xylar merged commit a470fd1 into conda-forge:master Oct 19, 2020
@xylar xylar deleted the remove_gdal_dependency branch October 19, 2020 09:11
@danschef
Copy link
Copy Markdown

@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
Copy link
Copy Markdown
Contributor Author

xylar commented Oct 20, 2020

@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
Copy link
Copy Markdown
Contributor Author

xylar commented Oct 20, 2020

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

@danschef
Copy link
Copy Markdown

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
Copy link
Copy Markdown
Contributor Author

xylar commented Oct 20, 2020

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

@danschef
Copy link
Copy Markdown

Yes, I know.

@mathause
Copy link
Copy Markdown

mathause commented Oct 22, 2020

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
Copy link
Copy Markdown
Contributor Author

xylar commented Oct 22, 2020

@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
Copy link
Copy Markdown
Contributor Author

xylar commented Oct 22, 2020

@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
Copy link
Copy Markdown

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
Copy link
Copy Markdown
Member

ocefpaf commented Dec 2, 2020

@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
Copy link
Copy Markdown
Contributor Author

xylar commented Dec 2, 2020

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

@dopplershift
Copy link
Copy Markdown
Member

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
Copy link
Copy Markdown
Contributor Author

xylar commented Dec 2, 2020

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
Copy link
Copy Markdown
Member

ocefpaf commented Dec 2, 2020

@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
Copy link
Copy Markdown
Member

dopplershift commented Dec 2, 2020

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. 😉

@xylar
Copy link
Copy Markdown
Contributor Author

xylar commented Dec 3, 2020

@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
Copy link
Copy Markdown
Member

ocefpaf commented Dec 3, 2020

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
Copy link
Copy Markdown
Contributor Author

xylar commented Dec 3, 2020

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

@ocefpaf
Copy link
Copy Markdown
Member

ocefpaf commented Dec 3, 2020

@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
Copy link
Copy Markdown
Contributor Author

xylar commented Dec 3, 2020

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.

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.

8 participants