-
Notifications
You must be signed in to change notification settings - Fork 143
Merge support for tiered index #1155
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
Changes from all commits
f36540b
2a2fab5
bf53e81
3223009
4eb0dd5
53c293b
bfaae1d
bf49565
86d9243
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -246,6 +246,30 @@ cuvsError_t cuvsTieredIndexSearch(cuvsResources_t res, | |
| cuvsError_t cuvsTieredIndexExtend(cuvsResources_t res, | ||
| DLManagedTensor* new_vectors, | ||
| cuvsTieredIndex_t index); | ||
| /** | ||
| * @} | ||
| */ | ||
|
|
||
| /** | ||
| * @defgroup tiered_c_index_merge Tiered index merge | ||
| * @{ | ||
| */ | ||
| /** | ||
| * @brief Merge multiple indices together into a single index | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we flesh out this description a little more to provide more details for the user on exaclty how this does the merge? We should do the same in the c++ description also. Maybe add details about what happens to the vectors in tier 0, what happens to the indexes in tier 1, what are some of the memory considerations (I know there's copies- what are some of the assumptions a user will need to make?) |
||
| * | ||
| * @param[in] res cuvsResources_t opaque C handle | ||
| * @param[in] index_params Index parameters to use when merging | ||
| * @param[in] indices pointers to indices to merge together | ||
| * @param[in] num_indices the number of indices to merge | ||
| * @param[out] output_index the merged index | ||
| * @return cuvsError_t | ||
| */ | ||
| cuvsError_t cuvsTieredIndexMerge(cuvsResources_t res, | ||
| cuvsTieredIndexParams_t index_params, | ||
| cuvsTieredIndex_t* indices, | ||
| size_t num_indices, | ||
| cuvsTieredIndex_t output_index); | ||
|
|
||
| /** | ||
| * @} | ||
| */ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -297,6 +297,79 @@ auto build( | |
| return std::shared_ptr<index_state<UpstreamT>>(ret); | ||
| } | ||
|
|
||
| /** | ||
| * @brief merge multiple indices together | ||
| */ | ||
| template <typename UpstreamT> | ||
| auto merge(raft::resources const& res, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder- if we are going to rebuild the cagra index anyways to consolidate tiered indices, should we just go ahead and add all the vectors in tier0 to that cagra index too (e.g. should we also do a compaction?). Disregard if this is already what we're doing. |
||
| const index_params<typename UpstreamT::index_params_type>& index_params, | ||
| const std::vector<tiered_index::index<UpstreamT>*>& indices) | ||
| -> std::shared_ptr<index_state<UpstreamT>> | ||
| { | ||
| using value_type = typename UpstreamT::value_type; | ||
|
|
||
| RAFT_EXPECTS(indices.size() > 0, "Must pass at least one index to merge"); | ||
|
|
||
| for (auto index : indices) { | ||
| RAFT_EXPECTS(index != nullptr, | ||
| "Null pointer detected in 'indices'. Ensure all elements are valid before usage."); | ||
| } | ||
|
|
||
| // handle simple case of only one index being merged | ||
| if (indices.size() == 1) { return indices[0]->state; } | ||
|
|
||
| auto dim = indices[0]->state->dim(); | ||
| auto include_norms = indices[0]->state->storage->include_norms; | ||
|
|
||
| // validate data and check what needs to be merged | ||
| size_t bfknn_rows = 0, ann_rows = 0; | ||
| for (auto index : indices) { | ||
| RAFT_EXPECTS(dim == index->state->dim(), "Each index must have the same dimensionality"); | ||
| bfknn_rows += index->state->bfknn_rows(); | ||
| ann_rows += index->state->ann_rows(); | ||
| } | ||
|
|
||
| // degenerate case - all indices are empty, just re-use the state from the first index | ||
| if (!bfknn_rows && !ann_rows) { return indices[0]->state; } | ||
|
|
||
| // concatenate all the storages together | ||
| auto to_allocate = bfknn_rows + ann_rows; | ||
| auto new_storage = | ||
| std::make_shared<brute_force_storage<value_type>>(res, to_allocate, dim, include_norms); | ||
|
|
||
| for (auto index : indices) { | ||
| auto storage = index->state->storage; | ||
|
|
||
| // copy over dataset to new storage | ||
| raft::copy(res, | ||
| raft::make_device_matrix_view<value_type, int64_t, raft::row_major>( | ||
| new_storage->dataset.data() + new_storage->num_rows_used * dim, | ||
| storage->num_rows_used, | ||
| dim), | ||
| raft::make_device_matrix_view<const value_type, int64_t, raft::row_major>( | ||
| storage->dataset.data(), storage->num_rows_used, dim)); | ||
|
|
||
| // copy over precalculated norms | ||
| if (include_norms) { | ||
| raft::copy(res, | ||
| raft::make_device_vector_view<value_type, int64_t, raft::row_major>( | ||
| new_storage->norms->data() + new_storage->num_rows_used, storage->num_rows_used), | ||
| raft::make_device_vector_view<const value_type, int64_t, raft::row_major>( | ||
| storage->norms->data(), storage->num_rows_used)); | ||
| } | ||
| new_storage->num_rows_used += storage->num_rows_used; | ||
| } | ||
|
|
||
| auto next_state = std::make_shared<index_state<UpstreamT>>(*indices[0]->state); | ||
| next_state->storage = new_storage; | ||
| next_state->build_params = index_params; | ||
|
|
||
| if (next_state->bfknn_rows() > static_cast<size_t>(next_state->build_params.min_ann_rows)) { | ||
| next_state = compact(res, *next_state); | ||
| } | ||
| return next_state; | ||
| } | ||
|
|
||
| template <typename UpstreamT> | ||
| auto extend(raft::resources const& res, | ||
| const index_state<UpstreamT>& current, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -175,6 +175,33 @@ void search(raft::resources const& res, | |
| res, search_params, ivf_pq::typed_search, queries, neighbors, distances, sample_filter); | ||
| } | ||
|
|
||
| auto merge(raft::resources const& res, | ||
| const index_params<cagra::index_params>& index_params, | ||
| const std::vector<tiered_index::index<cagra::index<float, uint32_t>>*>& indices) | ||
| -> tiered_index::index<cagra::index<float, uint32_t>> | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think ideally we would have merge() return index and, as you have done, use the PIMPL idom to allow the return of either the same index type that was input, or a composite_index. |
||
| { | ||
| auto state = detail::merge(res, index_params, indices); | ||
| return cuvs::neighbors::tiered_index::index<cagra::index<float, uint32_t>>(state); | ||
| } | ||
|
|
||
| auto merge(raft::resources const& res, | ||
| const index_params<ivf_flat::index_params>& index_params, | ||
| const std::vector<tiered_index::index<ivf_flat::index<float, int64_t>>*>& indices) | ||
| -> tiered_index::index<ivf_flat::index<float, int64_t>> | ||
| { | ||
| auto state = detail::merge(res, index_params, indices); | ||
| return cuvs::neighbors::tiered_index::index<ivf_flat::index<float, int64_t>>(state); | ||
| } | ||
|
|
||
| auto merge(raft::resources const& res, | ||
| const index_params<ivf_pq::index_params>& index_params, | ||
| const std::vector<tiered_index::index<ivf_pq::typed_index<float, int64_t>>*>& indices) | ||
| -> tiered_index::index<ivf_pq::typed_index<float, int64_t>> | ||
| { | ||
| auto state = detail::merge(res, index_params, indices); | ||
| return cuvs::neighbors::tiered_index::index<ivf_pq::typed_index<float, int64_t>>(state); | ||
| } | ||
|
|
||
| template <typename UpstreamT> | ||
| int64_t index<UpstreamT>::size() const noexcept | ||
| { | ||
|
|
||
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.
Based on our discussion yesterday, we should just remove the "merge strategy" concept all together and support only physical in the "merge" apis. We can still provide a composite that will allow for multiple indexes to be searched concurrently, but don't need to make it part of the merge() apis.