Skip to content

fix: store data members & make faiss tests pass #209

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.

Sign up for GitHub

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

Open
wants to merge 6 commits into
base: rfsaliev/cpp-runtime-binding
Choose a base branch
from
Open

fix: store data members & make faiss tests pass #209

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a3ecf1f
fix: store data members & make faiss tests pass
ahuber21 Oct 31, 2025
faf3d65
bring back explicit enum vals
ahuber21 Oct 31, 2025
6db834b
fix version comparison
ahuber21 Nov 3, 2025
7af7137
store vamana members
ahuber21 Nov 3, 2025
8d8b99a
wip
ahuber21 Nov 4, 2025
32e882e
Add build_leanvec
ahuber21 Nov 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion bindings/cpp/include/IndexSVSImplDefs.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,8 @@ enum class ErrorCode {
UNKNOWN_ERROR = 1,
INVALID_ARGUMENT = 2,
NOT_IMPLEMENTED = 3,
NOT_INITIALIZED = 4
NOT_INITIALIZED = 4,
IO_ERROR = 5
};

struct Status {
Expand Down
2 changes: 1 addition & 1 deletion bindings/cpp/include/IndexSVSVamanaImpl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ struct SVS_RUNTIME_API IndexSVSVamanaImpl {
virtual void reset() noexcept;

/* Serialization and deserialization helpers */
Status serialize_impl(std::ostream& out) const noexcept;
virtual Status serialize_impl(std::ostream& out) const noexcept;
virtual Status deserialize_impl(std::istream& in) noexcept;

MetricType metric_type_;
Expand Down
1 change: 1 addition & 0 deletions bindings/cpp/include/IndexSVSVamanaLVQImpl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ struct SVS_RUNTIME_API IndexSVSVamanaLVQImpl : IndexSVSVamanaImpl {
static IndexSVSVamanaLVQImpl*
build(size_t dim, MetricType metric, const BuildParams& params, LVQLevel lvq) noexcept;

Status serialize_impl(std::ostream& out) const noexcept override;
Status deserialize_impl(std::istream& in) noexcept override;

protected:
Expand Down
4 changes: 3 additions & 1 deletion bindings/cpp/include/IndexSVSVamanaLeanVecImpl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ struct SVS_RUNTIME_API IndexSVSVamanaLeanVecImpl : IndexSVSVamanaImpl {

Status train(size_t n, const float* x) noexcept;

Status serialize_impl(std::ostream& out) const noexcept override;

Status deserialize_impl(std::istream& in) noexcept override;

bool is_trained() const noexcept { return trained; }
Expand All @@ -64,8 +66,8 @@ struct SVS_RUNTIME_API IndexSVSVamanaLeanVecImpl : IndexSVSVamanaImpl {

size_t leanvec_d;
LeanVecLevel leanvec_level;
std::unique_ptr<svs::leanvec::LeanVecMatrices<std::dynamic_extent>> leanvec_matrix;
bool trained = false;
std::unique_ptr<svs::leanvec::LeanVecMatrices<std::dynamic_extent>> leanvec_matrix;
};

} // namespace runtime
Expand Down
16 changes: 11 additions & 5 deletions bindings/cpp/src/IndexSVSVamanaImpl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -466,12 +466,12 @@ Status IndexSVSVamanaImpl::init_impl(size_t n, const float* x) noexcept {
}

Status IndexSVSVamanaImpl::serialize_impl(std::ostream& out) const noexcept {
if (!impl) {
return Status{
ErrorCode::NOT_INITIALIZED, "Cannot serialize: SVS index not initialized."};
}
bool initialized = impl != nullptr;
Copy link
Member

@rfsaliev rfsaliev Oct 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please explain, why we have to serializae/deserialize empty inidices?

Copy link
Contributor Author

@ahuber21 ahuber21 Oct 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the use cases in this test
https://github.com/ahuber21/faiss/blob/b709fa114afc522b3d10ffd1356df1d9a9548951/tests/test_svs.cpp#L111

which is created to validate the scenario in this issue
ahuber21/faiss#37

Copy link
Member

@rfsaliev rfsaliev Nov 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @ahuber21 for the problem description
After some time of investigation ana analysis, I got understanding, that inside the implementation code in SVS side we do not need to call DynamicVamana::save(std::ostream&) anymore, instead we can use filesystem-based DynamicVamana::save(std::filesystem::path, ...) method which allows to manage serialization in a more controlled and simpler way.

out.write(reinterpret_cast<const char*>(&initialized), sizeof(bool));

impl->save(out);
if (initialized) {
impl->save(out);
}
return Status_Ok;
}

Expand All @@ -482,6 +482,12 @@ Status IndexSVSVamanaImpl::deserialize_impl(std::istream& in) noexcept {
"Cannot deserialize: SVS index already initialized."};
}

bool initialized = false;
in.read(reinterpret_cast<char*>(&initialized), sizeof(bool));
if (!initialized) {
return Status_Ok;
}

impl.reset(std::visit(
[&](auto element) {
using ElementType = std::decay_t<decltype(element)>;
Expand Down
18 changes: 18 additions & 0 deletions bindings/cpp/src/IndexSVSVamanaLVQImpl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,13 +171,31 @@ Status IndexSVSVamanaLVQImpl::init_impl(size_t n, const float* x) noexcept {
);
}

Status IndexSVSVamanaLVQImpl::serialize_impl(std::ostream& out) const noexcept {
// Also store LVQ specific members
out.write(reinterpret_cast<const char*>(&lvq_level), sizeof(LVQLevel));
Copy link
Member

@rfsaliev rfsaliev Oct 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So far, to make everything consistent, shouldn't we save IndexSVSVamanaImpl members as well?

Copy link
Contributor Author

@ahuber21 ahuber21 Oct 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't there only ntotal_soft_deleted which is always 0 because we compact on save?

Yes, I guess it makes sense to add the public members too.


// This will also write whether or not we're initialized
return IndexSVSVamanaImpl::serialize_impl(out);

return Status_Ok;
}

Status IndexSVSVamanaLVQImpl::deserialize_impl(std::istream& in) noexcept {
if (impl) {
return Status{
ErrorCode::INVALID_ARGUMENT,
"Cannot deserialize: SVS index already initialized."};
}

in.read(reinterpret_cast<char*>(&lvq_level), sizeof(LVQLevel));

bool initialized = false;
in.read(reinterpret_cast<char*>(&initialized), sizeof(bool));
if (!initialized) {
return Status_Ok;
}

if (svs::detail::intel_enabled()) {
switch (lvq_level) {
case LVQLevel::LVQ4x0:
Expand Down
78 changes: 75 additions & 3 deletions bindings/cpp/src/IndexSVSVamanaLeanVecImpl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ IndexSVSVamanaLeanVecImpl::~IndexSVSVamanaLeanVecImpl() = default;

void IndexSVSVamanaLeanVecImpl::reset() noexcept {
IndexSVSVamanaImpl::reset();
leanvec_matrix.reset();
// leanvec_matrix.reset();
}

Status IndexSVSVamanaLeanVecImpl::train(size_t n, const float* x) noexcept {
Expand All @@ -157,7 +157,7 @@ Status IndexSVSVamanaLeanVecImpl::init_impl(size_t n, const float* x) noexcept {
return Status{ErrorCode::UNKNOWN_ERROR, "Index already initialized"};
}

if (!is_trained()) {
if (!trained) {
return {
ErrorCode::NOT_INITIALIZED,
"Cannot initialize SVS LeanVec index without training first."};
Expand Down Expand Up @@ -237,13 +237,86 @@ Status IndexSVSVamanaLeanVecImpl::init_impl(size_t n, const float* x) noexcept {
);
}

Status IndexSVSVamanaLeanVecImpl::serialize_impl(std::ostream& out) const noexcept {
// TODO: try/catch around writes, report error codes --> macro?

// Store LeanVec specific members
out.write(reinterpret_cast<const char*>(&leanvec_d), sizeof(size_t));
out.write(reinterpret_cast<const char*>(&leanvec_level), sizeof(LeanVecLevel));
out.write(reinterpret_cast<const char*>(&trained), sizeof(bool));

bool has_matrix = (leanvec_matrix != nullptr);
out.write(reinterpret_cast<const char*>(&has_matrix), sizeof(bool));

if (has_matrix) {
// To avoid another temp file, stream the data directly
size_t num_rows = leanvec_matrix->num_rows();
size_t num_cols = leanvec_matrix->num_cols();
// check for overflow (guard against division by zero when num_cols == 0)
if (num_cols != 0 && num_rows > std::numeric_limits<size_t>::max() / num_cols) {
return Status{
ErrorCode::IO_ERROR, "Failed to serialize leanvec matrix: size overflow"};
}

// data and query matrices are the same, can use either
auto matrix = leanvec_matrix->view_data_matrix();

size_t elements = num_rows * num_cols;

out.write(reinterpret_cast<const char*>(&num_rows), sizeof(size_t));
out.write(reinterpret_cast<const char*>(&num_cols), sizeof(size_t));
out.write(reinterpret_cast<const char*>(matrix.data()), elements * sizeof(float));
}

// This will also write whether or not we're initialized
return IndexSVSVamanaImpl::serialize_impl(out);
}

Status IndexSVSVamanaLeanVecImpl::deserialize_impl(std::istream& in) noexcept {
if (impl) {
return Status{
ErrorCode::INVALID_ARGUMENT,
"Cannot deserialize: SVS index already initialized."};
}

in.read(reinterpret_cast<char*>(&leanvec_d), sizeof(size_t));
Copy link
Member

@rfsaliev rfsaliev Oct 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. I would reuse existing matrix serialization mechanizm implemented in SVS

Copy link
Contributor Author

@ahuber21 ahuber21 Oct 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, now that it has been moved to SVS we can just use that. Is it difficult to re-use what you added for index save/load?

in.read(reinterpret_cast<char*>(&leanvec_level), sizeof(LeanVecLevel));
in.read(reinterpret_cast<char*>(&trained), sizeof(bool));

bool has_matrix = false;
in.read(reinterpret_cast<char*>(&has_matrix), sizeof(bool));

if (has_matrix) {
size_t num_rows = 0;
size_t num_cols = 0;

// TODO: read macro for error handling
in.read(reinterpret_cast<char*>(&num_rows), sizeof(size_t));
in.read(reinterpret_cast<char*>(&num_cols), sizeof(size_t));

std::vector<float> matrix_data(num_rows * num_cols);
in.read(
reinterpret_cast<char*>(matrix_data.data()), num_rows * num_cols * sizeof(float)
);

auto matrix = svs::data::SimpleData<float, svs::Dynamic>(num_rows, num_cols);
for (size_t i = 0; i < num_rows; ++i) {
const auto datum =
std::span<const float>(matrix_data.data() + i * num_cols, num_cols);
matrix.set_datum(i, datum);
}

leanvec_matrix =
Copy link
Member

@rfsaliev rfsaliev Oct 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would try to extract leanvec matrices from loaded svs::leanvec::LeanDataset instead of implementing custom serialization here.

Copy link
Contributor Author

@ahuber21 ahuber21 Oct 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the current implementation it's possible to have a leanvec matrix (after running train) without having an index.

std::make_unique<svs::leanvec::LeanVecMatrices<svs::Dynamic>>(matrix, matrix);
}

bool initialized = false;
in.read(reinterpret_cast<char*>(&initialized), sizeof(bool));

if (!initialized) {
return Status_Ok;
}

if (svs::detail::intel_enabled()) {
switch (leanvec_level) {
case LeanVecLevel::LeanVec4x4:
Expand All @@ -264,7 +337,6 @@ Status IndexSVSVamanaLeanVecImpl::deserialize_impl(std::istream& in) noexcept {
impl.reset(deserialize_impl_t<storage_type_sq>(in, metric_type_));
}

trained = true;
return Status_Ok;
}

Expand Down
Loading