Skip to content

Commit b22e5c2

Browse files
rfsalievrfsaliev
authored
Fix SVS factory issue on non-SVS and non-LVQ platforms [MOD-9413] (#664)
* Return NULL Index if SVS LVQ is requested but not supported. * Add tests for non-supported SVS cases. * Fix SVS LVQ mode support detection. * Simplify ASSERT_INDEX() macro in svs_test * Enable quantization fallback to non-quantized mode * Added comments according to code review requests.
1 parent 1f26bf1 commit b22e5c2

File tree

7 files changed

+156
-45
lines changed

7 files changed

+156
-45
lines changed

‎cmake/svs.cmake

-1
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,6 @@ if(USE_SVS)
3838
message(STATUS "SVS support enabled")
3939
# Configure SVS build
4040
add_compile_definitions("HAVE_SVS=1")
41-
set(svs_factory_file "index_factories/svs_factory.cpp")
4241

4342
# detect if build environment is using glibc
4443
include(CheckSymbolExists)

‎src/VecSim/CMakeLists.txt

+1-1
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ add_library(VectorSimilarity ${VECSIM_LIBTYPE}
2929
index_factories/brute_force_factory.cpp
3030
index_factories/hnsw_factory.cpp
3131
index_factories/tiered_factory.cpp
32-
${svs_factory_file}
32+
index_factories/svs_factory.cpp
3333
index_factories/index_factory.cpp
3434
index_factories/components/components_factory.cpp
3535
algorithms/hnsw/visited_nodes_handler.cpp

‎src/VecSim/algorithms/svs/svs_utils.h

+43
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,12 @@
1515
#include "svs/lib/float16.h"
1616
#include "svs/index/vamana/dynamic_index.h"
1717

18+
#include <cpuid.h>
19+
#include <cstdint>
20+
#include <cstdlib>
21+
#include <string>
22+
#include <utility>
23+
1824
namespace svs_details {
1925
// VecSim->SVS data type conversion
2026
template <typename T>
@@ -142,6 +148,43 @@ inline svs::lib::PowerOfTwo SVSBlockSize(size_t bs, size_t elem_size) {
142148
return svs_bs;
143149
}
144150

151+
// clang-format off
152+
inline bool check_cpuid() {
153+
uint32_t eax, ebx, ecx, edx;
154+
__cpuid(0, eax, ebx, ecx, edx);
155+
std::string vendor_id = std::string((const char*)&ebx, 4) +
156+
std::string((const char*)&edx, 4) +
157+
std::string((const char*)&ecx, 4);
158+
return (vendor_id == "GenuineIntel");
159+
}
160+
// clang-format on
161+
162+
// Check if the SVS implementation supports Quantization mode
163+
// @param quant_bits requested SVS quantization mode
164+
// @return pair<fallbackMode, bool>
165+
// @note even if VecSimSvsQuantBits is a simple enum value,
166+
// in theory, it can be a complex type with a combination of modes:
167+
// - primary bits, secondary/residual bits, dimesionality reduction, etc.
168+
// which can be incompatible to each-other.
169+
inline std::pair<VecSimSvsQuantBits, bool> isSVSQuantBitsSupported(VecSimSvsQuantBits quant_bits) {
170+
// If HAVE_SVS_LVQ is not defined, we don't support any quantization mode
171+
// else we check if the CPU supports SVS LVQ
172+
bool supported = quant_bits == VecSimSvsQuant_NONE
173+
#if HAVE_SVS_LVQ
174+
|| check_cpuid() // Check if the CPU supports SVS LVQ
175+
#endif
176+
;
177+
178+
// If the quantization mode is not supported, we fallback to non-quantized mode
179+
// - this is temporary solution until we have a basic quantization mode in SVS
180+
// TODO: use basic SVS quantization as a fallback for unsupported modes
181+
auto fallBack = supported ? quant_bits : VecSimSvsQuant_NONE;
182+
183+
// And always return true, as far as fallback mode is always supported
184+
// Upon further decision changes, some cases should treated as not-supported
185+
// So we will need return false.
186+
return std::make_pair(fallBack, true);
187+
}
145188
} // namespace svs_details
146189

147190
template <typename DataType, size_t QuantBits, size_t ResidualBits, class Enable = void>

‎src/VecSim/index_factories/index_factory.cpp

-8
Original file line numberDiff line numberDiff line change
@@ -11,9 +11,7 @@
1111
#include "hnsw_factory.h"
1212
#include "brute_force_factory.h"
1313
#include "tiered_factory.h"
14-
#if HAVE_SVS
1514
#include "svs_factory.h"
16-
#endif
1715

1816
namespace VecSimFactory {
1917
VecSimIndex *NewIndex(const VecSimParams *params) {
@@ -35,9 +33,7 @@ VecSimIndex *NewIndex(const VecSimParams *params) {
3533
break;
3634
}
3735
case VecSimAlgo_SVS: {
38-
#if HAVE_SVS
3936
index = SVSFactory::NewIndex(params);
40-
#endif
4137
break;
4238
}
4339
}
@@ -56,9 +52,7 @@ size_t EstimateInitialSize(const VecSimParams *params) {
5652
case VecSimAlgo_TIERED:
5753
return TieredFactory::EstimateInitialSize(&params->algoParams.tieredParams);
5854
case VecSimAlgo_SVS:; // empty statement if svs not available
59-
#if HAVE_SVS
6055
return SVSFactory::EstimateInitialSize(&params->algoParams.svsParams);
61-
#endif
6256
}
6357
return -1;
6458
}
@@ -72,9 +66,7 @@ size_t EstimateElementSize(const VecSimParams *params) {
7266
case VecSimAlgo_TIERED:
7367
return TieredFactory::EstimateElementSize(&params->algoParams.tieredParams);
7468
case VecSimAlgo_SVS:; // empty statement if svs not available
75-
#if HAVE_SVS
7669
return SVSFactory::EstimateElementSize(&params->algoParams.svsParams);
77-
#endif
7870
}
7971
return -1;
8072
}

‎src/VecSim/index_factories/svs_factory.cpp

+24-2
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,8 @@
88
*/
99

1010
#include "VecSim/index_factories/svs_factory.h"
11+
12+
#if HAVE_SVS
1113
#include "VecSim/memory/vecsim_malloc.h"
1214
#include "VecSim/vec_sim_index.h"
1315
#include "VecSim/algorithms/svs/svs.h"
@@ -44,7 +46,12 @@ VecSimIndex *NewIndexImpl(const VecSimParams *params, bool is_normalized) {
4446

4547
template <typename MetricType, typename DataType>
4648
VecSimIndex *NewIndexImpl(const VecSimParams *params, bool is_normalized) {
47-
switch (params->algoParams.svsParams.quantBits) {
49+
// Ignore the 'supported' flag because we always fallback at least to the non-quantized mode
50+
// elsewhere we got code coverage failure for the `supported==false` case
51+
auto quantBits =
52+
std::get<0>(svs_details::isSVSQuantBitsSupported(params->algoParams.svsParams.quantBits));
53+
54+
switch (quantBits) {
4855
case VecSimSvsQuant_NONE:
4956
return NewIndexImpl<MetricType, DataType, 0>(params, is_normalized);
5057
case VecSimSvsQuant_8:
@@ -100,7 +107,11 @@ constexpr size_t QuantizedVectorSize(size_t dims, size_t alignment = 0) {
100107

101108
template <typename DataType>
102109
size_t QuantizedVectorSize(VecSimSvsQuantBits quant_bits, size_t dims, size_t alignment = 0) {
103-
switch (quant_bits) {
110+
// Ignore the 'supported' flag because we always fallback at least to the non-quantized mode
111+
// elsewhere we got code coverage failure for the `supported==false` case
112+
auto quantBits = std::get<0>(svs_details::isSVSQuantBitsSupported(quant_bits));
113+
114+
switch (quantBits) {
104115
case VecSimSvsQuant_NONE:
105116
return QuantizedVectorSize<DataType, 0>(dims, alignment);
106117
case VecSimSvsQuant_8:
@@ -178,3 +189,14 @@ size_t EstimateInitialSize(const SVSParams *params, bool is_normalized) {
178189
}
179190

180191
} // namespace SVSFactory
192+
193+
// This is a temporary solution to avoid breaking the build when SVS is not available
194+
// and to allow the code to compile without SVS support.
195+
// TODO: remove HAVE_SVS when SVS will support all Redis platfoms and compilers
196+
#else // HAVE_SVS
197+
namespace SVSFactory {
198+
VecSimIndex *NewIndex(const VecSimParams *params, bool is_normalized) { return NULL; }
199+
size_t EstimateInitialSize(const SVSParams *params, bool is_normalized) { return -1; }
200+
size_t EstimateElementSize(const SVSParams *params) { return -1; }
201+
}; // namespace SVSFactory
202+
#endif // HAVE_SVS

‎tests/unit/CMakeLists.txt

+3-9
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,7 @@ add_executable(test_bf16 ../utils/mock_thread_pool.cpp test_bf16.cpp unit_test_u
4646
add_executable(test_fp16 ../utils/mock_thread_pool.cpp test_fp16.cpp unit_test_utils.cpp)
4747
add_executable(test_int8 ../utils/mock_thread_pool.cpp test_int8.cpp unit_test_utils.cpp)
4848
add_executable(test_uint8 ../utils/mock_thread_pool.cpp test_uint8.cpp unit_test_utils.cpp)
49+
add_executable(test_svs ../utils/mock_thread_pool.cpp test_svs.cpp unit_test_utils.cpp)
4950

5051
target_link_libraries(test_hnsw PUBLIC gtest_main VectorSimilarity)
5152
target_link_libraries(test_hnsw_parallel PUBLIC gtest_main VectorSimilarity)
@@ -59,11 +60,7 @@ target_link_libraries(test_bf16 PUBLIC gtest_main VectorSimilarity)
5960
target_link_libraries(test_fp16 PUBLIC gtest_main VectorSimilarity)
6061
target_link_libraries(test_int8 PUBLIC gtest_main VectorSimilarity)
6162
target_link_libraries(test_uint8 PUBLIC gtest_main VectorSimilarity)
62-
63-
if(USE_SVS)
64-
add_executable(test_svs ../utils/mock_thread_pool.cpp test_svs.cpp unit_test_utils.cpp)
65-
target_link_libraries(test_svs PUBLIC gtest_main VectorSimilarity)
66-
endif()
63+
target_link_libraries(test_svs PUBLIC gtest_main VectorSimilarity)
6764

6865
include(GoogleTest)
6966

@@ -79,7 +76,4 @@ gtest_discover_tests(test_bf16 TEST_PREFIX BF16UNIT_)
7976
gtest_discover_tests(test_fp16 TEST_PREFIX FP16UNIT_)
8077
gtest_discover_tests(test_int8 TEST_PREFIX INT8UNIT_)
8178
gtest_discover_tests(test_uint8 TEST_PREFIX UINT8UNIT_)
82-
83-
if(USE_SVS)
84-
gtest_discover_tests(test_svs)
85-
endif()
79+
gtest_discover_tests(test_svs)

0 commit comments

Comments
 (0)