Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Port NN-descent algorithm to use in cagra::build() #1748

Merged
merged 40 commits into from
Sep 26, 2023
Merged

Port NN-descent algorithm to use in cagra::build() #1748

merged 40 commits into from
Sep 26, 2023

Conversation

divyegala
Copy link
Member

@divyegala divyegala commented Aug 17, 2023 •
edited
Loading

  • Build Time comparison of end-to-end RAFT CAGRA+nn-descent against cuANN CAGRA+nn-descent
  • Recall comparison of RAFT nn-descent against cuANN nn-descent
  • RAFT types/APIs in ported code from cuANN
  • End-to-end CAGRA+nn-descent tests
  • Docs and code examples
  • Add graph_build_algo build param to CAGRA ann-bench for benchmarking builds with IVF-PQ or NN-Descent
  • All-neighbors knn graph nn-descent tests against brute-force knn

Recall Value comparison of RAFT nn-descent vs cuANN nn-descent

Dataset	graph_degree, intermediate_degree	Iterations	cuANN Recall	RAFT Recall
sift-128-euclidean	(64, 98)	            15	        0.9265991875	0.9471194688
sift-128-euclidean	(64, 98)	            50	        0.9831858594	0.9783938594
deep-image-96-inner	(64, 98)	            50	        0.9806211946	0.9801508853

@divyegala divyegala added feature request New feature or request non-breaking Non-breaking change labels Aug 17, 2023
@divyegala divyegala self-assigned this Aug 17, 2023
@github-actions github-actions bot added the cpp label Aug 17, 2023
@copy-pr-bot
Copy link

copy-pr-bot bot commented Aug 30, 2023

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@divyegala
Copy link
Member Author

/ok to test

@divyegala
Copy link
Member Author

/ok to test

@divyegala divyegala requested a review from a team as a code owner September 14, 2023 21:17
@divyegala
Copy link
Member Author

/ok to test

@divyegala divyegala mentioned this pull request Sep 14, 2023
Open
7 tasks
@divyegala
Copy link
Member Author

/ok to test

@divyegala
Copy link
Member Author

@cjnolet @tfeher I have also created a tracker issue for follow-up tasks #1827

This PR is now ready for a re-review.

tfeher
tfeher approved these changes Sep 20, 2023
View reviewed changes
Copy link
Contributor

@tfeher tfeher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @divyegala for addressing the issues so far! I have marked two smaller items to be addressed, otherwise the PR looks good to me, pre-approving.

cpp/include/raft/neighbors/detail/nn_descent.cuh Outdated Show resolved Hide resolved
cpp/include/raft/neighbors/detail/nn_descent.cuh Show resolved Hide resolved
cpp/include/raft/neighbors/detail/nn_descent.cuh Show resolved Hide resolved
cpp/include/raft/neighbors/detail/nn_descent.cuh Show resolved Hide resolved
@divyegala
Copy link
Member Author

/ok to test

cjnolet
cjnolet requested changes Sep 22, 2023
View reviewed changes
Copy link
Member

@cjnolet cjnolet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is almost there. Most of what's left is trivial clean up things (aside from the follow-on items)

cpp/include/raft/neighbors/cagra.cuh Outdated Show resolved Hide resolved
cpp/include/raft/neighbors/cagra_types.hpp Outdated Show resolved Hide resolved
cpp/include/raft/neighbors/detail/nn_descent.cuh Show resolved Hide resolved
cpp/include/raft/neighbors/detail/nn_descent.cuh Show resolved Hide resolved
cpp/include/raft/neighbors/detail/nn_descent.cuh
const size_t internal_node_degree,
const size_t num_samples);
void init_random_graph();
// Use Bloom filter to sample "new" neighbors for local joining
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add todo comment for this and reference the GitHub issue in the code.

cpp/include/raft/neighbors/nn_descent_types.hpp Show resolved Hide resolved
cpp/test/CMakeLists.txt Outdated Show resolved Hide resolved
cpp/test/neighbors/ann_cagra.cuh Show resolved Hide resolved
cpp/test/neighbors/ann_nn_descent.cuh Outdated Show resolved Hide resolved
python/pylibraft/pylibraft/test/test_cagra.py Show resolved Hide resolved
@divyegala
Copy link
Member Author

/ok to test

cjnolet
cjnolet approved these changes Sep 25, 2023
View reviewed changes
Copy link
Member

@cjnolet cjnolet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@cjnolet
Copy link
Member

cjnolet commented Sep 25, 2023

/ok to test

@cjnolet
Copy link
Member

cjnolet commented Sep 25, 2023

/merge

@divyegala
Copy link
Member Author

/ok to test

@cjnolet
Copy link
Member

cjnolet commented Sep 26, 2023

/merge

@rapids-bot rapids-bot bot merged commit a1002f8 into rapidsai:branch-23.10 Sep 26, 2023
57 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tfeher tfeher tfeher approved these changes

@RayWang96 RayWang96 RayWang96 approved these changes

@cjnolet cjnolet cjnolet approved these changes

@raydouglass raydouglass raydouglass approved these changes

Assignees

@divyegala divyegala

Labels
ci CMake cpp feature request New feature or request Milvus non-breaking Non-breaking change python REDIS Vector Search
Projects
VS/ML/DM Primitives Release Board
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@divyegala @cjnolet @raydouglass @tfeher @RayWang96