Migrate feature diff for NN Descent from RAFT to cuVS #421
Conversation
divyegala
added
feature request
| void build(raft::resources const& res, | ||
| index_params const& params, | ||
| raft::device_matrix_view<const float, int64_t, raft::row_major> dataset, | ||
| index<uint32_t>& index); |
There was a problem hiding this comment.
Why doesn't nn-descent return the built index like all the other index types?
There was a problem hiding this comment.
It does, there's an API for that as well. We need this API as well especially for CAGRA because it needs to own the knn graph that it sends to NN Descent. For that, it needs to construct an index first and that's why we need this API.
There was a problem hiding this comment.
But the usage docs for this function don't reflect that. The whole idea behind these build functon is to abstract away the index so that the user (and anyone using the public APIs) don't need to think about the underlying index object. Instead of having the user construct the index object on their own, we shuold have them pass an optional knn graph into the build function that it then uses when it constructs the underlying index instance underneath.
| index<uint32_t>& index); | ||
|
|
||
| /** | ||
| * @brief Build nn-descent Index with dataset in host memory |
There was a problem hiding this comment.
Make sure all of these are exposed through the docs.
There was a problem hiding this comment.
Yes, they are part of the doxygen group that is already in the docs source.
| @@ -146,6 +189,125 @@ class AnnNNDescentTest : public ::testing::TestWithParam<AnnNNDescentInputs> { | |||
| rmm::device_uvector<DataT> database; | |||
| }; | |||
|
|
|||
| template <typename DistanceT, typename DataT, typename IdxT> | |||
| class AnnNNDescentBatchTest : public ::testing::TestWithParam<AnnNNDescentBatchInputs> { | |||
There was a problem hiding this comment.
Do we have this consistently passing yet?
There was a problem hiding this comment.
No, that's a follow-on item. The tests are disabled for now both here and in RAFT.
|
/merge |
This PR is an amalgamation of the diff of 3 PRs in RAFT:
This PR also addresses part 1 and 2 of #419, closes #391 and makes CAGRA use the compiled headers of NN Descent, which seemed to have been a pending TODO
cuvs/cpp/src/neighbors/detail/cagra/cagra_build.cuh
Lines 36 to 37 in 009bb8d
Also, batch tests are disabled in this PR due to issue rapidsai/raft#2450. PR #424 will attempt to re-enable them.