Skip to content

Python API for CAGRA+HNSW#246

Merged
rapids-bot[bot] merged 26 commits into
NVIDIA:branch-24.10from
divyegala:hnsw-python-api
Oct 3, 2024
Merged

Python API for CAGRA+HNSW#246
rapids-bot[bot] merged 26 commits into
NVIDIA:branch-24.10from
divyegala:hnsw-python-api

Conversation

@divyegala

Copy link
Copy Markdown
Contributor

No description provided.

@divyegala divyegala added feature request New feature or request non-breaking Introduces a non-breaking change labels Jul 24, 2024
@divyegala divyegala self-assigned this Jul 24, 2024
@divyegala divyegala requested review from a team as code owners July 24, 2024 01:13
@divyegala divyegala changed the title Python API for CAGRA+HNSW Python and C API for CAGRA+HNSW Jul 24, 2024
@jameslamb

Copy link
Copy Markdown
Member

@divyegala I observed that same pip devcontainer build failure on an unrelated PR: #247 (comment)

So I suspect it's unrelated to your changes here.

@divyegala

Copy link
Copy Markdown
Contributor Author

@jameslamb I suspect it's related to this PR NVIDIA/raft#2346

What I cannot figure out is why conda-cpp builds pass (new conda packages released?) but wheel builds fail. cuVS wheel builds pick up libraft wheel builds?

@vyasr

vyasr commented Jul 24, 2024

Copy link
Copy Markdown
Contributor

Maybe conda packages weren't out yet when the builds failed? Wheel builds will pull the source of raft to rebuild the static lib (unless you changed something in how cuvs wheels specifically are built, but I don't see any indication of that in the CMake).

@cjnolet cjnolet left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good overall. Mostly a couple small documentation-related things to polish this off and then I think it's ready to merge.

bool include_dataset);

/**
* Save the CAGRA index to file in hnswlib format.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we add a note here that this must be loaded by the (patched) hnswlib wrappers inside cuVS and it can't just be loaded with hnswlib?

Comment thread python/cuvs/cuvs/neighbors/hnsw/hnsw.pyx
@divyegala divyegala changed the title Python and C API for CAGRA+HNSW Python API for CAGRA+HNSW Sep 16, 2024
Comment thread cpp/include/cuvs/neighbors/cagra.h Outdated
Comment thread python/cuvs/cuvs/neighbors/hnsw/hnsw.pyx Outdated
Comment thread python/cuvs/cuvs/neighbors/hnsw/hnsw.pyx
Comment thread python/cuvs/cuvs/neighbors/hnsw/hnsw.pyx Outdated
Comment thread python/cuvs/cuvs/neighbors/hnsw/hnsw.pyx Outdated
Comment thread python/cuvs/cuvs/neighbors/hnsw/hnsw.pyx Outdated

@cjnolet cjnolet left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The implementation is shaping up great. Mostly minor things, but important nonetheless.

Comment thread cpp/include/cuvs/neighbors/hnsw.hpp
Comment thread python/cuvs/cuvs/neighbors/hnsw/hnsw.pyx Outdated
Comment thread python/cuvs/cuvs/neighbors/hnsw/hnsw.pyx
@divyegala divyegala requested a review from a team as a code owner September 27, 2024 21:37
@cjnolet

cjnolet commented Oct 2, 2024

Copy link
Copy Markdown
Contributor

/merge

@rapids-bot rapids-bot Bot merged commit 3c7f117 into NVIDIA:branch-24.10 Oct 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CMake cpp feature request New feature or request non-breaking Introduces a non-breaking change Python

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants