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

Adding FAISS cpu to raft-ann-bench #1814

Merged
merged 44 commits into from
Oct 10, 2023

Conversation

cjnolet
Copy link
Member

@cjnolet cjnolet commented Sep 11, 2023

Closes #1813

This PR adds FAISS CPU indexes to the raft-Ann-bench benchmarks, and adjusts the build accordingly. In addition, this PR also updated the FAISS version to include the RAFT-enabled version, which required removing the FAISS conda packages and building FAISS from source.

@cjnolet cjnolet added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Sep 11, 2023
@cjnolet cjnolet self-assigned this Sep 11, 2023
@cjnolet cjnolet requested review from a team as code owners September 11, 2023 20:57
@cjnolet cjnolet marked this pull request as draft September 11, 2023 21:08
@cjnolet
Copy link
Member Author

cjnolet commented Sep 11, 2023

The big challenge with this change is that faiss already uses the faiss::faiss target and transitively passes through the dependency on the libfaiss-gpu target. It's not clear to me the best approach to pass through the target only sometimes, such as when building raft-ann-bench package but not when building the raft-ann-bench-cpu package.

I suppose one way to accomplish this would be to accept an option in get_faiss.cmake to set FAISS_ENABLE_GPU to OFF. Tagging @robertmaynard for any thoughts.

@achirkin
Copy link
Contributor

Ha-ha, I had some drafts on this too, but you beat me to it :) I struggled with the FAISS CPU/GPU builds as well and came up with this solution #1820 . It's not perfect and needs some refinement, but it works. Any thoughts on this?

@cjnolet cjnolet marked this pull request as ready for review September 13, 2023 22:06
@cjnolet cjnolet requested review from a team as code owners September 13, 2023 22:06
include(cmake/modules/FindAVX.cmake)

# Link against AVX CPU lib if it exists
set(RAFT_FAISS_GLOBAL_TARGETS faiss::faiss)
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need these conditionals around GLOBAL_TARGETS just provide them all and rapids-cmake will first check that they exist before doing any operations on them

@cjnolet
Copy link
Member Author

cjnolet commented Oct 4, 2023

@robertmaynard looks like this worked! Thanks so much. Honestly, I had no clue that the FAISS side of the cmake didn't include the same search paths as the raft side of the cmake. I just assumed that part was transitively. So happy to see this building in CI finally.

Copy link
Member

@divyegala divyegala left a comment

Choose a reason for hiding this comment

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

Why was it needed to get fmt and spdlog for CPU only builds? I know currently RMM brings those, but are we providing them for FAISS?

conda/recipes/raft-ann-bench-cpu/meta.yaml Outdated Show resolved Hide resolved
conda/recipes/raft-ann-bench/meta.yaml Outdated Show resolved Hide resolved
cpp/bench/ann/CMakeLists.txt Show resolved Hide resolved
cpp/bench/ann/src/faiss/faiss_cpu_benchmark.cpp Outdated Show resolved Hide resolved
cpp/bench/ann/src/faiss/faiss_cpu_benchmark.cpp Outdated Show resolved Hide resolved
@cjnolet cjnolet changed the base branch from branch-23.10 to branch-23.12 October 5, 2023 18:44
conda/environments/bench_ann_cuda-118_arch-x86_64.yaml Outdated Show resolved Hide resolved
@@ -107,7 +107,7 @@ class FaissGpu : public ANN<T> {

void build(const T* dataset, size_t nrow, cudaStream_t stream = 0) final;

void set_search_param(const AnnSearchParam& param) override;
virtual void set_search_param(const FaissGpu<T>::AnnSearchParam& param) {}
Copy link
Member

Choose a reason for hiding this comment

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

I'm not too sure about this, should this be?

Suggested change
virtual void set_search_param(const FaissGpu<T>::AnnSearchParam& param) {}
virtual void set_search_param(const FaissGpu<T>::AnnSearchParam& param) = 0;

- benchmark>=1.8.2
- faiss-proc=*=cuda
- rmm=23.10.*
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- rmm=23.10.*
- rmm=23.12.*

@cjnolet
Copy link
Member Author

cjnolet commented Oct 10, 2023

/merge

1 similar comment
@cjnolet
Copy link
Member Author

cjnolet commented Oct 10, 2023

/merge

@rapids-bot rapids-bot bot merged commit 99ed345 into rapidsai:branch-23.12 Oct 10, 2023
59 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CMake cpp improvement Improvement / enhancement to an existing function non-breaking Non-breaking change python
Projects
Development

Successfully merging this pull request may close these issues.

[FEA] Add FAISS cpu indexes to raft-ann-bench
7 participants