-
Notifications
You must be signed in to change notification settings - Fork 16
Fix SVS factory issue on non-SVS and non-LVQ platforms [MOD-9413] #664
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
base: main
Are you sure you want to change the base?
Conversation
f21b91e
to
7ddcc11
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #664 +/- ##
=======================================
Coverage 96.33% 96.34%
=======================================
Files 112 112
Lines 6225 6239 +14
=======================================
+ Hits 5997 6011 +14
Misses 228 228 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
d842254
to
cbde213
Compare
cbde213
to
1eb1cbf
Compare
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.
Looks good, few questions and requests for documentation
// If the quantization mode is not supported, we fallback to non-quantized mode | ||
auto fallBack = supported ? quant_bits : VecSimSvsQuant_NONE; | ||
|
||
// And always return true, as far as non-quantized mode is always supported |
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.
Please comment that this is temporary, and mark in TODO that the actual fallback should be the basic quantisation if check_cpuid() == false && quant_bits != VecSimSvsQuant_NONE
once it is available in svs
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.
Added comments
@@ -169,3 +180,11 @@ size_t EstimateInitialSize(const SVSParams *params, bool is_normalized) { | |||
} | |||
|
|||
} // namespace SVSFactory | |||
|
|||
#else // HAVE_SVS |
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.
Please comment that this is temporary, and we should never get here - once svs public repo will allow building for all platforms and we address platforms with GCC < 11
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.
Added comment
|
||
#define ASSERT_INDEX(index) \ | ||
if (index == nullptr) { \ | ||
if (std::get<1>(svs_details::isSVSQuantBitsSupported(TypeParam::get_quant_bits()))) { \ |
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.
std::get<1>(svs_details::isSVSQuantBitsSupported(TypeParam::get_quant_bits()))
always returns true currenly right? Can you explain the purpose of this assertion (now and in the future)?
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.
Added comment
472465b
to
6d567ba
Compare
Address issue with non-SVS and non-LVQ platforms
Which issues this PR fixes
Goal:
Ensure that the index creation process fails gracefully on unsupported platforms, avoiding any runtime crashes due to incompatible hardware or toolchain.
Currently, the index factory allows creating an LVQ index even on unsupported platforms. However, SVS performs a runtime CPU check, and if the platform doesn’t meet the requirements, it calls exit(1), abruptly terminating the process. This behavior should be avoided by preventing unsupported configurations earlier in the flow.
Proposed Solution:
Different build paths:
a. What happens if a user tries to create an index on a platform that doesn’t meet the build requirements (GCC version, non X86, non intel…))
b. What happens if the binary runs on a CPU that passes the build but fails the runtime check (e.g. AMD processors)?
Implemented Solution:
1. SVS Index Factory returns NULL in following cases:- Platform does not meet SVS build requirements (macos, arm)
- Platform meets SVS build requirements but does not meet runtime requirements (LVQ on non-Intel CPU)
Main objects this PR modified
This pull request refactors and enhances the integration of Scalable Vector Search (SVS) within the project. The changes include simplifying build configurations, improving SVS feature support checks, and enhancing test coverage. The most notable updates ensure better compatibility and maintainability by removing conditional compilation in certain areas and adding runtime checks for SVS capabilities.
Build System Changes:
svs_factory_file
variable incmake/svs.cmake
and directly referencedindex_factories/svs_factory.cpp
in the build configuration (src/VecSim/CMakeLists.txt
). This simplifies the build process by removing unnecessary indirection. [1] [2]SVS Feature Support Enhancements:
check_cpuid
function andisSVSQuantBitsSupported
utility insrc/VecSim/algorithms/svs/svs_utils.h
to verify CPU compatibility and supported quantization modes at runtime. This replaces compile-time checks for SVS features. [1] [2]svs_factory.cpp
to useisSVSQuantBitsSupported
for runtime validation of quantization modes and fallback handling. [1] [2]Codebase Simplification:
#if HAVE_SVS
) inindex_factory.cpp
to streamline the code and ensure SVS-related logic is always compiled, with runtime checks determining availability. [1] [2] [3] [4]Testing Improvements:
test_svs
executable intests/unit/CMakeLists.txt
without conditional compilation, ensuring tests are always built. [1] [2] [3]test_svs.cpp
to use runtime checks (ASSERT_INDEX
) for SVS feature support, replacing compile-time checks and skipping tests gracefully when unsupported. [1] [2] [3] [4]Other Changes:
svs_utils.h
to ensure compatibility and avoid potential build issues.Mark if applicable