Conversation
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
| #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.
|
|
||
| } // 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?
Add IVF static and dynamic index in cpp runtime