Skip to content

[diskann-garnet] Allow configurable metric types#824

Merged
tiagonapoli merged 13 commits intomainfrom
tiagonapoli/diskann-garnet/allow-different-metric-types
Mar 12, 2026
Merged

[diskann-garnet] Allow configurable metric types#824
tiagonapoli merged 13 commits intomainfrom
tiagonapoli/diskann-garnet/allow-different-metric-types

Conversation

@tiagonapoli
Copy link
Contributor

  • Does this PR have a descriptive title that could go in our release notes?
  • Does this PR add any new dependencies?
  • Does this PR modify any existing APIs?
  • Is the change to the API backwards compatible?
  • Should this result in any changes to our documentation, either updating existing docs or adding new ones?

What does this implement/fix? Briefly explain your changes.

Allow Garnet to configure the Metric type to be used by DiskANN

Any other comments?

Ported from fork branch tiagonapoli/diskann-garnet/metric-types.
Adds metric type support to the diskann-garnet FFI layer, including
tests for L2, cosine, and inner-product metrics, and invalid metric
type handling.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@tiagonapoli tiagonapoli changed the title Wire up metric types [diskann-garnet] Allow configurable metric types Mar 11, 2026
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR makes the Garnet DiskANN FFI/index creation support selectable distance metrics (instead of always using cosine), and updates tests/docs accordingly.

Changes:

  • Add a metric_type parameter to the create_index FFI and thread it through GarnetProvider so distance/query computers use the requested metric.
  • Fix test store multi-read parsing (pos advancement) and update affected expectations.
  • Expand FFI test coverage for valid/invalid metric values and add a grid search test using variable-length (string) external IDs; update FFI design docs.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
diskann-garnet/src/test_utils.rs Fixes multi-read buffer parsing and adjusts tests to match correct behavior; updates provider construction for new metric parameter.
diskann-garnet/src/provider.rs Stores selected metric on GarnetProvider and uses it when building distance/query computers.
diskann-garnet/src/lib.rs Extends create_index FFI to accept a raw metric_type and passes it into provider construction.
diskann-garnet/src/ffi_tests.rs Adds tests for metric validation and adds a string-ID grid search test; updates existing calls for new FFI signature.
diskann-garnet/docs/ffi-design.rs Documents the new metric_type argument and adds the repository license header.
Comments suppressed due to low confidence (2)

diskann-garnet/src/lib.rs:223

  • create_index now accepts metric_type, but the config is still built with config::PruneKind::TriangleInequality. For Metric::InnerProduct, DiskANN expects occluding-style pruning; using the triangle-inequality prune kind can produce incorrect/low-quality graphs for that metric. Consider using config::PruneKind::from_metric(metric_type) (or equivalent match) when building the config.
        target_degree,
        config::MaxDegree::Value(max_degree as usize),
        l_build as usize,
        config::PruneKind::TriangleInequality,
    )

diskann-garnet/docs/ffi-design.rs:37

  • The docs signature for create_index is still missing the rmw_callback parameter that exists in the actual exported function (diskann-garnet/src/lib.rs). Since this doc block was updated for metric_type, it should be brought back in sync with the real FFI signature.
extern "C" fn create_index(
    context: u64,
    dimensions: u32,
    reduce_dims: u32,
    quant_type: SomeCStyleEnumeration,
    metric_type: i32,
    build_exploration_factor: u32,
    num_links: u32,
    read_callback: unsafe extern "C" fn(u64, *const u8, usize, *mut u8, usize) -> i32,
    write_callback: unsafe extern "C" fn(u64, *const u8, usize, *const u8, usize) -> bool,
    delete_callback: unsafe extern "C" fn(u64, *const u8, usize) -> bool,
) -> *mut c_void;

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@tiagonapoli tiagonapoli requested a review from metajack March 11, 2026 18:59
Tiago Napoli added 4 commits March 11, 2026 17:16
Signed-off-by: Tiago Napoli <tiagonapoli@microsoft.com>
nit
Signed-off-by: Tiago Napoli <tiagonapoli@microsoft.com>
nit
Signed-off-by: Tiago Napoli <tiagonapoli@microsoft.com>
@tiagonapoli tiagonapoli force-pushed the tiagonapoli/diskann-garnet/allow-different-metric-types branch from 2c7819d to ff888e3 Compare March 12, 2026 17:26
Tiago Napoli added 2 commits March 12, 2026 10:31
fmt
Signed-off-by: Tiago Napoli <tiagonapoli@microsoft.com>
Signed-off-by: Tiago Napoli <tiagonapoli@microsoft.com>
@codecov-commenter
Copy link

codecov-commenter commented Mar 12, 2026

Codecov Report

❌ Patch coverage is 91.66667% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.11%. Comparing base (5e0e49d) to head (ab3e479).
⚠️ Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
diskann-vector/src/distance/metric.rs 89.28% 3 Missing ⚠️
diskann-garnet/src/lib.rs 87.50% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #824      +/-   ##
==========================================
+ Coverage   88.96%   89.11%   +0.14%     
==========================================
  Files         442      443       +1     
  Lines       81906    83354    +1448     
==========================================
+ Hits        72868    74281    +1413     
- Misses       9038     9073      +35     
Flag Coverage Δ
miri 89.11% <91.66%> (+0.14%) ⬆️
unittests 88.95% <91.66%> (+0.13%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
diskann-garnet/src/provider.rs 80.83% <100.00%> (+0.17%) ⬆️
diskann-garnet/src/test_utils.rs 99.31% <100.00%> (+<0.01%) ⬆️
diskann-garnet/src/lib.rs 67.13% <87.50%> (+0.30%) ⬆️
diskann-vector/src/distance/metric.rs 90.16% <89.28%> (-0.75%) ⬇️

... and 37 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Changed insert_f32_vector, insert_f32_vector_str, and do_search from
unsafe fn to safe fn with internal unsafe blocks. Removed unnecessary
unsafe wrappers at all call sites, keeping only drop_index in unsafe.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@tiagonapoli tiagonapoli force-pushed the tiagonapoli/diskann-garnet/allow-different-metric-types branch from 8d9b25a to 15ea3c8 Compare March 12, 2026 18:25
Remove coordinate suffix from grid vector IDs, changing from
'grid_vector_00000001_dim3_at_0_0_0' to 'grid_vector_00000001_dim3'.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Tiago Napoli and others added 4 commits March 12, 2026 11:36
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Move grid/circle recall helpers and 7 test functions from ffi_tests.rs
into a dedicated ffi_recall_tests.rs module for better organization.

Includes prior uncommitted refactoring:
- Epsilon-based distance bucketing for float-safe comparisons
- Unified brute_force_knn with distance_fn parameter
- Common run_recall function for grid and circle tests
- Named variables for search and index creation parameters
- parse_string_ids buffer consumption assertion
- Imperative loops throughout

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace EPSILON division (/ 0.001) with BUCKET_SCALE multiplication (* 1000.0)
for more precise and faster floating-point bucketing.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Use diskann_vector::distance::{SquaredL2, Cosine} via PureDistanceFunction
trait instead of custom squared_l2 and cosine_distance helper functions.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@tiagonapoli tiagonapoli merged commit 1b6ab6b into main Mar 12, 2026
24 checks passed
@tiagonapoli tiagonapoli deleted the tiagonapoli/diskann-garnet/allow-different-metric-types branch March 12, 2026 22:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants