Skip to content
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

Add numpy as setup requirement #67

Closed
wants to merge 4 commits into from

Conversation

kylebarron
Copy link

We got a downstream bug report of rio-color's install failing because the user didn't have numpy installed when he tried to install rio-color, so the headers couldn't be found.

This is a bit of a chicken-and-egg issue, because the numpy headers are needed to build, but you can't find them before installing numpy. This PR uses a technique I learned from pybind11 (though I can't find now exactly where in the documentation it was referenced), which uses a class to postpone finding numpy's headers until numpy is installed.

Tested locally (using conda just to create a virtual env):

Setup:

> conda create -n minimal-test -c conda-forge python -y
> source activate minimal-test

Before:

> python setup.py bdist_wheel
Numpy and its headers are required to run setup(). Exiting.

After, creating the wheel works.

@kylebarron
Copy link
Author

It looks like you could use this same technique in rasterio
https://github.com/mapbox/rasterio/blob/1d43837d1591c7bc3cc87c97e45f153063103d57/setup.py#L88-L92

@kylebarron
Copy link
Author

@vincentsarago suggested a generator instead of a class, and it seems to work fine as well

setup.py Outdated

def __str__(self):
import numpy
return numpy.get_include()
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe the class is overpriced!
a simple function should be enough

def get_numpy_include():
    import numpy
    yield numpy.get_include()

and the in the setup() do

    include_dirs=[get_numpy_include()],
    setup_requires=["numpy"],

🤷‍♂️

Copy link
Contributor

Choose a reason for hiding this comment

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

:homer:

Copy link
Author

Choose a reason for hiding this comment

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

image

@sgillies sgillies self-requested a review December 7, 2020 15:32
@sgillies
Copy link
Contributor

sgillies commented Dec 7, 2020

We don't want to make rasterio more like this PR. Instead we want to make this project more like rasterio. See #64 😄

setup_requires hasn't ever really worked. The modern approach is to give the project a file like https://github.com/mapbox/rio-color/blob/master/pyproject.toml defining its build requirements. The build system will fetch numpy before building and then the code at https://github.com/mapbox/rio-color/blob/master/setup.py#L14-L21 will (in theory) never exit.

I'm happy to discuss this more but we're not going to merge this PR. The big blocker right now is #65 and my lack of time to review #66. However, I can see the light at the end of the tunnel.

@sgillies
Copy link
Contributor

sgillies commented Dec 7, 2020

@vincentsarago @kylebarron since you're writing packages that depend on rio-color and have conda users, it could be good to add rio-color to conda-forge. I think they have good instructions and examples, shouldn't be too hard.

@kylebarron
Copy link
Author

I see... pip install git+https://github.com/mapbox/rio-color does work in my minimal environment, fetching Numpy first. I didn't check for new unreleased commits.

I personally get very frustrated dealing with Conda packaging. Someone made a Conda release of rio-tiler v1, but since then we've added more dependencies, so once rio-tiler v2 is released it'll take some time to become a conda package. Presumably at some point someone will make a rio-color dist on Conda.

@kylebarron kylebarron closed this Dec 7, 2020
@vincentsarago
Copy link
Contributor

@sgillies I'm not sure to fully understand the pyproject.toml but does this mean the issue has been resolved in master but not in pypi? again thanks for your time 🙏

@sgillies
Copy link
Contributor

sgillies commented Dec 8, 2020

Correct @vincentsarago . 1.0.0 did not specify build dependencies, but 1.0.1 will. The thing that's blocking me from releasing 1.0.1 is lack of working wheel building infra. If I made a source-only 1.0.1 release, this tracker would fill up with "I can't install 1.0.1" issues.

@vincentsarago
Copy link
Contributor

🙏 thanks @sgillies

@vincentsarago
Copy link
Contributor

I'll just ping @jacquestardie so he finds some time for you then!

@sgillies
Copy link
Contributor

sgillies commented Dec 8, 2020

@vincentsarago ping him hourly!

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.

None yet

3 participants