resizing to align blocksize #177

meiravgri · 2022-07-27T05:00:52Z

Changes made in this PR:

1. in HNSW index

add: When exceeding capacity, it is increased and aligned with vector block size
delete: if we free an entire vector block, and the size is at least one block size smaller than the capacity, the capacity is
aligned to block size and decreased by block size.
check if label exist in hnsw_wrapper, before calling hnswlib add or delete

2. in brute force index:

add: When exceeding capacity (no more empty cells in idToVectorMemeberMapping) , it is increased and aligned with
vector block size
no resizing down upon deleteion because in the current logic, idToVectorMemeberMapping is not continous.

… size fits blockSize

…ndexing_same_vector test

codecov · 2022-07-27T05:41:06Z

Codecov Report

Merging #177 (93df064) into main (4d6a240) will increase coverage by 0.14%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #177      +/-   ##
==========================================
+ Coverage   93.54%   93.68%   +0.14%     
==========================================
  Files          40       40              
  Lines        2338     2344       +6     
==========================================
+ Hits         2187     2196       +9     
+ Misses        151      148       -3

Impacted Files	Coverage Δ
src/VecSim/algorithms/brute_force/brute_force.cpp	`100.00% <100.00%> (+0.45%)`	⬆️
src/VecSim/algorithms/hnsw/hnsw_wrapper.cpp	`97.40% <100.00%> (+0.12%)`	⬆️
src/VecSim/algorithms/hnsw/hnswlib.h	`97.22% <100.00%> (+0.34%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4d6a240...93df064. Read the comment docs.

…leteVector in case an entire block is removed, the capacity is aligned to blocksize and a decreased by block size, added test for resize cases in hnswlib

alonre24

Good first PR!! :)
I left some comments and questions

src/VecSim/algorithms/brute_force/brute_force.cpp

src/VecSim/algorithms/hnsw/hnsw_wrapper.cpp

tests/unit/test_bruteforce.cpp

tests/unit/test_hnswlib.cpp

…orSimilarity into update_resize_logic

also changed HNSWIndex::delete vector to return false in this case (before checking if resizing is required) - this covers (hopedully) also empty index case and resizing only when actual deletion happend

…in the wrrapper

…llements from addPoint (cant happen, we check this in addVector)

alonre24

Last comments

src/VecSim/algorithms/hnsw/hnsw_wrapper.cpp

tests/unit/test_bruteforce.cpp

tests/unit/test_hnswlib.cpp

Co-authored-by: alonre24 <alonreshef24@gmail.com>

…orSimilarity into update_resize_logic

tests/unit/test_hnswlib.cpp

DvirDukhan

Great

Please edit the PR description to describe your changes
please add a test where capacity is dropped to 0. If it is already covered by one of the existing tests, just mention it.

src/VecSim/algorithms/brute_force/brute_force.cpp

DvirDukhan · 2022-07-31T13:18:53Z

src/VecSim/algorithms/hnsw/hnsw_wrapper.cpp

+        size_t extra_space_to_free = curr_capacity % blockSize;
+
+        // Remove one block from the capacity. TODO check that we dont go below zero
+        this->hnsw->resizeIndex(curr_capacity - blockSize - extra_space_to_free);


What about this TODO?

tests/unit/test_hnswlib.cpp

Co-authored-by: DvirDukhan <dvir@redis.com>

* modified resize logic of BF addvector so that the ids2vectors mapping size fits blockSize * fixed increasing count in case od re-using id, update brute_force_reindexing_same_vector test * clang format * HNSWIndex::addVector aligns the capacity to block size, HNSWIndex::deleteVector in case an entire block is removed, the capacity is aligned to blocksize and a decreased by block size, added test for resize cases in hnswlib * make format * update size estimation test * fixed after Alon's review * clang format * cahnged hnsw::remove point to return false in case not found, also changed HNSWIndex::delete vector to return false in this case (before checking if resizing is required) - this covers (hopedully) also empty index case and resizing only when actual deletion happend * added tests for empty HNSW index * added isLabelExists to hnswlib and modified test of id overrrides * hnsw::removepoint return void, checking if label exists happens only in the wrrapper * clang format * update test to removed checking if element count is bigger than max_ellements from addPoint (cant happen, we check this in addVector) * override test intial size * remove if exists in addvector update * Update src/VecSim/algorithms/hnsw/hnsw_wrapper.cpp Co-authored-by: alonre24 <alonreshef24@gmail.com> * Update tests/unit/test_hnswlib.cpp Co-authored-by: alonre24 <alonreshef24@gmail.com> * Update tests/unit/test_hnswlib.cpp Co-authored-by: alonre24 <alonreshef24@gmail.com> * removed redudent tests after alon's review * fixed tests * Update src/VecSim/algorithms/brute_force/brute_force.cpp Co-authored-by: DvirDukhan <dvir@redis.com> * Update tests/unit/test_hnswlib.cpp Co-authored-by: DvirDukhan <dvir@redis.com> * empty index and capacity = 0 tests. removed todo from hnsw_wrapper Co-authored-by: alonre24 <alonreshef24@gmail.com> Co-authored-by: DvirDukhan <dvir@redis.com>

modified resize logic of BF addvector so that the ids2vectors mapping…

50788eb

… size fits blockSize

meiravgri requested review from DvirDukhan and alonre24 July 27, 2022 05:07

fixed increasing count in case od re-using id, update brute_force_rei…

ea40e48

…ndexing_same_vector test

clang format

7e79daa

meiravgri removed request for DvirDukhan and alonre24 July 27, 2022 11:39

meiravgri added 2 commits July 27, 2022 15:26

HNSWIndex::addVector aligns the capacity to block size, HNSWIndex::de…

312cfea

…leteVector in case an entire block is removed, the capacity is aligned to blocksize and a decreased by block size, added test for resize cases in hnswlib

make format

2948388

meiravgri requested review from alonre24, DvirDukhan and GuyAv46 July 27, 2022 12:28

meiravgri and others added 2 commits July 27, 2022 16:50

update size estimation test

04f07da

Merge branch 'main' into update_resize_logic

97eaae9

meiravgri changed the title ~~modified resize logic of BF addvector so that the ids2vectors mapping…~~ resizing to align blocksize Jul 27, 2022

alonre24 reviewed Jul 27, 2022

View reviewed changes

meiravgri and others added 12 commits July 28, 2022 08:02

fixed after Alon's review

b277fca

Merge branch 'update_resize_logic' of https://github.com/RedisAI/Vect…

68b0106

…orSimilarity into update_resize_logic

clang format

58d639e

cahnged hnsw::remove point to return false in case not found,

3de7b8c

also changed HNSWIndex::delete vector to return false in this case (before checking if resizing is required) - this covers (hopedully) also empty index case and resizing only when actual deletion happend

added tests for empty HNSW index

aec50b2

added isLabelExists to hnswlib and modified test of id overrrides

f3021d1

hnsw::removepoint return void, checking if label exists happens only …

7fa6cac

…in the wrrapper

clang format

3acbf38

update test to removed checking if element count is bigger than max_e…

d8d1f01

…llements from addPoint (cant happen, we check this in addVector)

override test intial size

14990ee

remove if exists in addvector update

2bb1b4f

Merge branch 'main' into update_resize_logic

87be64f

alonre24 reviewed Jul 31, 2022

View reviewed changes

meiravgri and others added 5 commits July 31, 2022 11:07

Update src/VecSim/algorithms/hnsw/hnsw_wrapper.cpp

4d26b35

Co-authored-by: alonre24 <alonreshef24@gmail.com>

Update tests/unit/test_hnswlib.cpp

d8aee57

Co-authored-by: alonre24 <alonreshef24@gmail.com>

Update tests/unit/test_hnswlib.cpp

394e792

Co-authored-by: alonre24 <alonreshef24@gmail.com>

removed redudent tests after alon's review

4cfa183

Merge branch 'update_resize_logic' of https://github.com/RedisAI/Vect…

809d051

…orSimilarity into update_resize_logic

alonre24 reviewed Jul 31, 2022

View reviewed changes

tests/unit/test_hnswlib.cpp Outdated Show resolved Hide resolved

fixed tests

4aa3ca5

alonre24 previously approved these changes Jul 31, 2022

View reviewed changes

DvirDukhan requested changes Jul 31, 2022

View reviewed changes

Update src/VecSim/algorithms/brute_force/brute_force.cpp

cf3bb52

Co-authored-by: DvirDukhan <dvir@redis.com>

meiravgri dismissed alonre24’s stale review via cf3bb52 August 1, 2022 04:04

meiravgri and others added 2 commits August 1, 2022 07:05

Update tests/unit/test_hnswlib.cpp

4abaa1d

Co-authored-by: DvirDukhan <dvir@redis.com>

empty index and capacity = 0 tests. removed todo from hnsw_wrapper

93df064

DvirDukhan approved these changes Aug 1, 2022

View reviewed changes

meiravgri merged commit 68129ce into main Aug 1, 2022

meiravgri deleted the update_resize_logic branch August 1, 2022 06:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

resizing to align blocksize #177

resizing to align blocksize #177

meiravgri commented Jul 27, 2022 •

edited

Loading

codecov bot commented Jul 27, 2022 •

edited

Loading

alonre24 left a comment

alonre24 left a comment

DvirDukhan left a comment

DvirDukhan Jul 31, 2022

resizing to align blocksize #177

resizing to align blocksize #177

Conversation

meiravgri commented Jul 27, 2022 • edited Loading

Changes made in this PR:

1. in HNSW index

2. in brute force index:

codecov bot commented Jul 27, 2022 • edited Loading

Codecov Report

alonre24 left a comment

Choose a reason for hiding this comment

alonre24 left a comment

Choose a reason for hiding this comment

DvirDukhan left a comment

Choose a reason for hiding this comment

DvirDukhan Jul 31, 2022

Choose a reason for hiding this comment

meiravgri commented Jul 27, 2022 •

edited

Loading

codecov bot commented Jul 27, 2022 •

edited

Loading