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 6 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[out] vpq_dataset newly allocated owning VPQ dataset handle | ||
| * @return cuvsError_t | ||
| */ | ||
| CUVS_EXPORT cuvsError_t cuvsDatasetMakeVpq(cuvsResources_t res, |
There was a problem hiding this comment.
Let's drop the V from this and call it cuvsDatasetMakePq.
There was a problem hiding this comment.
Just wanted to clarify my understanding of make_vpq_dataset() factory. There is a PQ step and a VQ step but the VQ step is optional and run only when the flag bool use_vq = true. Is that why we are renaming the make_vpq_dataset() factory to make_pq_dataset()? So we will have pq dataset, rabitq dataset, bbq dataset, and sq dataset but no standalone vpq dataset? We will treat vpq as a special condition of pq dataset?
There was a problem hiding this comment.
The VQ is not a proper VQ. It's misnamed. It's coarse centroid assignments. But we don't need to include that in the name.
| * @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");
| */ | ||
| template <typename T, typename IdxT, typename IndexViewT> | ||
| requires cuvs::neighbors::ann_dataset_view<IndexViewT> | ||
| auto attach_dataset(raft::resources const& res, |
There was a problem hiding this comment.
Please stop naming this attach_dataset. The only function that should be exposed to the outside world is update_dataset. Since we're not rushing to get this into 26.08, we need to fix this.
| [[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.
|
|
||
| TEST(CagraC, BuildAttachVpqSearch) | ||
| { | ||
| // CAGRA-Q smoke test: dense build → MakeVpq → UpdateDataset(VPQ) → Search. |
There was a problem hiding this comment.
Thanks for adding the tests to C api and language wrappers. I don't see tests in cpp though. We should add that.
| 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 |
| ) | ||
| padded = make_device_padded_dataset(dataset_device) | ||
| compression = cagra.CompressionParams(pq_bits=8, pq_dim=8) | ||
| vpq = cagra.make_vpq_dataset(padded, compression_params=compression) |
There was a problem hiding this comment.
Are we saying the user needs to do make_device_padded_dataset before make_vpq_dataset? Can you just call make_vpq_dataset by itself?
|
This PR is marked a a breaking change, can you clarify what is breaking here? |
3 participants
Addresses issue #2405.