Skip to content

BLD do not cythonize sdist#15335

Closed
glemaitre wants to merge 1 commit intoscikit-learn:masterfrom
glemaitre:is/14863
Closed

BLD do not cythonize sdist#15335
glemaitre wants to merge 1 commit intoscikit-learn:masterfrom
glemaitre:is/14863

Conversation

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

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

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

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

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