Migrate from RAFT to CUVS #3549
Conversation
|
Hi @tarang-jain! Thanks for the PR. I quickly skimmed through the PR and noticed there are still a lot of references to RAFT. Is the plan to completely remove all references to RAFT and replace with cuVS? For example, try |
|
@asadoughi thanks for your comment. I have updated the PR description! |
There was a problem hiding this comment.
Hi @tarang-jain - thanks for the changes. I provided some comments inline.
| @@ -110,7 +110,7 @@ Several options can be passed to CMake, among which: | |||
| values are `ON` and `OFF`), | |||
| - `-DFAISS_ENABLE_PYTHON=OFF` in order to disable building python bindings | |||
| (possible values are `ON` and `OFF`), | |||
| - `-DFAISS_ENABLE_RAFT=ON` in order to enable building the RAFT implementations | |||
There was a problem hiding this comment.
What about lines 9, 20-21, 27, 38-39? Will we have a faiss-gpu-cuvs package?
There was a problem hiding this comment.
I intentionally did not make changes to the docs (readme and install) because I thought that is something entirely under FAISS' governance. Don't want to modify that without approval from you guys. But to answer your question: yes it will be called faiss-gpu-cuvs.
There was a problem hiding this comment.
Those lines reference packages distributed by the nvidia channel so if you have faiss-gpu-cuvs prepared we can coordinate the release.
| @@ -41,13 +41,13 @@ def aa(*args, **kwargs): | |||
|
|
|||
|
|
|||
| group = parser.add_argument_group('benchmarking options') | |||
| aa('--raft_only', default=False, action='store_true', | |||
There was a problem hiding this comment.
Have you run this before and after the change to determine the change (if any) in performance?
| @@ -43,8 +43,8 @@ def aa(*args, **kwargs): | |||
| help='whether to benchmark add operation on GPU index') | |||
| aa('--bm_search', default=True, | |||
| help='whether to benchmark search operation on GPU index') | |||
| aa('--raft_only', default=False, action='store_true', | |||
There was a problem hiding this comment.
Have you run this before and after the change to determine the change (if any) in performance?
tarang-jain
force-pushed
the
cuvs-migrate
branch
from
eee2662
to
4fdea22
Compare
|
Hi @tarang-jain - it looks like the test fails on https://github.com/facebookresearch/faiss/actions/runs/10587022390/job/29337315242#step:3:2446 related to a core dump. Are you able to debug the issue? |
|
@asadoughi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
Regarding the segfault, consider rebasing off of the latest main to see if #3806 has an effect. |
|
@asadoughi I was able to resolve the seg fault. It was related to some bugs in the cuvs parts of bfKnn. |
Remove the dependency on
raft::compiledand modify GPU implementations to use cuVS backend in place of RAFT.A deeper insight into the dependency:
FAISS gets the ANN algorithm implementations such as IVF-Flat and IVF-PQ from cuVS. RAFT is meant to be a lightweight C++ header-only template library that cuVS relies on for the more fundamental / low-level utilities. Some examples of these are RAFT's device mdarray and mdspan objects; the RAFT resource object (
raft::resource) that takes care of the stream ordering of device functions; linear algebra functions such as mapping, reduction, BLAS routines etc. A lot of the cuVS functions take the RAFT mdspan objects as arguments (for exampleraft::device_matrix_view). Therefore FAISS relies on both cuVS and RAFT. FAISS gets RAFT headers through cuVS and uses them to create the function arguments that can be consumed by cuVS. Note that we are not explicitly linking FAISS againstraft::raftorraft::compiled. Only the required headers are included and compiled rather than compiling the whole RAFT shared library. This is the reason we still see mentions ofraftin FAISS.