-
Notifications
You must be signed in to change notification settings - Fork 21
[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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
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. 🚀 New features to boost your workflow:
|
There was a problem hiding this 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 inSVSIndex
,SVSStorageTraits
, and factory helpers. - Adds new LeanVec quantization modes (
4x8_LeanVec
,8x8_LeanVec
) to theVecSimSvsQuantBits
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 distinguishesquantBits == 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 onIsLeanVec
or inspectingparams->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 viaisSVSQuantBitsSupported
. You should retrieve the fallback mode (std::get<0>(isSVSQuantBitsSupported(params.quantBits))
) and use that value when callingEstimateElementSize
, 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 usingstd::vector<std::array<float, dim>>
orstd::vector<std::vector<float>>
to store fixed-size vectors.
std::vector<float[dim]> v(n);
src/VecSim/vec_sim_common.h
Outdated
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 |
There was a problem hiding this comment.
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.
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.
There was a problem hiding this comment.
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?
@rfsaliev Can we merge this? |
Describe the changes in the pull request
This PR introduces SVS LeanVec index compression integration to VecSim
SVSIndex
.Which issues this PR fixes
Main objects this PR modified
IsLeanVec
forSVSIndex
class in svs.hSVSStorageTraits
specialization forIsLeanVec=true
in svs_extensions.hSVSStorageTraits
interface extended bycompute_distance_by_id()
used inSVSIndex::getDistanceFrom_Unsafe()
VecSimSvsQuantBits
enum in vec_sim_common.htest_svs.cpp
which constructsSVSIndex
with different kind of compression/quantization.Mark if applicable