Skip to content

[SVS] Add SVS LeanVec compression support #684

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.

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

Merged
merged 8 commits into from
Jun 9, 2025
Merged

Conversation

rfsaliev
Copy link
Collaborator

@rfsaliev rfsaliev commented May 26, 2025

Describe the changes in the pull request

This PR introduces SVS LeanVec index compression integration to VecSim SVSIndex.

Which issues this PR fixes

  1. No issues

Main objects this PR modified

  1. Added SVS LeanVec compression support:
    • New template parameter IsLeanVec for SVSIndex class in svs.h
    • New SVSStorageTraits specialization for IsLeanVec=true in svs_extensions.h
    • SVSStorageTraits interface extended by compute_distance_by_id() used in SVSIndex::getDistanceFrom_Unsafe()
    • Added new LeanVec options to VecSimSvsQuantBits enum in vec_sim_common.h
    • Reflect new LeanVec options in svs_factory.h
    • Added new test to test_svs.cpp which constructs SVSIndex with different kind of compression/quantization.

Mark if applicable

  • This PR introduces API changes
  • This PR introduces serialization changes

@rfsaliev rfsaliev requested review from alonre24 and meiravgri May 26, 2025 13:36
Copy link

codecov bot commented May 26, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.52%. Comparing base (552b295) to head (493d732).
Report is 6 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #684      +/-   ##
==========================================
+ Coverage   96.24%   96.52%   +0.28%     
==========================================
  Files         112      111       -1     
  Lines        6278     6325      +47     
==========================================
+ Hits         6042     6105      +63     
+ Misses        236      220      -16     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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

@DvirDukhan DvirDukhan requested a review from Copilot June 4, 2025 13:14
Copy link
Contributor

@Copilot 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 integrates SVS LeanVec index compression into the VecSim SVSIndex by extending the template parameters and storage traits, updating factory functions, and adding corresponding tests.

  • Introduces IsLeanVec template parameter and specializations in SVSIndex, SVSStorageTraits, and factory helpers.
  • Adds new LeanVec quantization modes (4x8_LeanVec, 8x8_LeanVec) to the VecSimSvsQuantBits enum and updates CMake to fetch a matching SVS release.
  • Expands unit tests to cover size estimation and search behavior for the new LeanVec modes.

Reviewed Changes

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

Show a summary per file
File Description
tests/unit/test_svs.cpp Added tests for LeanVec quantization modes; adjusted size tests
src/VecSim/vec_sim_common.h Updated VecSimSvsQuantBits enum to include LeanVec options
src/VecSim/index_factories/svs_factory.cpp Extended template signatures and switch cases for LeanVec
src/VecSim/algorithms/svs/svs_utils.h Added generic compute_distance_by_id to primary traits
src/VecSim/algorithms/svs/svs_extensions.h Included LeanVec header and added specializations for LeanVec
src/VecSim/algorithms/svs/svs.h Added IsLeanVec to SVSIndex and delegated distance logic
cmake/svs.cmake Bumped SVS_URL to a newer nightly release
Comments suppressed due to low confidence (3)

src/VecSim/vec_sim_common.h:195

  • EstimateInitialSize only distinguishes quantBits == 0 vs. nonzero quant and always uses the non-LeanVec template. For LeanVec modes, the base object size differs and should be accounted for by branching on IsLeanVec or inspecting params->quantBits for LeanVec flags.
est += (params->quantBits == 0)
               ? sizeof(SVSIndex<svs::distance::DistanceL2, float, 0, 0, false>)
               : sizeof(SVSIndex<svs::distance::DistanceL2, float, 8, 0, false>);

tests/unit/test_svs.cpp:2190

  • The test uses the original params.quantBits for size estimation but the code may fallback unsupported modes via isSVSQuantBitsSupported. You should retrieve the fallback mode (std::get<0>(isSVSQuantBitsSupported(params.quantBits))) and use that value when calling EstimateElementSize, to keep the test aligned with the index creation.
double estimation_accuracy = (quant_bits != VecSimSvsQuant_NONE) ? 0.11 : 0.01;

tests/unit/test_svs.cpp:2172

  • Using std::vector<float[dim]> is not valid C++; array types cannot be direct vector element types. Consider using std::vector<std::array<float, dim>> or std::vector<std::vector<float>> to store fixed-size vectors.
std::vector<float[dim]> v(n);

Comment on lines 139 to 142
VecSimSvsQuant_4x4 = 4 | (4 << 8), // 4-bit quantization with 4-bit residuals
VecSimSvsQuant_4x8 = 4 | (8 << 8), // 4-bit quantization with 8-bit residuals
VecSimSvsQuant_4x8_LeanVec = 4 | (8 << 8) | (1 << 16), // LeanVec 4x4 quantization
VecSimSvsQuant_8x8_LeanVec = 8 | (8 << 8) | (1 << 16), // LeanVec 8x8 quantization
Copy link
Preview

Copilot AI Jun 4, 2025

Choose a reason for hiding this comment

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

The shift amount for packing residual bits was changed from << 10 to << 8, altering the numeric values of existing enum members. This could break backward compatibility or bit-field parsing. Consider restoring the original shift or documenting the new bit layout explicitly.

Suggested change
VecSimSvsQuant_4x4 = 4 | (4 << 8), // 4-bit quantization with 4-bit residuals
VecSimSvsQuant_4x8 = 4 | (8 << 8), // 4-bit quantization with 8-bit residuals
VecSimSvsQuant_4x8_LeanVec = 4 | (8 << 8) | (1 << 16), // LeanVec 4x4 quantization
VecSimSvsQuant_8x8_LeanVec = 8 | (8 << 8) | (1 << 16), // LeanVec 8x8 quantization
VecSimSvsQuant_4x4 = 4 | (4 << 10), // 4-bit quantization with 4-bit residuals
VecSimSvsQuant_4x8 = 4 | (8 << 10), // 4-bit quantization with 8-bit residuals
VecSimSvsQuant_4x8_LeanVec = 4 | (8 << 10) | (1 << 16), // LeanVec 4x4 quantization
VecSimSvsQuant_8x8_LeanVec = 8 | (8 << 10) | (1 << 16), // LeanVec 8x8 quantization

Copilot uses AI. Check for mistakes.

Copy link
Collaborator Author

@rfsaliev rfsaliev Jun 5, 2025

Choose a reason for hiding this comment

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

I tried to encode quantization mode in a kind of bitmask. Had thoughts that such encoding would be useful for further processing. I also tried to keep enough bits in the bitmask to support up to 32-bit quantization.
But current code does not use such masks and limited quantization modes are supported by pre-compiled SVS library.
Instead, in tests I see that LeanVec case is not readable. E.g.:

1452/1478 Test #1454: SVSTest.svs_empty_index<SVSIndexType<(VecSimType)0,float,(VecSimSvsQuantBits)67592>> ...................................... Passed 0.01 sec

IMHO, at this moment, we can set any values for these constants and we can make them more readable. E.g.:

typedef enum {
    VecSimSvsQuant_NONE = 0,           // No quantization.
    VecSimSvsQuant_4 = 4,              // 4-bit quantization
    VecSimSvsQuant_8 = 8,              // 8-bit quantization
    VecSimSvsQuant_4x4 = 404, // 4-bit quantization with 4-bit residuals
    VecSimSvsQuant_4x8 = 408, // 4-bit quantization with 8-bit residuals
    VecSimSvsQuant_4x8_LeanVec = 10408, // LeanVec 4x8 quantization
    VecSimSvsQuant_8x8_LeanVec = 10808, // LeanVec 8x8 quantization
    // Further scalar fallback quantization:
    VecSimSvsQuant_Scalar = 20008 // 8-bit scalar quantization
} VecSimSvsQuantBits;

@DvirDukhan , @alonre24 , what do you think about?

@alonre24 alonre24 requested review from dor-forer and removed request for meiravgri June 5, 2025 09:30
@dor-forer
Copy link
Collaborator

@rfsaliev Can we merge this?

@rfsaliev rfsaliev added this pull request to the merge queue Jun 9, 2025
Merged via the queue into main with commit b0d4bf6 Jun 9, 2025
17 checks passed
@rfsaliev rfsaliev deleted the rfsaliev/svs-leanvec branch June 9, 2025 16:55
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.

2 participants