Skip to content
This repository has been archived by the owner on Aug 16, 2023. It is now read-only.

Provide support for RAFT-based indexes #712

Merged
merged 18 commits into from
Mar 13, 2023

Conversation

wphicks
Copy link
Contributor

@wphicks wphicks commented Mar 2, 2023

This is still a work-in-progress. Complete PR description will be added once it is ready for review. Resolve #715.

issue: #715

@sre-ci-robot sre-ci-robot added the do-not-merge/work-in-progress Don't merge even CI passed. label Mar 2, 2023
@sre-ci-robot
Copy link

Welcome @wphicks! It looks like this is your first PR to milvus-io/knowhere 🎉

@mergify mergify bot added needs-dco DCO is missing in this pull request. do-not-merge/missing-related-issue labels Mar 2, 2023
@mergify
Copy link

mergify bot commented Mar 2, 2023

@wphicks Please associate the related issue to the body of your Pull Request. (eg. “issue: #”)

@xiaofan-luan
Copy link
Contributor

@wphicks Welcome !

Copy link

@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.

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.

CMakeLists.txt Outdated Show resolved Hide resolved
src/index/ivf_raft/ivf_raft.cuh Outdated Show resolved Hide resolved
src/index/ivf_raft/ivf_raft.cuh Outdated Show resolved Hide resolved
src/index/ivf_raft/ivf_raft.cuh Outdated Show resolved Hide resolved
src/index/ivf_raft/ivf_raft.cuh Outdated Show resolved Hide resolved
@matrixji
Copy link
Contributor

matrixji commented Mar 3, 2023

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?

@liliu-z
Copy link
Member

liliu-z commented Mar 3, 2023

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.

@wphicks
Copy link
Contributor Author

wphicks commented Mar 6, 2023

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?

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 <whicks@nvidia.com>
Signed-off-by: William Hicks <whicks@nvidia.com>
Signed-off-by: William Hicks <whicks@nvidia.com>
Signed-off-by: William Hicks <whicks@nvidia.com>
Signed-off-by: William Hicks <whicks@nvidia.com>
Signed-off-by: William Hicks <whicks@nvidia.com>
Signed-off-by: William Hicks <whicks@nvidia.com>
Also correct indices passed during extend

Signed-off-by: William Hicks <whicks@nvidia.com>
Signed-off-by: William Hicks <whicks@nvidia.com>
Signed-off-by: William Hicks <whicks@nvidia.com>
Signed-off-by: William Hicks <whicks@nvidia.com>
@mergify mergify bot added the dco-passed DCO check passed. label Mar 6, 2023
@mergify mergify bot removed the needs-dco DCO is missing in this pull request. label Mar 6, 2023
Signed-off-by: William Hicks <whicks@nvidia.com>
Signed-off-by: William Hicks <whicks@nvidia.com>
Signed-off-by: William Hicks <whicks@nvidia.com>
@Presburger
Copy link
Collaborator

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?

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.

raft allows knowhere to rely on fewer libs, faiss lacks modularity in design, so I prefer raft.

Signed-off-by: William Hicks <whicks@nvidia.com>
Signed-off-by: William Hicks <whicks@nvidia.com>
Signed-off-by: William Hicks <whicks@nvidia.com>
@wphicks
Copy link
Contributor Author

wphicks commented Mar 7, 2023

I've just updated with support for a pool allocator and all of RAFT's IVF parameters.

Signed-off-by: William Hicks <whicks@nvidia.com>
@mergify mergify bot added the ci-passed label Mar 9, 2023
@Presburger Presburger marked this pull request as ready for review March 10, 2023 02:55
@sre-ci-robot sre-ci-robot removed the do-not-merge/work-in-progress Don't merge even CI passed. label Mar 10, 2023
@mergify mergify bot added ci-passed and removed ci-passed labels Mar 10, 2023
@sre-ci-robot
Copy link

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@Presburger
Copy link
Collaborator

@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)
Copy link
Contributor

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

@sre-ci-robot sre-ci-robot merged commit 11f606c into milvus-io:main Mar 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide support for RAFT Indexes
7 participants