Iterative CAGRA-Q#1810
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
aamijar
added
non-breaking
… search - Configurable growth-phase in-build search params (itopk_size, search_width, max_iterations) and internal/smem dtype; itopk auto-forced on the final full-size iteration. - Decouple compression params used during iterative construction from the target index compression. - Add shuffle_dataset option; fix out-of-bounds access from the in-place raft gather by switching to an out-of-place gather.
…around) The shuffle_dataset path used an out-of-place gather into a temporary buffer to work around an illegal memory access in raft's in-place gather overload when n_rows * row_len exceeded 2^31 (32-bit index overflow). That bug is now fixed upstream in raft (NVIDIA/raft#3059, closes #3055), which the cuvs raft pin now includes. Revert to the in-place gather to drop the extra full-size temporary allocation and copy.
8fc10ac to
4f4068a
Compare
|
Hi @irina-resh-nvda, thanks for the contribution! I've added some commits to clean up some small things in this PR:
|
| // {std::optional<bool>{std::nullopt}}, | ||
| // {cuvs::neighbors::MergeStrategy::MERGE_STRATEGY_PHYSICAL, | ||
| // cuvs::neighbors::MergeStrategy::MERGE_STRATEGY_LOGICAL}); | ||
| // inputs.insert(inputs.end(), inputs2.begin(), inputs2.end()); |
There was a problem hiding this comment.
Why has this test been commented out?
| {16}, // k | ||
| {32}, // degree | ||
| { // graph_build_algo::IVF_PQ, | ||
| // graph_build_algo::NN_DESCENT, |
There was a problem hiding this comment.
Same question: what is the reason for the changes here?
| {true}, | ||
| {false}, | ||
| {0.995}, | ||
| {0.01}, |
There was a problem hiding this comment.
Why has the recall changed to 1%?
| // {std::optional<bool>{std::nullopt}}, | ||
| // {cuvs::neighbors::MergeStrategy::MERGE_STRATEGY_PHYSICAL, | ||
| // cuvs::neighbors::MergeStrategy::MERGE_STRATEGY_LOGICAL}); | ||
| // inputs.insert(inputs.end(), inputs2.begin(), inputs2.end()); |
There was a problem hiding this comment.
Same question: why was this commented out?
| // Create a dataset with 1000 points | ||
| constexpr int64_t n_dataset = 1000; | ||
| // Create a dataset with 10000 points | ||
| constexpr int64_t n_dataset = 10000; |
There was a problem hiding this comment.
What is the reason this test file has been changed to use n_dataset = 10,000?
| dev_ptr, | ||
| n_bytes, | ||
| cudaMemcpyDeviceToHost, | ||
| writeback_stream_)); |
There was a problem hiding this comment.
We should have a raft function that does this already.
// use memcpy instead of raft::copy to avoid strange behavior with HMM/ATS memory
If there is a problem here, we should try to root fix that in raft.
| host_view_.data_handle() + src_row_offset * host_view_.extent(1), | ||
| n_bytes, | ||
| cudaMemcpyHostToDevice, | ||
| prefetch_stream_)); |
There was a problem hiding this comment.
Same as before: let's try to use the raft copy function.
| cudaPointerAttributes attr; | ||
| RAFT_CUDA_TRY(cudaPointerGetAttributes(&attr, ptr)); | ||
| return attr.hostPointer != nullptr; | ||
| } |
There was a problem hiding this comment.
We should already have utilities somewhere to do check if a pointer is host or device accessible, either within in raft or cuvs.
I can try to find those functions for you.
…build-time random seeds
| * @tparam IdxT The type of the index | ||
| */ | ||
| template <typename T, typename IdxT> | ||
| class batched_device_view_from_host { |
There was a problem hiding this comment.
Can't we use batch_load_iterator for the same purpose?
5 participants
Iterative cagra graph construction using CAGRA-Q search.
This PR improves the iterative CAGRA build method by enabling PQ compression: the dataset is compressed before the iterative search starts, and CAGRA-Q is used to iteratively update the KNN graph.