Skip to content

Conversation

@razdoburdin
Copy link
Contributor

This PR adds ability to set custom index blocksize
Reopening of #235

@rfsaliev
Copy link
Member

Hi!
Thank you for the PR.
Looks good but I see some issues:

  • Extra dependencies on SVS internal headers (svs/lib/...) added to the runtime API in index_blocksize.h - I would avoid such dependencies by replacing:
    • usage svs::lib::PowerOfTwo with just a size_t BlockSizeExponent(); and size_t BlockSizeBytes() const { return 1 << blocksize_exp_; }
    • replace IndexBlockSize .ctor with a static factory method e.g. static Status IndexBlockSize::make(size_t blocksize_exp) noexcept; - as it was made to other runtime API classes.
  • The DynamicVamanaIndex::add(..., IndexBlockSize); method has unclear semantic, because blocksize argument can be applied at the first call only, but not at subsequential calls.

Instead I would propose to setup block size in a build() method:

  • One of solutions can be blocksize field in VamanaIndex::BuildParams - but it seems block size makes no sense in non-dynamic indices.
  • Another option: define a dedicated structure with parameters for dynamic indices: e.g. blocksize, reuse_empty_slots, etc.

Copy link
Member

@rfsaliev rfsaliev left a comment

Choose a reason for hiding this comment

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

Please also read my standalone comment to the PR as well.

#include <cstddef>

#include <svs/lib/exception.h>
#include <svs/lib/misc.h>
Copy link
Member

Choose a reason for hiding this comment

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

Please avoid SVS internals dependencies in SVS Runtime

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

public:
explicit IndexBlockSize(size_t blocksize_exp) {
if (blocksize_exp > kMaxBlockSizeExp) {
throw ANNEXCEPTION("Blocksize is too large!");
Copy link
Member

Choose a reason for hiding this comment

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

SVS runtime API should be noexcept

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the IndexBlockSize structure is removed

return runtime_error_wrapper([&] {
svs::data::ConstSimpleDataView<float> data{x, n, impl_->dimensions()};
std::span<const size_t> lbls(labels, n);
impl_->add(data, lbls, blocksize);
Copy link
Member

Choose a reason for hiding this comment

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

Please convert SVS Runtime API data types (IndexBlockSize) to SVS internal type (PowerOfTwo) here.
As it was made for data argument

return slots;
}

lib::PowerOfTwo blocksize_bytes() const {
Copy link
Member

Choose a reason for hiding this comment

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

I do not think it makes sense to modify SVS internals just to implement a query for SVS runtime API

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed
The getter now calls for parameter filed, not the SVS internals

@razdoburdin
Copy link
Contributor Author

Thanks for comments.

I have modified the PR, according to your request:

  1. I have added a new structure VamanaIndex::DynamicIndexParams, being passed to build() constructor. It contains the only field blocksize_exp now, but it can be expanded.
  2. In this case the dedicated IndexBlockSize is no longer needed, so I have removed it. The DynamicVamanaIndex class is no responsible for checking the validity of input parameters by calling check_params method.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants