-
Notifications
You must be signed in to change notification settings - Fork 389
restore PEP-517 support #1681
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
restore PEP-517 support #1681
Conversation
Co-authored-by: "Noah D. Brenowitz" <[email protected]>
requirements/default.txt
Outdated
| @@ -1,4 +1,3 @@ | |||
| numpy>=1.10 | |||
| numpy>=1.13 | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cartopy only supports python 3.5+, and numpy only started 3.5 support in v1.13.3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are actually only testing against 3.6 on master now, so it is even newer than that!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably worth doing NEP29 and pyupgrade in a follow-up PR
|
+1 for making cartopy PEP-517 complaint again. This simplifies usage of this package alot on modern serverless architectures with limited configuration options other than requirements.txt. Now we automatically resort to custom docker images because we need multiple installation steps to install all dependencies. |
|
@thijsvanwinden Wouldn't you still have issues with needing to build against GEOS and PROJ? Those dependencies can't appear in |
|
You are totally right. Overlooked that in my build script when updating to the cartopy 0.18. Still I think PEP-517 compliance is desirable. |
|
You'll still have the same problems with GEOS and PROJ, but that's much easier to solve than installing numpy before cartopy |
greglucas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with the sentiment here that this simplifies the pip installs all being on one line and it doesn't seem to have much of a downside as far as I can tell? Sure, you'll have to install the other packages, but that was the case even before this PR.
|
To be clear, I'm 👍 on this in principle, just don't want anyone thinking it's a magic bullet solving CartoPy install issues. It's definitely a step forward, though, |
|
Thanks @graingert! I think this will help. We can add other fixes in later if we need to clarify anything. |
|
@greglucas can you make a 0.19a0 pre-release with this in? I'm currently pulling it from my personal github repo |
|
That would be fine with me. I've been waiting for review of PRs from the list on #1703 before doing the release, but I don't see why those couldn't be put in after a pre-release? Honestly, at this point maybe we just need to get a 0.19 out the door and those can go into 0.20. @dopplershift and @QuLogic do you have any objections/thoughts? |
See SciTools/cartopy#1681 for background, hopefully this will be fixed in their next release.
* Remove python-epsg as requirement, add cartopy * Remove cartopy from requirements.txt See SciTools/cartopy#1681 for background, hopefully this will be fixed in their next release. * Update README * Reword a few things in README * Fix typos
Rationale
pip installs are not currently compliant with PEP-517. This PEP is relied upon by external tooling. This behavior was reverted in 454a911 because it made it harder to test cartopy against a set particular version of numpy. IMO it is not necessary to remove PEP-517 compliance to enable this testing pattern. Using python setup.py develop versus pip install is enough. See this example:
Implications
Cannot pip install cartopy with one pip command.
Checklist
Fixes #1552.
based on #1680