Native serialization for Vamana index#285
Native serialization for Vamana index#285razdoburdin merged 9 commits intointel:dev/razdoburdin_streamingfrom
Conversation
razdoburdin
requested review from
ahuber21,
ibhati,
mihaic and
yuejiaointel
as code owners
razdoburdin
requested review from
rfsaliev
and removed request for
ahuber21,
ibhati,
mihaic and
yuejiaointel
|
CI failure isn't related to the PR's content. |
include/svs/index/vamana/index.h
Outdated
|
|
||
| lib::begin_serialization(os); | ||
| // Config | ||
| lib::save_to_stream(parameters.save(), os); |
There was a problem hiding this comment.
Does it make sense to rename VamanaIndexParameters::save() to metadata() as well?
Or implement the infrastructure code for save_to_stream() similar to save_to_disk() then call just
lib::save_to_stream(parameters, os)?
include/svs/lib/saveload/load.h
Outdated
| if constexpr (requires { | ||
| T::load( | ||
| std::declval<const ContextFreeLoadTable&>(), | ||
| std::declval<Args&&>()... | ||
| ); | ||
| }) { | ||
| // Object is loadable from it's toml::table | ||
| return lib::load( | ||
| loader, detail::read_metadata(deserializer, stream), SVS_FWD(args)... | ||
| ); | ||
| } else { | ||
| return lib::load( | ||
| loader, | ||
| detail::read_metadata(deserializer, stream), | ||
| deserializer, | ||
| stream, | ||
| SVS_FWD(args)... | ||
| ); | ||
| } |
There was a problem hiding this comment.
Got some doubts how this condition looks like.
Doesn't it make sense to define 2 specializations of load_from_stream using newly defined concept ContextFreeLoadable which requires T::load(...?
...
I would also look for possibility of modifying/extending LoadContext infrastructure and related classes to store the {deserializer, stream} information in a context.
There was a problem hiding this comment.
I have created 2 specializations for load_from_stream
The existing infrastructure for LoadContext/SaveContext doesn't take the data order into account. Organizing of the input/output stream is made by DirectoryArchiver. But for native streaming we want to avoid (or at least minimize) creating of temporary buffers, that means that we have to control the read/write order. This is the reason I have chosen not to reuse this logic in native streaming.
Co-authored-by: Rafik Saliev <rafik.f.saliev@intel.com>
razdoburdin
merged commit 620ac9f
into
intel:dev/razdoburdin_streaming
2 participants
This PR introduce native serialization for
Vamanaindex.Main changes are:
svs::index::vamana::auto_assemblerequired for direct deserialization accepts lazy loaders and call them in a flexible order to cover legacy serilized models.save_tablemethod is renamed tometadatato avoid confusions, as far as it doesn't save anything.MutableVamanais also implemented to avoid compilation errors. Deserialization and related tests forMutableVamanaare expected later.