-
Notifications
You must be signed in to change notification settings - Fork 84
Provide support for RAFT-based indexes #712
Conversation
Welcome @wphicks! It looks like this is your first PR to milvus-io/knowhere 🎉 |
@wphicks Please associate the related issue to the body of your Pull Request. (eg. “issue: #”) |
@wphicks Welcome ! |
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.
Integration looks pretty clean so far! Leaving some feedback as we will probably want to allow sweeping through most, of not all, of the available parameters to get accurate benchmarks.
I see raft support for faiss, right now Knowhere reuses some faiss's code for some GPU Index. Does it possible to reuse the faiss's impl, if Faiss would support RAFT? |
We will definitely do that. Since it takes time for Faiss' integration to be done, we can first support RAFT to fillfull users' needs. |
Yes, as @liliu-z mentioned, integrating RAFT is mostly to make it available within Milvus sooner. It may also be an integration path that will be useful to support longer term, since ANN improvements are a very active area of research and development in RAFT. New innovations there may take a bit of time to be supported through FAISS, so having RAFT support will allow Milvus to more quickly take advantage of the latest and greatest we can come up with. |
Signed-off-by: William Hicks <[email protected]>
Signed-off-by: William Hicks <[email protected]>
Signed-off-by: William Hicks <[email protected]>
Signed-off-by: William Hicks <[email protected]>
Signed-off-by: William Hicks <[email protected]>
Signed-off-by: William Hicks <[email protected]>
Signed-off-by: William Hicks <[email protected]>
Also correct indices passed during extend Signed-off-by: William Hicks <[email protected]>
Signed-off-by: William Hicks <[email protected]>
Signed-off-by: William Hicks <[email protected]>
Signed-off-by: William Hicks <[email protected]>
3527014
to
247f2b1
Compare
Signed-off-by: William Hicks <[email protected]>
Signed-off-by: William Hicks <[email protected]>
Signed-off-by: William Hicks <[email protected]>
raft allows knowhere to rely on fewer libs, faiss lacks modularity in design, so I prefer raft. |
Signed-off-by: William Hicks <[email protected]>
Signed-off-by: William Hicks <[email protected]>
Signed-off-by: William Hicks <[email protected]>
I've just updated with support for a pool allocator and all of RAFT's IVF parameters. |
Signed-off-by: William Hicks <[email protected]>
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Presburger, wphicks The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@wphicks hi, if you can merge the commit into single one and signed the commits, then we will merge the pr. |
@@ -97,6 +121,11 @@ if(NOT USE_CUDA) | |||
list(REMOVE_ITEM KNOWHERE_SRCS ${KNOWHERE_GPU_SRCS}) | |||
endif() | |||
|
|||
if(NOT WITH_RAFT) | |||
knowhere_file_glob(GLOB_RECURSE KNOWHERE_RAFT_SRCS src/index/index_raft/*.cc src/index/index_raft/*.cu) |
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.
seems here index_raft
should be ivf_raft
This is still a work-in-progress. Complete PR description will be added once it is ready for review. Resolve #715.
issue: #715