-
Notifications
You must be signed in to change notification settings - Fork 197
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
Refactor install/build guide. #1899
Refactor install/build guide. #1899
Conversation
/ok to test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are helpful updates! Thanks. I left a few high-level comments but please treat my feedback as non-blocking since I may not be able to re-review in a timely fashion.
docs/source/build.md
Outdated
All of RAFT's C++ APIs can be used header-only but pre-compiled shared libraries also contain some host-accessible APIs and template instantiations to accelerate compile times. | ||
#### Conda environment scripts | ||
|
||
Conda environment scripts are provided for installing the necessary dependencies to build both the C++ and Python libraries from source. It is preferred to use `mamba`, as it provides significant speedup over `conda`. In addition you will have to manually install a CUDA toolkit which is greater than, or equal to, the version you install into you conda environment. Installing CUDA toolkit into your host system is necessary because `nvcc` is not provided with Conda's cudatoolkit dependencies. The following example will install create and install dependencies for a CUDA 11.8 conda environment: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This situation changes pretty dramatically (for the better) with CUDA 12. Should we figure out a way to tell the "simple" CUDA 12 story first, which has no system CTK requirement, and then describe the CUDA 11 case after that? (Eventually the CUDA 11 info will be safe to remove, so I'd try to separate it now.)
Co-authored-by: Bradley Dice <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving with a couple minor comments. The changes are a big improvement!
pip install pylibraft-cu11 --extra-index-url=https://pypi.nvidia.com | ||
pip install raft-dask-cu11 --extra-index-url=https://pypi.nvidia.com |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe combine?
pip install pylibraft-cu11 --extra-index-url=https://pypi.nvidia.com | |
pip install raft-dask-cu11 --extra-index-url=https://pypi.nvidia.com | |
pip install pylibraft-cu11 raft-dask-cu11 --extra-index-url=https://pypi.nvidia.com |
docs/source/build.md
Outdated
There are a few different strategies for including RAFT in downstream projects, depending on whether the [required build dependencies](#build-dependencies) have already been installed and are available on the `lib` and `include` paths. | ||
|
||
Using cmake, you can enable CUDA support right in your project's declaration: | ||
When using the GPU parts of RAFT, you will need to enable CUDA support in your Cmake project declaration: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't mark all these as noted in my previous review -- maybe use find/replace "Cmake" to "CMake."
When using the GPU parts of RAFT, you will need to enable CUDA support in your Cmake project declaration: | |
When using the GPU parts of RAFT, you will need to enable CUDA support in your CMake project declaration: |
/merge |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changes look great!
/merge |
This PR makes some mcuh needed changes to the installation and builde guide. This PR also fixes a few APIs that were missing from the API docs. Closes rapidsai#1895 Authors: - Corey J. Nolet (https://github.com/cjnolet) Approvers: - Bradley Dice (https://github.com/bdice) - Dante Gama Dessavre (https://github.com/dantegd) URL: rapidsai#1899
This PR makes some mcuh needed changes to the installation and builde guide. This PR also fixes a few APIs that were missing from the API docs. Closes rapidsai#1895 Authors: - Corey J. Nolet (https://github.com/cjnolet) Approvers: - Bradley Dice (https://github.com/bdice) - Dante Gama Dessavre (https://github.com/dantegd) URL: rapidsai#1899
This PR makes some mcuh needed changes to the installation and builde guide. This PR also fixes a few APIs that were missing from the API docs.
Closes #1895