Skip to content

Add CAGRA build heuristics independent of HNSW parameters#2345

Open
achirkin wants to merge 5 commits into
NVIDIA:release/26.08from
achirkin:fea-cagra-default-heuristics
Open

Add CAGRA build heuristics independent of HNSW parameters#2345
achirkin wants to merge 5 commits into
NVIDIA:release/26.08from
achirkin:fea-cagra-default-heuristics

Conversation

@achirkin

Copy link
Copy Markdown
Contributor

Split the cagra::index_params::from_hnsw_params into a core cagra::index_params::from_dataset and a thin wrapper ``cagra::index_params::from_hnsw_params`. This solves two problems:

  1. Avoid using the HNSW parameters that do not make sense for CAGRA
  2. Allow HNSW parameter heuristics change without affecting CAGRA in future

The new parameter defaults are selected to yield exactly the same configurations at old call sites of from_hnsw_params

@tfeher tfeher 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.

Thanks Artem, it makes sense to separate the dataset shape dependent CAGRA heuristics to general CAGRA heuristic and HNSW specific ones. The PR looks good to me, apart from two small docstring issues.

Comment thread cpp/include/cuvs/neighbors/cagra.hpp
Comment thread cpp/include/cuvs/neighbors/cagra.hpp
@achirkin
achirkin requested a review from tfeher July 22, 2026 15:17

@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.

This is a relatively small PR in comparison to the dataset changes (that we absolutely need merged for this release).

If we keep merging (lower priority) changes that depend on the current dataset APIs, we aren't going to be able to merge the datasets api changes in time for the release.

Let's at least wait until @HowardHuang1's PR is merged for this one. Given how close it is to the release, I'd almost suggest we proactively push this to 26.10.

@achirkin

Copy link
Copy Markdown
Contributor Author

@cjnolet I understand, this PR surely can wait.
FIY, it shouldn't overlap with the dataset API PR in any way, it only adds two functions that consume extents (not an mdspan, mdarray, or a new dataset type): caga::index_params::from_dataset and cagra::index_params::graph_build_heuristic.

@cjnolet

cjnolet commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Thanks @achirkin. I see now, this is expecting the shape information individually, instead of through an mdspan/dataset.

@imotov if not too much trouble, can you merge this in your heuristic PR and work from that? I agree with Artem here that the change seems not too terrible on the Java side. Will help us streamline a bit more.

imotov added a commit to imotov/cuvs-lucene that referenced this pull request Jul 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature request New feature or request non-breaking Introduces a non-breaking change

Projects

Status: In progress

Development

Successfully merging this pull request may close these issues.

3 participants