Removing FAISS from build #1340
Conversation
Replace faiss bfKnn with code that leverages our pairwise_distance api and select_k api - by computing tiling over the inputs. This lets us remove faiss as a dependency
This reverts commit 8eaba84. Change didn't seem to build in CI
Also remove metrics processors - since is handled inside PW distance
This reverts commit fe728e9. This is causing incorrect results, just use the faiss select_k call instead
|
@bdice I'm a little concerned doing that large of a change at the very end of 23.04, which is why we're targeting this specifically for early 23.06. What might not be obvious from the |
Gotcha. Sounds good, thanks for the context! |
There was a problem hiding this comment.
Looks like faiss still needs to be removed from the conda recipes (libcudf/meta.yaml and conda_build_config.yaml). Also, I still see faiss_select files and namespaces (coming from selection_faiss.cuh) in a number of places, do those need to be removed as well?
@vyasr done. And no, the |
|
Thinking about this more, we would like to have RAFT integrated into FAISS by early 23.06, which means FAISS will likely want to be using 23.04, at least to start. Given the transition has been smoother than I had originally thought this far, and since it's still a couple weeks from burndown, I'm going to go ahead and merge this over now. If we encounter any problems leading not burndown, im going to revert the change and push to 23.06. Cc @bdice @jakirkham @vyasr |
|
/merge |
…#1333) Now that we no longer need to rely on the FAISS dependency, this PR: 1. Consolidates the `libraft-distance` and `libraft-nn` targets into a single `libraft` artifact 2. Introduces a new `raft::compiled` target to link against the new `libraft.so` binary. Removes `raft::distance`and `raft::nn` targets. 3. Consolidates `RAFT_DISTANCE_COMPILED` and `RAFT_NN_COMPILED` pre-processor vars into a single `RAFT_COMPILED` (to match similar pattern implementated by `spdlog`) 4. Consolidates `specializations.cuh` headers 5. Updates all docs, scripts, and build infra This change has been a long time coming and is intended to be a 23.04 feature. This is further going to require updates to several projects downstream. Here's a checklist to track that progress: - [x] offline announcement - [x] cuml rapidsai/cuml#5272 - [x] cugraph rapidsai/cugraph#3348 - [x] cuopt rapidsai/cuopt#1023 - [x] cugraph-ops rapidsai/cugraph-ops#429 This PR depended on #1340 (removing FAISS from the build) and on #1202 (replacing the FAISS bfknn w/ our own), both of which have been merged. Closes #824 Authors: - Corey J. Nolet (https://github.com/cjnolet) - Ben Frederickson (https://github.com/benfred) Approvers: - Sevag H (https://github.com/sevagh) - Divye Gala (https://github.com/divyegala) - Ben Frederickson (https://github.com/benfred) - Vyas Ramasubramani (https://github.com/vyasr) URL: #1333
This depends on #1202 so those changes are also included here. This should also not be merged until that PR is merged.