-
Notifications
You must be signed in to change notification settings - Fork 218
Recover cagra-q path in C API and downstream language wrappers #2413
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
base: main
Are you sure you want to change the base?
Changes from all commits
309c59b
6418b95
f2ed015
fb5cb26
645519c
114b72a
035140d
d294c2f
a933dae
61b2dc0
65a7c0c
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 |
|---|---|---|
|
|
@@ -255,6 +255,24 @@ CUVS_EXPORT cuvsError_t cuvsCagraCompressionParamsCreate(cuvsCagraCompressionPar | |
| */ | ||
| CUVS_EXPORT cuvsError_t cuvsCagraCompressionParamsDestroy(cuvsCagraCompressionParams_t params); | ||
|
|
||
| /** | ||
| * @brief Train an owning device VPQ (f16 codebook) dataset from a device-padded source. | ||
| * | ||
| * Used for CAGRA-Q: build a dense CAGRA index, train VPQ with this factory, then attach via | ||
| * `cuvsCagraUpdateDataset`. Caller owns the returned dataset and must keep it alive while any | ||
| * index uses it. Metric for subsequent search must remain `L2Expanded`. | ||
| * | ||
| * @param[in] res cuvs resources | ||
| * @param[in] source_dataset device-padded dataset (owning or view) | ||
| * @param[in] params VPQ compression params; NULL selects defaults | ||
| * @param[out] vpq_dataset newly allocated owning VPQ dataset handle | ||
| * @return cuvsError_t | ||
| */ | ||
| CUVS_EXPORT cuvsError_t cuvsDatasetMakeVpq(cuvsResources_t res, | ||
| cuvsDataset_t source_dataset, | ||
| cuvsCagraCompressionParams_t params, | ||
| cuvsDataset_t* vpq_dataset); | ||
|
|
||
| /** | ||
| * @brief Allocate ACE params, and populate with default values | ||
| * | ||
|
|
@@ -580,21 +598,25 @@ CUVS_EXPORT cuvsError_t cuvsCagraIndexGetDataset(cuvsCagraIndex_t index, DLManag | |
| CUVS_EXPORT cuvsError_t cuvsCagraIndexGetGraph(cuvsCagraIndex_t index, DLManagedTensor* graph); | ||
|
|
||
| /** | ||
| * @brief Update a CAGRA index with a device-padded dataset. | ||
| * @brief Update a CAGRA index with a device dataset (padded or VPQ). | ||
| * | ||
| * This is the centralized dataset update/attach operation for C callers. | ||
| * | ||
| * - Device-padded dataset: if \p index is already device-padded, its dataset view is replaced in | ||
| * place (same index object); otherwise the index is converted via attach and rebound. | ||
| * - Device VPQ_F16 dataset (from `cuvsDatasetMakeVpq`): if \p index is already VPQ-typed, its | ||
| * dataset view is replaced in place; otherwise the graph is copied into a new VPQ-typed index | ||
| * (CAGRA-Q). Search requires metric `L2Expanded`. The VPQ handle must be owning. | ||
| * | ||
| * This is the centralized dataset update operation for C callers. If \p index | ||
| * is already device-padded, its dataset view is replaced in place. Otherwise, | ||
| * the index is converted and its opaque handle is rebound to a search-ready | ||
| * device-padded index. Caller retains ownership of | ||
| * \p device_padded_dataset and must keep it alive while \p index uses it. | ||
| * Caller retains ownership of \p dataset and must keep it alive while \p index uses it. | ||
| * | ||
| * @param[in] res cuvsResources_t opaque C handle | ||
| * @param[in] device_padded_dataset owning or non-owning device-padded dataset handle | ||
| * @param[inout] index CAGRA index handle | ||
| * @param[in] res cuvsResources_t opaque C handle | ||
| * @param[in] dataset device-padded or owning device VPQ_F16 dataset handle | ||
|
Contributor
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. Hmm. I don't know that i like this naming. VPQ_F16... that's cryptic... and why only F16? Is that really the only data type that's supported? Let's keep the naming contention here:
Contributor
Author
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. VPQ_F32 is not supported right now in the cagra-q search kernels. Source vectors can be f32/f16/int8/uint8 but the codebook math type stored for search is forced to half. compute_distance_vpq-impl.cuh has this line:
Contributor
Author
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. |
||
| * @param[inout] index CAGRA index handle | ||
| * @return cuvsError_t | ||
| */ | ||
| CUVS_EXPORT cuvsError_t cuvsCagraUpdateDataset(cuvsResources_t res, | ||
| cuvsDataset_t device_padded_dataset, | ||
| cuvsDataset_t dataset, | ||
| cuvsCagraIndex_t index); | ||
|
|
||
| /** | ||
|
|
||
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4548,6 +4548,42 @@ auto convert_standard_to_padded_index( | |
| return out; | ||
| } | ||
|
|
||
| /** | ||
| * @brief Convert a dense (non-VPQ) CAGRA index into a VPQ f16 index and attach a VPQ dataset. | ||
| * | ||
| * Copies graph/source-indices from `src` into a new `vpq_f16_index` and attaches `vpq_dataset`. | ||
| * Caller owns `vpq_dataset` storage for the lifetime of the returned index. | ||
| */ | ||
| template <typename T, typename IdxT, typename IndexViewT> | ||
| requires cuvs::neighbors::ann_dataset_view<IndexViewT> | ||
| auto convert_dense_to_vpq_f16_index( | ||
|
Contributor
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. This function seems unnecessary. We shouldn't be copying the cagra graph just to change the template type.
Contributor
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. This should not be in the include directory. |
||
| raft::resources const& res, | ||
| index<T, IdxT, IndexViewT> const& src, | ||
| cuvs::neighbors::device_vpq_dataset_view<half, int64_t> const& vpq_dataset) | ||
| -> vpq_f16_index<T, IdxT> | ||
| { | ||
| RAFT_EXPECTS(vpq_dataset.n_rows() == src.size(), | ||
| "VPQ dataset row count must match the index size"); | ||
|
|
||
| vpq_f16_index<T, IdxT> out(res, src.metric()); | ||
| if (src.graph().extent(0) > 0) { | ||
| using GraphIndexType = typename index<T, IdxT, IndexViewT>::graph_index_type; | ||
| auto graph_host = | ||
| raft::make_host_matrix<GraphIndexType, int64_t>(src.graph().extent(0), src.graph().extent(1)); | ||
| raft::copy(graph_host.data_handle(), | ||
| src.graph().data_handle(), | ||
| src.graph().size(), | ||
| raft::resource::get_cuda_stream(res)); | ||
| raft::resource::sync_stream(res); | ||
| out.update_graph(res, raft::make_const_mdspan(graph_host.view())); | ||
| } | ||
| if (src.source_indices().has_value()) { | ||
| out.update_source_indices(res, src.source_indices().value()); | ||
| } | ||
| out.update_device_dataset_same_layout(res, vpq_dataset); | ||
| return out; | ||
| } | ||
|
|
||
| /** | ||
| * @brief Attach a device-padded dataset and return a search-ready padded-device index. | ||
| * | ||
|
|
@@ -4598,6 +4634,35 @@ auto attach_dataset( | |
| } | ||
| } | ||
|
|
||
| /** | ||
| * @brief Attach a device VPQ (f16 codebook) dataset and return a search-ready `vpq_f16_index`. | ||
| * | ||
| * Builds a new VPQ-typed index by copying the graph from `idx`. Caller owns `vpq_dataset` storage. | ||
| * | ||
| * If `idx` is already a `vpq_f16_index`, call `idx.update_device_dataset_same_layout(res, | ||
| * vpq_dataset)` directly. | ||
| */ | ||
| template <typename T, typename IdxT, typename IndexViewT> | ||
| requires cuvs::neighbors::ann_dataset_view<IndexViewT> | ||
| auto attach_dataset(raft::resources const& res, | ||
|
Contributor
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. Please stop naming this attach_dataset. The only function that should be exposed to the outside world is
Contributor
Author
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. This attach_dataset() is not meant to be called publicly at the C++ API layer. It is an internal helper that is called within the user-callable public facing update_dataset() which is the single entry point for users to update/attach any sort of dataset. C API and language wrappers already have this implementation but due to mutability guarantees in C++, we didn't get to implementing a unified update_dataset() at the C++ API layer in 26.08. Am planning to unify it under a single public facing update_dataset() at C++ API layer with separate overloads with different mutability guarantees wrapping attach_dataset() and update_device_dataset_same_layout() in a separate PR.
Contributor
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. If it's not meant to be called publicly then put it in src/ We also should not have templates for free functions in header files in include
Contributor
Author
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. Got it, will address this in a separate PR!
Contributor
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. Please address it here . This is being introduced in this PR. I would prefer to fix it here. |
||
| index<T, IdxT, IndexViewT> const& idx, | ||
| cuvs::neighbors::device_vpq_dataset_view<half, int64_t> const& vpq_dataset) | ||
| -> vpq_f16_index<T, IdxT> | ||
| { | ||
| if constexpr (cuvs::neighbors::is_device_vpq_f16_dataset_view_v<IndexViewT>) { | ||
| RAFT_LOG_WARN( | ||
| "cagra::attach_dataset called with an already vpq_f16 index. " | ||
| "To avoid an unnecessary index copy, call " | ||
| "index.update_device_dataset_same_layout(res, vpq_dataset) " | ||
| "directly on the original index."); | ||
| RAFT_FAIL( | ||
| "cagra::attach_dataset: vpq_f16_index input is not supported in this overload. " | ||
| "Call index.update_device_dataset_same_layout(res, vpq_dataset) directly."); | ||
| } else { | ||
| return convert_dense_to_vpq_f16_index(res, idx, vpq_dataset); | ||
| } | ||
| } | ||
|
|
||
| } // namespace cagra | ||
| } // namespace neighbors | ||
| } // namespace CUVS_EXPORT cuvs | ||
|
|
||
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.