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

Updating README #1836

Closed
wants to merge 4 commits into from
Closed

Conversation

nwstephens
Copy link
Contributor

The installation instructions in the readme contains a bug. The Conda instructions should include a reference to the CUDA version. Also, the build from source instructions were out of sync with the docs, so I'm pointing the reader to the docs instead of including all of the instructions in the readme.

Updating installing instructions, bringing it into parity with the docs. Including reference to CUDA version with Conda. Pointing readers to the docs for building and installing from source.
Updated header for building from source.
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@cjnolet cjnolet added the non-breaking Non-breaking change label Sep 25, 2023
Fixing typos

Co-authored-by: Bradley Dice <[email protected]>
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.

I think overall, I'd like to not remove the C++ usage instructions nor give the impression that RAFT is solely a Python library. I think we should mention that we have benchmarking packages and docker containers and then link out to the raft_ann_benchmarks.md file for more information about installing and using the docker containers.

RAFT is a C++ library w/ Python bindings. Even if we expose a C API or Java API, RAFT is still going to be a C++ library at its core, just like the rest of the C++ libraries in RAPIDS. I think users should expect to see a very quick overview of the ways to use RAFT on the front page and then follow a link to more comprehensive guide. We just need to make sure it's up to date and you've outlined here a couple places where they weren't updated.

```

You can also install the conda packages individually using the `mamba` command above.

After installing RAFT, `find_package(raft COMPONENTS compiled distributed)` can be used in your CUDA/C++ cmake build to compile and/or link against needed dependencies in your raft target. `COMPONENTS` are optional and will depend on the packages installed.
Copy link
Member

Choose a reason for hiding this comment

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

This one is correct. nn and distance don't exist anymore.

```

You can also install the conda packages individually using the `mamba` command above.

After installing RAFT, `find_package(raft COMPONENTS compiled distributed)` can be used in your CUDA/C++ cmake build to compile and/or link against needed dependencies in your raft target. `COMPONENTS` are optional and will depend on the packages installed.
After installing RAFT, `find_package(raft COMPONENTS nn distance)` can be used in your CUDA/C++ cmake build to compile and/or link against needed dependencies in your raft target. `COMPONENTS` are optional and will depend on the packages installed.
Copy link
Member

Choose a reason for hiding this comment

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

This components no longer exist. If they are still in the build instructions elsewhere then that needs to be updated.


#### Example Template Project

You can find an [example RAFT](cpp/template/README.md) project template in the `cpp/template` directory, which demonstrates how to build a new application with RAFT or incorporate RAFT into an existing cmake project.
Copy link
Member

Choose a reason for hiding this comment

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

I would not remove the C++ installation instructions from the front page. There's a slight difference between the use-cases of using raft through cmake and building raft from source. We include both in the installation and build guide but we only include the former in the readme. This is pretty standard practive. As RAFT is a C++ library at its core, and many users use it through C++, I'd prefer not to remove the quick and dirty samples from the front page.

Perhaps we could adjust the verbiage a tad, though. Many cmake users these days are going to be familiar w/ the cmake package manager (CPM), however we could make the verbiage more friendly by maybe renaming the section title to "Using RAFT through Cmake".

- `libraft-headers` RAFT headers
- `libraft` (optional) shared library of pre-compiled template instantiations and runtime APIs.
- `libraft-headers` RAFT headers.
- `libraft` (optional) shared library containing pre-compiled template instantiations and runtime APIs.
- `pylibraft` (optional) Python wrappers around RAFT algorithms and primitives.
- `raft-dask` (optional) enables deployment of multi-node multi-GPU algorithms that use RAFT `raft::comms` in Dask clusters.
Copy link
Member

Choose a reason for hiding this comment

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

We probably want to add raft-ann-bench and raft-ann-bench-cpu here as well, since those are conda/python packages that ship w/ raft.

Additional CMake targets can be made available by adding components in the table below to the `RAFT_COMPONENTS` list above, separated by spaces. The `raft::raft` target will always be available. RAFT headers require, at a minimum, the CUDA toolkit libraries and RMM dependencies.

| Component | Target | Description | Base Dependencies |
|-------------|---------------------|----------------------------------------------------------|----------------------------------------|
Copy link
Member

Choose a reason for hiding this comment

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

For the reasons outlined above, I'd prefer not to remove this from the docs. For cmake users, these are very important.

I also think should keep the Source section below, and continue pointing the user to the build and install guide for more comprehensive instructions.

@cjnolet cjnolet changed the base branch from branch-23.10 to branch-23.12 October 5, 2023 23:24
@nwstephens
Copy link
Contributor Author

Duplicate. Closing.

@nwstephens nwstephens closed this Oct 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Documentation non-breaking Non-breaking change Vector Search
Projects
Development

Successfully merging this pull request may close these issues.

3 participants