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

[WIP] Build cuml Cython Using scikit-build #3367

Conversation

mdemoret-nv
Copy link
Contributor

@mdemoret-nv mdemoret-nv commented Jan 13, 2021

This follows the RMM PR, rapidsai/rmm#637, to use scikit-build instead of distutils to compile and install the python cuml library. Using scikit-build has a few advantages:

  • Uses cmake to specify the build sources and parameters
    • Allows us to reuse much of the configuration created for libcuml++ and libcuml
  • Eliminates the need to maintain two configuration systems (especially helpful for things like finding CUDA)
  • Allows more flexibility in the build (for example, compiling the python code with -O1 is a huge pain with distutils)
  • Dependencies between libraries are properly maintained
    • For example, changes to a C++ file will be recompiled even when building the python library
  • Outputs a single compile_commands.json (Great for IDEs)
  • Potentially faster builds (TBD. ~25% increase currently)

There are several things that need to be completed before merging this PR:

  • Determine unified build folder layout (more on this below)
  • Remove #include dependencies on cpp/src folder (these will break dependent targets if they are not installed) (See issue [BUG] Clean Up #include Dependencies #3376)
  • Update cuml CMakeLists.txt to modern standards (needed for new python dependent target) (See issue [FEA] Modernize CMake Build Structure #3375)
    • Remove all CMAKE_CXX_FLAGS and CMAKE_CUDA_FLAGS
    • Switch to target based architecture and export necessary includes and linked libraries
    • Use test configuration in cmake (Optional)
  • Update RAPIDS dependencies to properly export targets and config
    • RMM exports targets. (See also PR: Build RMM Cython using scikit-build rmm#637.) PR has been tested and works well when importing
    • RAFT
      • Determine whether to change RAFT to a conda package or determine alternate way to include cython files
    • CumlPrims_MG
    • UCX?
  • Update python library build system to use scikit-build
    • Import libcuml++ as sub_directory
    • Update setup.py to use scikit-build and remove extra config code
    • Add CMakeLists.txt to all folders with Cython sources
    • Determine fix for name collision on python packages
  • Update ./build.sh

Note: Since both the C++ and Python folders will be using cmake, both output binary (build) folders must reside in the same directory in order to use add_subdirectory(). This is necessary to allow building just the C++ binaries or both the C++ and Python binaries together. Currently the build folders are duplicated and the organization is show below:

cuml/
├── cpp/
│   ├── build/ (current CMake binary folder for C++ libraries)
│   ├── CMakeLists.txt
│   ├── include/
│   ├── src/
│   └── src_prims/
└── python/
    ├── _skbuild/ (new CMake binary folder for both C++ and Python)
    │   ├── cuml/
    │   └── cuml-cpp/ (duplicate of cpp/build)
    ├── CMakeLists.txt
    ├── cuml/
    └── setup.py

One possibility is to make a CMakeLists.txt at the root directory and move the cpp/build folder to the root as well:

cuml/
├── build/
│   ├── cuml/ (Python binary files)
│   └── libcuml/ (C++ binary files)
├── CMakeLists.txt (adds subdirectories for cpp/CMakeLists.txt and python/CMakeLists.txt)
├── cpp/
│   ├── CMakeLists.txt
│   ├── include/
│   ├── src/
│   └── src_prims/
└── python/
    ├── CMakeLists.txt
    ├── cuml/
    └── setup.py

This is still very much a WIP (which should be obvious from all the commented out code)

@mdemoret-nv mdemoret-nv requested review from a team as code owners January 13, 2021 01:18
@github-actions github-actions bot added CMake Cython / Python Cython or Python issue libcuml labels Jan 13, 2021
@mdemoret-nv mdemoret-nv added 2 - In Progress Currenty a work in progress Build or Dep Issues related to building the code or dependencies feature request New feature or request non-breaking Non-breaking change Ops and removed Cython / Python Cython or Python issue libcuml labels Jan 13, 2021
@dantegd
Copy link
Member

dantegd commented Jan 13, 2021

@mdemoret-nv I think the development and review process of the PR is going to be significantly smoother if at least these items:

- [ ] Remove `#include` dependencies on `cpp/src` folder (these will break dependent targets if they are not installed)
- [ ] Update `cuml` CMakeLists.txt to modern standards (needed for new python dependent target)
   - [ ] Remove all `CMAKE_CXX_FLAGS` and `CMAKE_CUDA_FLAGS`
   - [x] Switch to target based architecture and export necessary includes and linked libraries
   - [ ] Use test configuration in cmake (Optional)

go into their own PRs, since they are independent and don't need to be blocked on items that overall block scikit-build, what do you think?

@dantegd
Copy link
Member

dantegd commented Jan 13, 2021

Regarding the item:

 Determine unified build folder layout (more on this below)

This is something that should have a unified approach across RAPIDS repos, so tagging @kkraus14 for visibility and input (and he can tag appropriate folks for discussing if needed)

@mdemoret-nv
Copy link
Contributor Author

mdemoret-nv commented Jan 14, 2021

@dantegd Agreed. Will make 2 PRs for the items you have mentioned and note then in this PR.

Edit: Added issues #3375 and #3376

@mdemoret-nv mdemoret-nv marked this pull request as draft January 14, 2021 23:06
@github-actions github-actions bot added Cython / Python Cython or Python issue libcuml and removed Ops labels Jan 14, 2021
@github-actions
Copy link

This PR has been marked stale due to no recent activity in the past 30d. Please close this PR if it is no longer required. Otherwise, please respond with a comment indicating any updates. This PR will be marked rotten if there is no activity in the next 60d.

@mdemoret-nv
Copy link
Contributor Author

Development is currently blocked by the bullet points listed above and linked issues.

@github-actions
Copy link

This PR has been labeled inactive-30d due to no recent activity in the past 30 days. Please close this PR if it is no longer required. Otherwise, please respond with a comment indicating any updates. This PR will be labeled inactive-90d if there is no activity in the next 60 days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 - In Progress Currenty a work in progress Build or Dep Issues related to building the code or dependencies CMake Cython / Python Cython or Python issue feature request New feature or request inactive-30d libcuml non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants