[Feat] Expose C API for CAGRA merge
#860
Conversation
rhdong
added
feature request
| * | ||
| */ | ||
| /** Strategy for merging CAGRA indices. */ | ||
| typedef enum { |
There was a problem hiding this comment.
Can we reuse this in the C++ version so we don't have to duplicate?
There was a problem hiding this comment.
I suppose we can't do it because C doesn't support namespaces, :: , and enum class. It's not the first time we re-define in C( which should work well), like hash_mode vs cuvsCagraHashMode
There was a problem hiding this comment.
Can we forward it? Something like "using cuvsCagraMergeStrategy as cuvs::neighbors::cagra::merge_strategy"?
There was a problem hiding this comment.
We do this with the the "DistanceType` in pairwise distances. Maybe we can do the same thing here?
There was a problem hiding this comment.
We do this with the the "DistanceType` in pairwise distances. Maybe we can do the same thing here?
Sorry, I found we defined the separate DistanceType and converted it to cpp type via static_cast. Maybe I found the improper position. Could you hint me at the code position, by any chance, many thanks?
cpp/src/neighbors/cagra_c.cpp
Outdated
| if (out_idx_params.graph_build_params) { | ||
| auto ivf_params = static_cast<cuvsIvfPqParams*>(out_idx_params.graph_build_params); | ||
| if (ivf_params->ivf_pq_build_params) { | ||
| auto bp = ivf_params->ivf_pq_build_params; | ||
| pq_params.build_params.add_data_on_build = bp->add_data_on_build; | ||
| pq_params.build_params.n_lists = bp->n_lists; | ||
| pq_params.build_params.kmeans_n_iters = bp->kmeans_n_iters; | ||
| pq_params.build_params.kmeans_trainset_fraction = bp->kmeans_trainset_fraction; | ||
| pq_params.build_params.pq_bits = bp->pq_bits; | ||
| pq_params.build_params.pq_dim = bp->pq_dim; | ||
| pq_params.build_params.codebook_kind = | ||
| static_cast<cuvs::neighbors::ivf_pq::codebook_gen>(bp->codebook_kind); | ||
| pq_params.build_params.force_random_rotation = bp->force_random_rotation; | ||
| pq_params.build_params.conservative_memory_allocation = | ||
| bp->conservative_memory_allocation; | ||
| pq_params.build_params.max_train_points_per_pq_code = bp->max_train_points_per_pq_code; | ||
| } | ||
| if (ivf_params->ivf_pq_search_params) { | ||
| auto sp = ivf_params->ivf_pq_search_params; | ||
| pq_params.search_params.n_probes = sp->n_probes; | ||
| pq_params.search_params.lut_dtype = sp->lut_dtype; | ||
| pq_params.search_params.internal_distance_dtype = sp->internal_distance_dtype; | ||
| pq_params.search_params.preferred_shmem_carveout = sp->preferred_shmem_carveout; | ||
| } | ||
| if (ivf_params->refinement_rate > 1.0f) { | ||
| pq_params.refinement_rate = ivf_params->refinement_rate; | ||
| } | ||
| } |
There was a problem hiding this comment.
A bunch of this code seems cut and paste from the _build function - can we move into a function and share it so that we don't need to keep up to date in two spots?
|
/ok to test cbefdc6 |
merge