All neighbors C and Python bindings by viclafargue · Pull Request #1282 · rapidsai/cuvs

viclafargue · 2025-08-26T14:28:28Z

Answers #1279

jinsolp

Thanks for this PR @viclafargue ! Adding a few comments 🙂

jinsolp · 2025-08-27T17:59:59Z

    def __dealloc__(self):
        check_cuvs(cuvsResourcesDestroy(self.c_obj))

+cdef class SNMGResources:


I didn't know we had a separate resource wrapper in cuVS. If there is no explicit reason to not import raft, I think we can just use pylibraft.common.DeviceResourcesSNMG (used like this in cuML).
I think we should be able to have cuvsResources_t to be the arg type in the c api which will dynamically dispatch to the multi gpu if the user passes DeviceResourcesSNMG as the handle.

I removed the cuvsSNMGResources_t type to simplify the API. But, we need the user to be able to instantiate a multi-GPU resource from C. This also enables other bindings. When it comes to handling RAFT handles, this is definitely something we would want to do, but I feel like this may be out of the scope of this PR. Implementing it only in All Neighbors would be inconsistent with the rest of the cuVS Python API. Maybe others would have different insights though. cc @cjnolet

Nitpick- can we please rename deviceSNMG* to DeviceMultiGpu all the way down to the C++ layer? I'm realizing this naming is becoming confusing to users.

Any C/C++ public APIs should absolutely not leak RAFT types. This is actually for more reasons than may be immediately obvious- it turns out the way symbol lookups are resolved in binaries are non-deterministic, and so they can causes nasty segfaults when, say, a symbol is looked up in cugraph or cuml, instantiated, and then later looked up in cuVS (where the instantiated symbol resolves to an uninstantiated one, thus causing a memory illegal access).

I think it's okay to use RAFT types in the Python layer only, so long as the symbol is being exposed through pylibraft's cython/python layer and not being wrapped through cuVS' cython.

cjnolet · 2025-08-29T12:33:36Z

@@ -0,0 +1,71 @@
+All-neighbors


I love the docs, but all-neighbros isn't really an index type, it's more-so a way to take a series of dense vectors and construct a knn graph from them (it's really a way to convert dense vectors into a corresponding knn graph when you think about it).
That said, I think this documentation is immensely useful, I'm just proposing we put it in a different spot in the dots (that better reflects its purpose) or rename the "indexes" directory/rst file to be something else- like neighbors.

What would you like the exact change to be regarding documentation?

I put the cpp doc here so maybe this should go under docs/source/c_api

First change I would make is to not put it in the "indexes" docs (`docs/source/indexes/all_neighbors.rst).

The all_neighbors.rst does not document the C API though. In the end, what made the most sense was to rename the indexes directory to neighbors and Nearest Neighbor Indexes to Nearest Neighbor. I also updated the links.

cjnolet · 2025-08-29T12:41:52Z

    return wrapper
+
+
+def auto_sync_snmg_resources(f):


The more I think about this, the more I realize we should just be using pylibraft for this instead of wrapping a new type just in cuvs. That would allow cuml and others to benefit from this as well.

I agree that it would be neat to allow the use of RAFT handles. But, I think that this would have to be a dedicated follow-up work across the full API. Otherwise, it would need a lot of rework and would make the cuVS API inconsistent.

@viclafargue if you haven't done so already, can you please create an issue for this so it doesn't get lost in the noise of all other things going on?

cjnolet · 2025-09-16T17:05:48Z

+from cuvs.common.exceptions import check_cuvs
+
+
+cdef class MultiGpuResources:


I haven't checked yet, but do we have a Github issue for this one? Can you please reference it in the TODO here so that we don't lose sight of it? Having a DeviceResources from raft and MultiGpuResources is not consistent and it's ultimately going to affect the user experience. We should make that fix asap (though we don't need to hold up this PR).

I think Victor left an issue here. Please correct me if this isn't the right one @viclafargue

Thanks @jinsolp, an issue has indeed been opened: #1318.

jinsolp

Thanks for the work Victor! Left a few comments : )

jinsolp · 2025-09-16T16:58:25Z

@@ -0,0 +1,71 @@
+All-neighbors


I put the cpp doc here so maybe this should go under docs/source/c_api

jinsolp · 2025-09-16T17:23:22Z

+    if snmg:
+        assert recall > 0.85


Do we experience recall drops with snmg?

The recall for multi-GPU IVF-PQ was a bit low. I updated the n_lists parameters for the multi-GPU case. I set a unique threshold at 0.83 to be resilient to minor testing inconsistencies.

Got it. yeah ivfpq seemed to be very sensitive to n_lists.

jinsolp

Looking great @viclafargue ! A few final minor comments about the docs 🙂

cjnolet · 2025-09-18T16:42:49Z

/merge

All neighbors Python bindings

9256f97

viclafargue requested review from a team as code owners August 26, 2025 14:28

github-project-automation Bot added this to Unstructured Data Processing Aug 26, 2025

github-project-automation Bot moved this to Todo in Unstructured Data Processing Aug 26, 2025

KyleFromNVIDIA approved these changes Aug 26, 2025

View reviewed changes

viclafargue added improvement Improves an existing functionality non-breaking Introduces a non-breaking change labels Aug 26, 2025

viclafargue added 2 commits August 26, 2025 16:56

Allowing indices in-place write and optionally returning core_distances

a86a809

additional checks

6c8aecc

jinsolp self-requested a review August 27, 2025 16:53

jinsolp reviewed Aug 27, 2025

View reviewed changes

viclafargue added 3 commits August 28, 2025 16:10

adressing review

7ee9f45

Making single GPU the default

71971e2

document multi-gpu

ff968f5

Intron7 reviewed Aug 28, 2025

View reviewed changes

Comment thread python/cuvs/cuvs/neighbors/all_neighbors/all_neighbors.pyx

viclafargue added 2 commits August 29, 2025 11:49

additional doc

e4f00ad

Merge branch 'branch-25.10' into all-neighbors-python-bindings

312519e

viclafargue requested a review from a team as a code owner August 29, 2025 11:03

cjnolet reviewed Aug 29, 2025

View reviewed changes

Comment thread cpp/include/cuvs/core/c_api.h Outdated

cjnolet reviewed Aug 29, 2025

View reviewed changes

Comment thread cpp/include/cuvs/neighbors/all_neighbors.h Outdated

cjnolet reviewed Aug 29, 2025

View reviewed changes

Comment thread cpp/src/core/c_api.cpp Outdated

cjnolet reviewed Aug 29, 2025

View reviewed changes

Comment thread python/cuvs/cuvs/neighbors/all_neighbors/all_neighbors.pyx Outdated

viclafargue added 2 commits September 2, 2025 15:01

Merge branch 'branch-25.10' into all-neighbors-python-bindings

2864baa

Answering review

a027467

cjnolet assigned viclafargue Sep 5, 2025

cjnolet moved this from Todo to In Progress in Unstructured Data Processing Sep 5, 2025

viclafargue added 2 commits September 5, 2025 17:57

Merge branch 'branch-25.10' into all-neighbors-python-bindings

b24f757

Fix doc for single GPU CI

5986273

cjnolet reviewed Sep 16, 2025

View reviewed changes

jinsolp reviewed Sep 16, 2025

View reviewed changes

cjnolet and others added 4 commits September 16, 2025 15:47

Merge branch 'branch-25.10' into all-neighbors-python-bindings

64c454e

Answered reviews

87ecff6

recall threshold update

b361d33

Merge branch 'branch-25.10' into all-neighbors-python-bindings

0eb4186

jinsolp reviewed Sep 17, 2025

View reviewed changes

Comment thread python/cuvs/cuvs/tests/test_all_neighbors.py Outdated

Comment thread docs/source/neighbors/all_neighbors.rst Outdated

Comment thread docs/source/neighbors/all_neighbors.rst Outdated

viclafargue added 3 commits September 18, 2025 14:40

Adding All Neighbors C API

a967350

Updating pytest

d917af8

Merge branch 'branch-25.10' into all-neighbors-python-bindings

2c22e14

viclafargue force-pushed the all-neighbors-python-bindings branch from b70ffd4 to 2c22e14 Compare September 18, 2025 12:55

update doc

9842b10

cjnolet approved these changes Sep 18, 2025

View reviewed changes

jinsolp approved these changes Sep 18, 2025

View reviewed changes

rapids-bot Bot merged commit f33de2a into rapidsai:branch-25.10 Sep 18, 2025
84 checks passed

github-project-automation Bot moved this from In Progress to Done in Unstructured Data Processing Sep 18, 2025

viclafargue mentioned this pull request Feb 13, 2026

[FEA] all neighbors python API #1279

Closed

		from cuvs.common.exceptions import check_cuvs


		cdef class MultiGpuResources:

Conversation

viclafargue commented Aug 26, 2025

Uh oh!

jinsolp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

viclafargue Aug 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cjnolet Aug 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

viclafargue Sep 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jinsolp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jinsolp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cjnolet commented Sep 18, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

viclafargue Aug 28, 2025 •

edited

Loading

cjnolet Aug 29, 2025 •

edited

Loading

viclafargue Sep 17, 2025 •

edited

Loading