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

Build pylibraft with scikit-build #633

Merged
merged 39 commits into from
May 13, 2022
Merged

Build pylibraft with scikit-build #633

merged 39 commits into from
May 13, 2022

Conversation

vyasr
Copy link
Contributor

@vyasr vyasr commented Apr 27, 2022 •
edited
Loading

This PR changes the build system for pylibraft to use scikit-build and CMake, allowing it to standardize more logic across all of RAPIDS and leverage the benefits of CMake to simplify building the C++ libraries along with the Python packages.

vyasr added 16 commits May 4, 2022 16:04
@vyasr
Add _skbuild to gitignore.
6b5e80d
@vyasr
Initial version with skbuild.
f38cfbd
@vyasr
Rework add_cython_modules to support passing in the list of libraries.
6287c1b
@vyasr
Rework add_cython_modules to support passing in the base directory fo…
a0c5d54 
…r installation.
@vyasr
Switch to using new rapids-cmake function for creating cython modules.
f0dcb67
@vyasr
Remove skbuild patch and rely on rapids-cmake.
7b04525
@vyasr
Standardize some more boilerplate and add TODOs.
a05ef51
@vyasr
Update build to use newest rapids-cython code.
12fe3ce
@vyasr
Rename project to pylibraft.
3b5dc46
@vyasr
Rename cython_modules to cython_sources.
a1ec517
@vyasr
Remove outdated TODO.
6dc4c8f
@vyasr
Add pyproject.toml and remove superfluous components of setup.py.
65a2d04
@vyasr
Update conda files.
381df61
@vyasr
Update update_version.sh and the name of the version variable in CMak…
87f44f4 
…eLists.txt.
@vyasr
Improve formatting of build.sh.
1e8dc7e
@vyasr
Update relevant parts of build.sh.
d805dd4
@vyasr vyasr force-pushed the feature/skbuild branch from 1ac6c2f to d805dd4 Compare May 4, 2022 23:39
@github-actions github-actions bot added the gpuCI label May 4, 2022
@vyasr vyasr marked this pull request as ready for review May 6, 2022 00:51
@vyasr vyasr requested review from a team as code owners May 6, 2022 00:51
@github-actions github-actions bot removed the cpp label May 6, 2022
@vyasr
Copy link
Contributor Author

vyasr commented May 6, 2022 •
edited
Loading

For some reason nvcc is selecting a much older host compiler (c++ 4.8.2) even though CMake correctly finds g++ 9.4 when it is configuring. I'm not sure why this would be the case. The pylibraft builds shouldn't need to rebuild libraft anyway, it should be finding the already present raft libraries, so I will update the scripts to handle doing that correctly, but I'm still a little concerned by the C++ compiler of choice here.

@vyasr
Merge remote-tracking branch 'origin/branch-22.06' into feature/skbuild
549c765 
# Conflicts:
#	build.sh
#	conda/recipes/pylibraft/build.sh
@vyasr vyasr mentioned this pull request May 11, 2022
Merged
cjnolet
cjnolet requested changes May 11, 2022
View reviewed changes
Copy link
Member

@cjnolet cjnolet left a comment

Choose a reason for hiding this comment

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

Sorry, Vyas, I had this review done and realized I never pressed submit

cpp/CMakeLists.txt Outdated Show resolved Hide resolved
cpp/CMakeLists.txt Outdated Show resolved Hide resolved
build.sh Show resolved Hide resolved
@vyasr
Copy link
Contributor Author

vyasr commented May 12, 2022

This PR should not be merged until we resolve rapidsai/rapids-cmake#189. For the case where we're building the Python library without a preexisting build of the C++ lib, we need to come up with a standard scheme for how we package up the C++ library and link to it.

@vyasr vyasr force-pushed the feature/skbuild branch from 7ff68e4 to 2729be0 Compare May 12, 2022 02:51
@vyasr
Copy link
Contributor Author

vyasr commented May 12, 2022 •
edited
Loading

Sorry, Vyas, I had this review done and realized I never pressed submit

No worries, thanks for the review. We're nearly there, but we have a couple of blockers to resolve (the rapids-cmake issue above and maybe a better way to handle the case where we're linking to a libraft without distances compiled in).

@vyasr vyasr requested a review from cjnolet May 13, 2022 00:12
@vyasr
Copy link
Contributor Author

vyasr commented May 13, 2022

This PR depends on rapidsai/rapids-cmake#189, and if someone on the raft team is used to building raft on bare metal I'd appreciate a tester of building both with and without a preexisting C++ library just to be sure that everything is behaving as expected. I am running into one issue locally, but I'm having trouble isolating whether it's because I'm using rapids-compose or not.

@vyasr
Copy link
Contributor Author

vyasr commented May 13, 2022

I confirmed that the above issue is not related to rapids-compose. I can reproduce it on bare metal, so I'll need to track this down.

@vyasr
Copy link
Contributor Author

vyasr commented May 13, 2022

OK I think this PR is all set now. There were errors in the configuration of the CUDA architectures when the C++ library build was triggered by the Python build, now that has been fixed. Tests won't pass here until we merge rapidsai/rapids-cmake#189, but then this PR should be good to go.

cjnolet
cjnolet requested changes May 13, 2022
View reviewed changes
Copy link
Member

@cjnolet cjnolet left a comment

Choose a reason for hiding this comment

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

Just one very minor thing in the top-level CMakeLists, otherwise I think it looks great. Thanks again for going this conversion for RAFT!

cpp/CMakeLists.txt Show resolved Hide resolved
@vyasr
Copy link
Contributor Author

vyasr commented May 13, 2022

rerun tests

vyasr
vyasr commented May 13, 2022
View reviewed changes
cpp/CMakeLists.txt Outdated Show resolved Hide resolved
@vyasr vyasr requested a review from cjnolet May 13, 2022 21:55
cjnolet
cjnolet approved these changes May 13, 2022
View reviewed changes
Copy link
Member

@cjnolet cjnolet left a comment

Choose a reason for hiding this comment

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

LGTM!

@cjnolet
Copy link
Member

cjnolet commented May 13, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 9ed013f into rapidsai:branch-22.06 May 13, 2022
rapids-bot bot pushed a commit that referenced this pull request May 17, 2022
@vyasr
Build pyraft with scikit-build (#644)
fd0b447 
This PR changes the pyraft build system to use scikit-build and CMake. It depends on the changes in #633 and should be merged after that PR.

This PR is largely complete, except that I am not yet sure exactly which libraries/headers are entirely necessary for the build. This version builds locally for me, but not all tests pass since I don't have nccl. However, I am a bit surprised to see the build succeeding without me even searching for UCX (perhaps this is because it's already on my system include path) so I need to figure out what do with UCX, NCCL, and the various CUDA math libraries (cusparse, cublas, etc).

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

Approvers:
  - Sevag Hanssian (https://github.com/sevagh)
  - Ray Douglass (https://github.com/raydouglass)
  - Corey J. Nolet (https://github.com/cjnolet)

URL: #644
@vyasr vyasr mentioned this pull request May 19, 2022
Merged
rapids-bot bot pushed a commit that referenced this pull request May 20, 2022
@vyasr
Fix minor errors in CMake configuration (#662)
6e8e41d 
This PR fixes a number of small errors that slipped through in #644 and #633. None of them materially affect the build (whether a preexisting libraft exists or not) since 1) benchmarks are currently turned off by default anyway in the C++ build, and 2) pyraft only depends on libraft for headers, not for compiled libraries.

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

Approvers:
  - Corey J. Nolet (https://github.com/cjnolet)

URL: #662
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ajschmidt8 ajschmidt8 ajschmidt8 approved these changes

@cjnolet cjnolet cjnolet approved these changes

Assignees

@vyasr vyasr

Labels
3 - Ready for Review CMake cpp gpuCI improvement Improvement / enhancement to an existing function non-breaking Non-breaking change python
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@vyasr @cjnolet @ajschmidt8 @trxcllnt