Recover cagra-q path in C API and downstream language wrappers - #2413
Recover cagra-q path in C API and downstream language wrappers#2413HowardHuang1 wants to merge 14 commits into
Conversation
…ke_vpq_dataset() factory and update_dataset() branches for attaching vpq dataset at C API and downstream language wrappers
HowardHuang1
added
feature request
…estored cagra-q vpq search path
92cae9c to
6418b95
Compare
| * @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 |
There was a problem hiding this comment.
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: device-padded or pq`. Also- there's no reason we should need to specify "owning device" here... that should be opaque to the user (they just create a dataset in the c layer and they pass it in and we worry about the view creation under the hood).
There was a problem hiding this comment.
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:
static_assert(std::is_same_v<CODE_BOOK_T, half>, "Only CODE_BOOK_T = half is supported now");
| [[nodiscard]] auto make_vpq_dataset(raft::resources const& res, | ||
| cuvs::neighbors::vpq_params const& params, | ||
| SrcT const& src) | ||
| [[nodiscard]] auto make_device_vpq_dataset(raft::resources const& res, |
There was a problem hiding this comment.
Please rename- pq_dataset. We don't need the vpq in the name.
| } | ||
|
|
||
| // Owning VPQ dataset handle for CAGRA-Q search. | ||
| type VpqDataset struct { |
There was a problem hiding this comment.
Why is this a separate struct? Also.. please rename ALL instances of vpq_dataset to pq_dataset.
There was a problem hiding this comment.
It was a mistake that it was ever named vpq to begin with- I understand it does a vector quant on top of the pq, but that's a feature, not a defining name.
| */ | ||
| template <typename T, typename IdxT, typename IndexViewT> | ||
| requires cuvs::neighbors::ann_dataset_view<IndexViewT> | ||
| auto convert_dense_to_vpq_f16_index( |
There was a problem hiding this comment.
This function seems unnecessary. We shouldn't be copying the cagra graph just to change the template type.
I am making the change to update_dataset to do the move instead of copy. We should be able to reuse update_dataset for this.
| CUVS_DATASET_LAYOUT_PADDED = 1 | ||
| CUVS_DATASET_LAYOUT_PADDED = 1, | ||
| /** Device VPQ storage with f16 codebooks (CAGRA-Q search dataset). */ | ||
| CUVS_DATASET_LAYOUT_VPQ_F16 = 2 |
There was a problem hiding this comment.
| CUVS_DATASET_LAYOUT_VPQ_F16 = 2 | |
| CUVS_DATASET_LAYOUT_PQ = 2 |
|
This PR is marked a a breaking change, can you clarify what is breaking here? |
…Vpq at C API layer to pq instead of vpq. Language wrappers have not undergone this renaming yet.
…_vpq_dataset(), update_device_dataset_same_layout(), and dispatch functions at C API layer. Dispatch now happens through overloads of update_dataset() at C++ API layer. C API layer just calls C++ update_dataset() function and the C++ API layer routes to the correct overload. This unifies C++ API layer to use centralized update_dataset() as single entry point for caller rather than having separate attach_dataset() and update_device_dataset_same_layout() functions
4 participants
Addresses issue #2405.