Initial implementation of SVS Runtime package

[rfsaliev]

No description provided.

[ahuber21]

Some minor comments. Let's discuss how to tackle todos.

[ahuber21]

I suggest to sort all headers in the order

1. "Local" headers (includes with quotes, same directory)
2. SVS runtime headers
3. SVS core headers
4. deps headers
5. std library, other core headers

Also, in svs/include we use mainly quote includes. IMO we should stay consistent here. Or are there reasons against?

[rfsaliev]

This exact file (dynamic_vamana_index.cpp) implements declarations made in svs/runtime/dynamic_vamana_index.h
So, the rule I followed:

• The declarations header for this implementation file
• 'Local' utility headers
• Standard library headers
• SVS core headers

PS. @ahuber21, do you want your suggestions to be applied in this PR?

[ahuber21]

My suggestion for this particular file would be

#include "dynamic_vamana_index.h" // i.e. quotes - why was this in <> but the two headers below not?
#include "dynamic_vamana_index_impl.h"
#include "svs_runtime_utils.h"

#ifdef SVS_LEANVEC_HEADER
#include "dynamic_vamana_index_leanvec_impl.h"
#endif

// svs core headers
#include <svs/core/data.h>
#include <svs/core/distance.h>
#include <svs/core/query_result.h>
#include <svs/extensions/vamana/scalar.h>
#include <svs/lib/float16.h>
#include <svs/orchestrators/dynamic_vamana.h>
#include <svs/quantization/scalar/scalar.h>

// std library
#include <algorithm>
#include <memory>
#include <variant>
#include <vector>

This introduces a notion of "proximity" - we include the closest headers first and expand our circle until we have everything.

I know it's nitpicking, but I noticed inconsistencies in the code and sometimes it does make a difference. E.g., if we forgot to include <vector> in the SVS core headers, we wouldn't see it when building the current version of dynamic_vamana_index.cpp because <vector> is included before the SVS core headers.

[ahuber21]

Oh, and #include "dynamic_vamana_index.h" should be enabled by defining the correct include paths in cmake. Please correct me if I'm misunderstanding that part.

[rfsaliev]

I think, it is not good idea to mix API and local helper headers.
So, applied the rule:

#include "svs/runtime/API_HEADER"
// space to prevent sorting by clang format 
#include "LOCAL_HELPER_HEADER"
...
// space to prevent sorting by clang format 
#include <svs/CORE_HEADER>
...
// space to prevent sorting by clang format 
#include <STANDARD_HEADER>
...

[ahuber21]

Is padding with -1 specific to Faiss? Would it make sense to accept a parameter to pick a different value for padding? Maybe even skip this entirely?

[rfsaliev]

Good question.
The search() method should somehow 'return' the actual number of results - for example, if k > index.size().
Possible solutions:

1. Pad result buffers with special values: label = max_size_t, distance = infinity
2. One more argument - pointer to a vector of size_t where to store numbers
3. Use ResultsAllocator

[rfsaliev]

PS. I would vote for the option 3. - it will make search signatures consitent.

[rfsaliev]

@ahuber21, can you please respond?
Thank you.

[ahuber21]

Sorry for not replying. Good point about consistency. In its current form, is ResultsAllocator already capable of initializing to arbitrary values?
In other words, can we have a special allocator for Faiss that initializes with inf distance and -1 index?

Would the initialization happen on allocation, or would it only backfill between [found, k-1]? Given that search is typically an expensive operation, I wouldn't expect a big impact from initializing everything first..

[rfsaliev]

It leads some effort to implement ResultsAllocator support.
So, for now, I just applied padding with Unspecify<T>()

[Copilot]

Pull request overview

This PR introduces the initial implementation of the SVS Runtime package, a C++ runtime binding library that provides a versioned, stable API for SVS functionality. The implementation includes support for Vamana and Flat indices with various storage types (FP32, FP16, SQI8, LVQ, LeanVec) and comprehensive testing infrastructure.

Key changes:

• New runtime binding library with versioned API (v0 namespace)
• Support for DynamicVamana and Flat index types with multiple storage kinds
• Complete build and test infrastructure including Docker-based CI/CD workflows
• Integration with clang-format for the new bindings/cpp directory

Reviewed changes

Copilot reviewed 28 out of 29 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
tools/clang-format.sh	Added bindings/cpp to clang-format directories
include/svs/orchestrators/dynamic_vamana.h	Changed loader parameters to rvalue references with perfect forwarding
docker/x86_64/test-cpp-runtime-bindings.sh	New test script for validating runtime bindings against FAISS CI
docker/x86_64/manylinux228/Dockerfile	New Docker image for building runtime bindings in manylinux environment
docker/x86_64/build-cpp-runtime-bindings.sh	Build script for creating runtime binding tarballs
cmake/mkl.cmake	Added static MKL linking support for runtime bindings
bindings/cpp/tests/utils.h	Test utilities for runtime binding tests
bindings/cpp/tests/runtime_test.cpp	Comprehensive test suite for runtime bindings
bindings/cpp/tests/CMakeLists.txt	Test configuration using Catch2 framework
bindings/cpp/src/*.cpp	Implementation files for runtime binding API
bindings/cpp/include/svs/runtime/*.h	Public API headers with versioned namespace
bindings/cpp/CMakeLists.txt	Main build configuration for runtime library
.github/workflows/build-cpp-runtime-bindings.yml	CI workflow for building and testing runtime bindings

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[ethanglaser]

Thanks for cleaning up codefactor but I don't know that all of them should be addressed. At least for individual package versions, while it provides a sense of absolute reproducibility, it also introduces maintenance cost with having to manage dependencies (further multiplied considering we now have several docker files in private repo). Arguably same for the the image, but I think using specific instead of latest there is alright

[ethanglaser]

Mostly just clarification questions and a few minor suggestions. Looks ready for merge.

[ahuber21]

Thanks for cleaning up codefactor but I don't know that all of them should be addressed. At least for individual package versions, while it provides a sense of absolute reproducibility, it also introduces maintenance cost with having to manage dependencies (further multiplied considering we now have several docker files in private repo). Arguably same for the the image, but I think using specific instead of latest there is alright

I disagree. There is no need to always bump to the latest version of packages and images, we can do it when things start to break.
On the flipside, silent upgrades can introduce functional changes that are extremely hard to catch.

And generally these codefactor comments must be addressed. If not by changing the flagged code, then at least by greenlighting it in the configuration.