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.

Sign up for GitHub

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

Jump to bottom

Always run cythonize on sdist installation #4619

Merged
merged 8 commits into from
Feb 24, 2021
Merged

Always run cythonize on sdist installation #4619

merged 8 commits into from
Feb 24, 2021

Conversation

kmaehashi
Copy link
Member

@kmaehashi kmaehashi commented Feb 4, 2021 •
edited
Loading

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_requires configuration.

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.

@kmaehashi kmaehashi added cat:bug Bugs no-compat Disrupts backward compatibility prio:high labels Feb 4, 2021
@kmaehashi kmaehashi changed the title [WIP] Always run cythonize on sdist installation Always run cythonize on sdist installation Feb 4, 2021
@kmaehashi
Copy link
Member Author

Ready for review, updated the description.
pfnCI, test this please.

@chainer-ci
Copy link
Member

Jenkins CI test (for commit 3413f4b, target branch master) failed with status FAILURE.

leofang
leofang reviewed Feb 4, 2021
View reviewed changes
cupy_setup_build.py Outdated Show resolved Hide resolved
cupy_setup_build.py
'NOTICE: Skipping cythonize as {} does not exist.'.format(pyx))
if not use_cython:
pyx = base + '.cpp'
pyx = base + ('.pyx' if use_cython else '.cpp')
Copy link
Member

Choose a reason for hiding this comment

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

I thought .cpp is removed?

Copy link
Member Author

@kmaehashi kmaehashi Feb 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This is a hacky part (it's better to do refactoring). When doing pip install cupy:

  1. setup.py: use use_cython=False to create a list of extension CPP sources generated by Cython (*.cpp)
  2. setup.py: pass it to setuptools.setup() (at the bottom of the script)
  3. setuptools: install Cython, then run build_ext
  4. cupy_setup_build.py: use use_cython=True to create a list of extension PYX sources and cythonize them (e.g., generate core.cpp from core.pyx).
  5. setuptools: build the resulting CPP files, using the list generated in 1.

So use_cython is still needed here.

@leofang
Copy link
Member

leofang commented Feb 4, 2021

Jenkins, test this please

@chainer-ci
Copy link
Member

Jenkins CI test (for commit fe3b254, target branch master) succeeded!

@kmaehashi kmaehashi mentioned this pull request Feb 5, 2021
Closed
@jakirkham
Copy link
Member

jakirkham commented Feb 5, 2021 •
edited
Loading

Instead of using setup_requires what do you think about using pyproject.toml from PEP 517? This fixed some warts with setup_requires IIRC

FWIW setuptools has builtin support for this. We could just add Cython to requires there

@kmaehashi
Copy link
Member Author

@jakirkham Thanks for the nice suggestion! Moved the dependencies to pyproject.yaml.

We should be aware that pip v19.0 (released on 2019-01-22) is the first version that supports pyproject.yaml.
I believe most users are OK (we suggest upgrading pip in the installation guide), but we can add setup_requires to setup.py as a fallback for old pip users anytime.

@kmaehashi
Copy link
Member Author

pfnCI, test this please.

@chainer-ci
Copy link
Member

Jenkins CI test (for commit 6ab58ba, target branch master) failed with status FAILURE.

@jakirkham
Copy link
Member

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 setup_requires.

@kmaehashi
Copy link
Member Author

kmaehashi commented Feb 5, 2021 •
edited
Loading

I see, so should we check versions of both setuptools and pip?

I think it's also ok to always specify setup_requires if we have concern 😃 There's no harm for pip v19+ users.

@kmaehashi
Copy link
Member Author

pfnCI, test this please.

@chainer-ci
Copy link
Member

Jenkins CI test (for commit 6ac34e1, target branch master) failed with status FAILURE.

@takagi takagi added this to the v9.0.0b3 milestone Feb 9, 2021
@emcastillo emcastillo mentioned this pull request Feb 18, 2021
Closed
@kmaehashi kmaehashi mentioned this pull request Feb 19, 2021
Merged
@kmaehashi
Copy link
Member Author

Blocked by chainer/chainer-test#634

@takagi takagi added the st:blocked-by-another-pr Blocked by another pull-request label Feb 20, 2021
@takagi
Copy link
Member

takagi commented Feb 20, 2021

The change looks good to me! Waiting for the blocker merged.

@kmaehashi kmaehashi mentioned this pull request Feb 23, 2021
Merged
@emcastillo
Copy link
Member

Jenkins, test this please

@chainer-ci
Copy link
Member

Jenkins CI test (for commit 6ed33ae, target branch master) succeeded!

@takagi takagi removed the st:blocked-by-another-pr Blocked by another pull-request label Feb 23, 2021
@takagi
Copy link
Member

takagi commented Feb 23, 2021

pfnCI, test this please.

@takagi takagi added the st:test-and-merge (deprecated) Ready to merge after test pass. label Feb 23, 2021
@chainer-ci
Copy link
Member

Jenkins CI test (for commit c542c70, target branch master) failed with status FAILURE.

@takagi
Copy link
Member

takagi commented Feb 23, 2021

pfnCI, test this please.

@chainer-ci
Copy link
Member

Jenkins CI test (for commit c542c70, target branch master) failed with status FAILURE.

@leofang
Copy link
Member

leofang commented Feb 23, 2021

Jenkins, test this please

@chainer-ci
Copy link
Member

Jenkins CI test (for commit c542c70, target branch master) failed with status FAILURE.

@leofang
Copy link
Member

leofang commented Feb 23, 2021

Jenkins, test this please

@chainer-ci
Copy link
Member

Jenkins CI test (for commit c542c70, target branch master) succeeded!

@mergify mergify bot merged commit 226a43b into cupy:master Feb 24, 2021
@kmaehashi kmaehashi deleted the cython-dependency branch February 24, 2021 02:11
@leofang
Copy link
Member

leofang commented Feb 24, 2021

@kmaehashi will this be backported for fixing #4597 in v8?

@emcastillo
Copy link
Member

let me backport it

@emcastillo emcastillo added the to-be-backported Pull-requests to be backported to stable branch label Feb 24, 2021
emcastillo pushed a commit to emcastillo/cupy that referenced this pull request Feb 24, 2021
@mergify
Merge pull request cupy#4619 from kmaehashi/cython-dependency
e618d2e 
Always run cythonize on sdist installation
@emcastillo emcastillo mentioned this pull request Feb 24, 2021
Merged
This was referenced Feb 24, 2021
Closed
Merged
Merged
@kmaehashi kmaehashi mentioned this pull request Mar 21, 2021
Closed
@kmaehashi kmaehashi mentioned this pull request Aug 13, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leofang leofang leofang left review comments

Assignees

@takagi takagi

Labels
cat:bug Bugs no-compat Disrupts backward compatibility prio:high st:test-and-merge (deprecated) Ready to merge after test pass. to-be-backported Pull-requests to be backported to stable branch
Projects
None yet
Milestone
v9.0.0b3
Development

Successfully merging this pull request may close these issues.

BUG: CUDA_VERSION is set to 0 in sdist
6 participants
@kmaehashi @chainer-ci @leofang @jakirkham @takagi @emcastillo