-
Notifications
You must be signed in to change notification settings - Fork 196
Auto select CAGRA build algorithm for hnsw::build #1719
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
5297ce0
1b5eb73
c9ad00a
10cd13f
21a24fa
7ce5cd0
958cdbf
cd2ef18
9020994
a4b5df0
44f07db
78db247
41343ba
4c33a4a
ce9692d
1442604
9b56a19
c918a3f
aa6c26f
2b72f2e
871ebb9
db2a1cf
5d52dc4
95daa58
6f232ab
1ce8715
f6d8c35
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 |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| /* | ||
| * SPDX-FileCopyrightText: Copyright (c) 2023-2025, NVIDIA CORPORATION. | ||
| * SPDX-FileCopyrightText: Copyright (c) 2023-2026, NVIDIA CORPORATION. | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
| #pragma once | ||
|
|
@@ -85,38 +85,9 @@ void cuvs_cagra_hnswlib<T, IdxT>::build(const T* dataset, size_t nrow) | |
| // when the data set is on host, we can pass it directly to HNSW | ||
| bool dataset_is_on_host = raft::get_device_for_address(dataset) == -1; | ||
|
|
||
| // re-use the CAGRA wrapper to parse build params | ||
| auto bps = build_param_.cagra_build_params; | ||
| // Not very conveniently, the CAGRA wrapper resolves parameters after the dataset shape is known, | ||
| // so it takes a lambda to do it. Even though we know the shape, we want to use the wrapper as-is, | ||
| // so we just modify that lambda. | ||
| bps.cagra_params = [dataset_is_on_host, orig_cagra_params = bps.cagra_params]( | ||
| auto dataset_extents, auto metric) { | ||
| auto params = orig_cagra_params(dataset_extents, metric); | ||
| params.attach_dataset_on_build = !dataset_is_on_host; | ||
| return params; | ||
| }; | ||
| cuvs_cagra<T, IdxT> cagra_wrapper{this->metric_, this->dim_, bps}; | ||
|
|
||
| // build the CAGRA index | ||
| cagra_wrapper.build(dataset, nrow); | ||
| auto& cagra_index = *cagra_wrapper.get_index(); | ||
|
|
||
| // pass the dataset directly to HNSW if it's on the host | ||
| std::optional<raft::host_matrix_view<const T, int64_t>> opt_dataset_view = std::nullopt; | ||
| if (dataset_is_on_host) { | ||
| opt_dataset_view.emplace( | ||
| raft::make_host_matrix_view<const T, int64_t>(dataset, nrow, this->dim_)); | ||
| } | ||
|
|
||
| auto dataset_view = raft::make_host_matrix_view<const T, int64_t>(dataset, nrow, this->dim_); | ||
| // convert the index to HNSW format | ||
| hnsw_index_ = cuvs::neighbors::hnsw::from_cagra( | ||
| handle_, build_param_.hnsw_index_params, cagra_index, opt_dataset_view); | ||
|
|
||
| // special treatment in save/serialize step | ||
| if (cagra_index.dataset_fd().has_value() && cagra_index.graph_fd().has_value()) { | ||
| cagra_ace_build_ = true; | ||
| } | ||
| hnsw_index_ = cuvs::neighbors::hnsw::build(handle_, build_param_.hnsw_index_params, dataset_view); | ||
|
Comment on lines
+88
to
+90
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. Preserve ACE/file-backed build state after direct Line 90 can now return an ACE-backed HNSW index, but this path never updates 🛠️ Proposed fix auto dataset_view = raft::make_host_matrix_view<const T, int64_t>(dataset, nrow, this->dim_);
// convert the index to HNSW format
hnsw_index_ = cuvs::neighbors::hnsw::build(handle_, build_param_.hnsw_index_params, dataset_view);
+ cagra_ace_build_ = !hnsw_index_->file_path().empty();🤖 Prompt for AI Agents |
||
| } | ||
|
|
||
| template <typename T, typename IdxT> | ||
|
|
||
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.
Is the data expected to always reside in host memory?
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.
ACE only supports host memory right now. The main reasons is that we expect the data size to be large and memory-mapped. Further, we do the partitioning and reordering on the host since there is no benefit of moving it to the GPU only to write it to disk afterwards.
Anyways, I think we can support device datasets easily since these should not end up using ACE with this heuristic. @tfeher What do you think?