Skip to content

BLD do not cythonize sdist #15335

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

Closed
wants to merge 1 commit into from
Closed

Conversation

glemaitre
Copy link
Member

closes #14863

Intend to not cythonize file when running python setup.py sdist as in https://github.com/numpy/numpy/pull/14453/files

@rth
Copy link
Member

rth commented Oct 22, 2019

Are you sure setup_requires works? njsmith was somewhat dubious about it in #7867 (comment)

PEP 518 might be the solution one day, but preferably I would rather see other scientific python projects adopting it first and working around possible issues :)

@glemaitre
Copy link
Member Author

I looked around a bit more and it seems that build-time dependencies are not an easy thing to solve.

I don't think that we should add cython to install_requires since we don't require it at runtime. Adding it to setup_requires will require some workaround, e.g. https://github.com/Blosc/bcolz/blob/master/setup.py#L34-L78

As you mentioned, PEP518 might be the solution. Actually scipy did something in this direction by adding a pyproject.toml and add the build-system dependencies (since version 1.0.0). This might good to go toward the same direction.

@thomasjpfan
Copy link
Member

It seems like numpy and scipy both have a pyproject.toml file. This looks like it enables one to run:

pip install git+https://github.com/numpy/numpy

on a fresh virtualenv (without cython) and it installs correctly. (pip 19.3.1)

@rth
Copy link
Member

rth commented Oct 23, 2019

The minimum supported version for PEP518 is pip v10 I think (or maybe even later). I wonder what happens with an older pip...

@jnothman
Copy link
Member

I think the consensus is to have this in for 0.22.

@thomasjpfan
Copy link
Member

thomasjpfan commented Oct 28, 2019

Are we going to be okay with bumping this up to Cython 1.0 3.0 for the 0.23? Cython 1.0 3.0 will have cython/cython#3118 which we can take advantage of.

Edit Cython 3.0

@adrinjalali adrinjalali added this to the 0.22 milestone Oct 28, 2019
@adrinjalali adrinjalali added the High Priority High priority issues and pull requests label Oct 28, 2019
@adrinjalali
Copy link
Member

Are we going to be okay with bumping this up to Cython 1.0 for the 0.23?

Isn't Cython going to be 3.0 in its next release? Anyhow, I guess our consensus was that eventhough we're going to have a cython dependency documented there with its version, but we're going to be very liberal with bumping up that dependency.

@thomasjpfan
Copy link
Member

Can we move forward with the pyproject.toml here like in https://github.com/numpy/numpy/pull/14453/files?

@adrinjalali
Copy link
Member

I'm very happy to see this go forward, but I remember one reason I didn't bring it up a while back, was this: https://discuss.python.org/t/pip-19-1-and-installing-in-editable-mode-with-pyproject-toml/1553/64

@rth
Copy link
Member

rth commented Oct 30, 2019

I'm very happy to see this go forward, but I remember one reason I didn't bring it up a while back, was this: discuss.python.org/t/pip-19-1-and-installing-in-editable-mode-with-pyproject-toml/1553/64

Yes, that was unfortunate but it was resolved in 19.1.1 I think (and introduced in 19.1.0) so it shouldn't be too much of an issue given that only affected party is scikit-learn contributors.

@jeremiedbb jeremiedbb added Superseded PR has been replace by a newer PR and removed High Priority High priority issues and pull requests labels Nov 6, 2019
@ogrisel
Copy link
Member

ogrisel commented Nov 6, 2019

Closing in favor of #15533.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Superseded PR has been replace by a newer PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stop cythonizing sdist PyPi releases
7 participants