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 rapids-cython component for scikit-build based Python package builds #184

Merged
merged 52 commits into from
May 6, 2022

Conversation

vyasr
Copy link
Contributor

@vyasr vyasr commented Apr 27, 2022

This PR introduces various utilities that will be used across all RAPIDS Python packages that are built using scikit-build. Centralizing these utilities reduces the CMake configurations of our Python libraries to a minimum. For now, all of the content is purely CMake code, so this repo is a good place for them. If we ever add Python utilities (e.g. to simplify setup.py scripts) we may need to move to a new repo rather than polluting rapids-cmake with Python code.

@robertmaynard
Copy link
Contributor

Please add .. versionadded:: v22.06.00 to the documentation in each cython/*.cmake file

@robertmaynard robertmaynard added feature request New feature or request non-breaking Introduces a non-breaking change labels Apr 28, 2022
@robertmaynard
Copy link
Contributor

The function names need to change to match the rapids-cmake style so create_cython_modules -> rapids_cython_create_modules

@vyasr
Copy link
Contributor Author

vyasr commented Apr 29, 2022

In addition to the inline TODOs, I have one additional question. Currently the rapids_cython_create_modules function is set up so that it will be called with modules in the same directory. The implication of that is that every Python subpackage will contain its own CMakeLists.txt. I set it up this way because that's how I've seen CMakeLists.txt files organized in projects in the past. Most RAPIDS projects seem to prefer a single monolithic CMakeLists.txt for all of the C++ code. Should I rework this function to match that pattern instead? That would entail some additional processing of file paths, but would leave us with a single listfile for all of the Python code.

@vyasr vyasr marked this pull request as ready for review April 29, 2022 12:56
rapids-cmake/cython/create_modules.cmake Show resolved Hide resolved
rapids-cmake/cython/create_modules.cmake Show resolved Hide resolved
rapids-cmake/cython/create_modules.cmake Outdated Show resolved Hide resolved
rapids-cmake/cython/create_modules.cmake Outdated Show resolved Hide resolved
rapids-cmake/cython/create_modules.cmake Outdated Show resolved Hide resolved
rapids-cmake/cython/init.cmake Show resolved Hide resolved
rapids-cmake/cython/init.cmake Show resolved Hide resolved
rapids-cmake/cython/init.cmake Outdated Show resolved Hide resolved
rapids-cmake/cython/create_modules.cmake Outdated Show resolved Hide resolved
rapids-cmake/cython/create_modules.cmake Outdated Show resolved Hide resolved
@robertmaynard
Copy link
Contributor

I actually have the change to move add_subdirectory(cython) inside the if(RAPIDS_CMAKE_ENABLE_DOWNLOAD_TESTS) locally, but I had told git to temporarily ignore changes to that file since I have also commented out builds of all other tests locally, so the changes weren't committed... sorry for the confusion there. Names like init are too generic for ctest -R

ctest -L cython will re-run all the tests in the cython subdirectory. If you wanted to run the init cython test you can do ctest -L cython -R init

@vyasr vyasr requested a review from robertmaynard May 4, 2022 21:24
Copy link
Contributor

@robertmaynard robertmaynard left a comment

Choose a reason for hiding this comment

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

A couple of doc changes and we are good to go

cmake-format-rapids-cmake.json Outdated Show resolved Hide resolved
rapids-cmake/cython/create_modules.cmake Outdated Show resolved Hide resolved
@vyasr
Copy link
Contributor Author

vyasr commented May 5, 2022

@robertmaynard I've addressed the two final requests that you had. There are also a couple of TODO items that I had questions about in the tests, create_modules/CMakeLists.txt and create_modules_with_library/CMakeLists.txt. Given that you're leaving for vacation tomorrow, though, I'm also OK with merging this PR (assuming that you're happy) and addressing these questions in a follow-up. Getting this PR merged would make it easier for me to start experimenting with this on other RAPIDS libraries while you're out.

@vyasr vyasr requested a review from robertmaynard May 5, 2022 23:55
@vyasr
Copy link
Contributor Author

vyasr commented May 6, 2022

@gpucibot merge

1 similar comment
@robertmaynard
Copy link
Contributor

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 368a821 into rapidsai:branch-22.06 May 6, 2022
rapids-bot bot pushed a commit to rapidsai/rmm that referenced this pull request May 11, 2022
…1031)

The custom CMake code written in python/cmake has now been generalized and ported to rapids-cmake so that it can be shared across rapids libraries.

This PR depends on rapidsai/rapids-cmake#184 and tests will not pass until that is merged.

Authors:
  - Vyas Ramasubramani (https://github.com/vyasr)

Approvers:
  - AJ Schmidt (https://github.com/ajschmidt8)
  - Robert Maynard (https://github.com/robertmaynard)
  - https://github.com/jakirkham

URL: #1031
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request non-breaking Introduces a non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants