Conversation
ahuber21
left a comment
ahuber21
left a comment
There was a problem hiding this comment.
Very nice! I have just a few nitpicks that would be easy to sort out.
bindings/cpp/src/ivf_index_impl.h
Outdated
|
|
||
| // Dispatch on storage kind for Dynamic IVF operations (uses blocked allocator) | ||
| template <typename F, typename... Args> | ||
| auto dispatch_ivf_blocked_storage_kind(StorageKind kind, F&& f, Args&&... args) { |
There was a problem hiding this comment.
I don't see this used anywhere. Will a blocked index be added separately?
bindings/cpp/tests/runtime_test.cpp
Outdated
| CATCH_REQUIRE(status.ok()); | ||
|
|
||
| status = index->compact(); | ||
| CATCH_REQUIRE(status.ok()); |
There was a problem hiding this comment.
Maybe also check for smaller size after compacting?
There was a problem hiding this comment.
We have compact related unit tests in the library
There was a problem hiding this comment.
This entire file would benefit from a few svs::runtime::v0::... statements for better readability.
There was a problem hiding this comment.
I am just following the existing structure, this could be a separate PR for better readability in vamana/ivf tests
ibhati
requested review from
ethanglaser,
homksei,
mihaic and
yuejiaointel
as code owners
| #include <svs/orchestrators/dynamic_vamana.h> | ||
| #include <svs/quantization/scalar/scalar.h> | ||
|
|
||
| #ifdef SVS_LVQ_HEADER |
There was a problem hiding this comment.
Definitions SVS_LVQ_HEADER and SVS_LEANVEC_HEADER used to allow directly include LVQ and LeanVec internal implementation headers in case of internal builds and allowed to apply compile-time optimizations in that case.
The proposed change removes possibility for compile-time optimizations.
There was a problem hiding this comment.
Yeah, the SVS_*_HEADER logic was getting pretty complex and verbose as I added the new headers and extensions for IVF‑LVQ and LeanVec. I updated the CMakeLists.txt in the internal repository to ensure those definitions are correctly propagated when building the runtime from source.
There was a problem hiding this comment.
I have put back the header files to apply the compile-time optimizations
|
|
||
| } // namespace | ||
|
|
||
| // Template function to write and read an index |
There was a problem hiding this comment.
Seems like removing this comment is the only change in this file.
Do we still need this file changes?
| target_compile_definitions(${TARGET_NAME} PRIVATE | ||
| SVS_LEANVEC_HEADER="${SVS_LEANVEC_HEADER}" | ||
| ) | ||
| endif() |
There was a problem hiding this comment.
I am afraid, public build with LVQ/LeanVec support (lines 122-147) is broken by these changes.
There was a problem hiding this comment.
Do you mean public build with shared library and LVQ/Leanvec enabled? Can you check this this CI flow https://github.com/intel/ScalableVectorSearch/actions/runs/22333378169/job/64620480018?pr=267
CI seems to be working. Please elaborate why it's broken?
3 participants
Add IVF static and dynamic index in cpp runtime