Always run cythonize on sdist installation#4619
Conversation
kmaehashi
changed the title
|
Ready for review, updated the description. |
|
Jenkins CI test (for commit 3413f4b, target branch master) failed with status FAILURE. |
| 'NOTICE: Skipping cythonize as {} does not exist.'.format(pyx)) | ||
| if not use_cython: | ||
| pyx = base + '.cpp' | ||
| pyx = base + ('.pyx' if use_cython else '.cpp') |
There was a problem hiding this comment.
This is a hacky part (it's better to do refactoring). When doing pip install cupy:
- setup.py: use
use_cython=Falseto create a list of extension CPP sources generated by Cython (*.cpp) - setup.py: pass it to
setuptools.setup()(at the bottom of the script) - setuptools: install Cython, then run
build_ext - cupy_setup_build.py: use
use_cython=Trueto create a list of extension PYX sources and cythonize them (e.g., generatecore.cppfromcore.pyx). - setuptools: build the resulting CPP files, using the list generated in 1.
So use_cython is still needed here.
|
Jenkins, test this please |
|
Jenkins CI test (for commit fe3b254, target branch master) succeeded! |
|
Instead of using FWIW setuptools has builtin support for this. We could just add Cython to |
|
@jakirkham Thanks for the nice suggestion! Moved the dependencies to We should be aware that pip v19.0 (released on 2019-01-22) is the first version that supports |
|
pfnCI, test this please. |
|
Jenkins CI test (for commit 6ab58ba, target branch master) failed with status FAILURE. |
|
That's a good point Kenichi 🙂 It looks like Setuptools version 36.6.0 first added support for PEP 517. We could just check if Setuptools is older than that. If it is, we can add |
|
I see, so should we check versions of both setuptools and pip? I think it's also ok to always specify |
|
pfnCI, test this please. |
|
Jenkins CI test (for commit 6ac34e1, target branch master) failed with status FAILURE. |
|
Blocked by chainer/chainer-test#634 |
kmaehashi
force-pushed
the
cython-dependency
branch
from
6ac34e1 to
6ed33ae
Compare
|
The change looks good to me! Waiting for the blocker merged. |
|
Jenkins, test this please |
|
Jenkins CI test (for commit 6ed33ae, target branch master) succeeded! |
kmaehashi
force-pushed
the
cython-dependency
branch
from
6ed33ae to
c542c70
Compare
|
pfnCI, test this please. |
|
Jenkins CI test (for commit c542c70, target branch master) failed with status FAILURE. |
|
pfnCI, test this please. |
|
Jenkins CI test (for commit c542c70, target branch master) failed with status FAILURE. |
|
Jenkins, test this please |
|
Jenkins CI test (for commit c542c70, target branch master) failed with status FAILURE. |
|
Jenkins, test this please |
|
Jenkins CI test (for commit c542c70, target branch master) succeeded! |
|
@kmaehashi will this be backported for fixing #4597 in v8? |
|
let me backport it |
emcastillo
added
the
to-be-backported
Always run cythonize on sdist installation
6 participants
This PR removes C++ sources from sdist and changes the CuPy installer to always run cythonize at user-side.
Cython will automatically (temporarily) be downloaded by setuptools via
setup_requiresconfiguration.The primary motivation for this is to use compile-time constants in the code appropriately (closes #4597).
Besides that, cythonize at user-side is also needed for #4395 which generates pyx during the installation.