Refactor + maintain node's in-degree field in HNSW #478

alonre24 · 2024-07-01T13:24:17Z

As a preparation for implementing unreachable nodes connection in HNSW - this PR introduced a new field in the graph node which is the overall node's indegree, and maintained it wherever needed.

This also includes:

Refactoring the graph data structures, using getters and setters, and moving it to a different file.
Introduce a new encoding version for HNSW (v4) and populate the new field for indexes that were serialized with older versions.
Update checkIntegrity to validate the integrity of the new field.

Mark if applicable

This PR introduces API changes
This PR introduces serialization changes

… - take care of serializer for benchmarks

…on and adjust loading accordingly.

codecov · 2024-07-01T13:48:12Z

Codecov Report

Attention: Patch coverage is 99.03537% with 3 lines in your changes missing coverage. Please review.

Project coverage is 97.15%. Comparing base (dbb9d24) to head (8db2527).
Report is 2 commits behind head on main.

Files	Patch %	Lines
src/VecSim/algorithms/hnsw/graph_data.h	98.18%	1 Missing ⚠️
src/VecSim/algorithms/hnsw/hnsw.h	99.45%	1 Missing ⚠️
src/VecSim/algorithms/hnsw/hnsw_serializer.h	98.11%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #478      +/-   ##
==========================================
+ Coverage   97.12%   97.15%   +0.03%     
==========================================
  Files          90       91       +1     
  Lines        4770     4858      +88     
==========================================
+ Hits         4633     4720      +87     
- Misses        137      138       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

GuyAv46 · 2024-07-03T10:33:41Z

src/VecSim/algorithms/hnsw/graph_data.h

Is there a reason to move these classes out of hnsw.h?

It is a long file, and now that these structures are encapsulated they can stand for themselves as I see it

GuyAv46 · 2024-07-03T10:33:45Z

src/VecSim/algorithms/hnsw/graph_data.h

+    void increaseTotalIncomingEdgesNum() { this->totalIncomingLinks++; }
+    void decreaseTotalIncomingEdgesNum() { this->totalIncomingLinks--; }


can't we encapsulate the total incoming links logic with the new API? (increase/decrease after push/pop)

no, because increase/decrease is not always correlated with the insertion to the incoming edges set which only includes the unidirectional edges.

can't we wrap it under connectUnidirectional(node, neighbour) and connectBiDirctional(node, neighbor) (and compliment disconnect methods) ?

GuyAv46 · 2024-07-03T10:34:25Z

src/VecSim/algorithms/hnsw/graph_data.h

+            }
+        }
+    }
+    ~ElementGraphData() = delete; // Should be destroyed using `destroyGraphData`


... which is not implemented in this file.

GuyAv46 · 2024-07-03T11:29:27Z

src/VecSim/utils/serializer.cpp

@@ -7,7 +7,7 @@
 void Serializer::saveIndex(const std::string &location) {

    // Serializing with V3.
-    EncodingVersion version = EncodingVersion_V3;
+    EncodingVersion version = EncodingVersion_V4;


update comment. Maybe we can change it to // Serializing with latest

GuyAv46 · 2024-07-04T09:40:48Z

src/VecSim/algorithms/hnsw/hnsw_serializer.h

+            if (cur.totalIncomingLinks != inbound_connections_num[i][l]) {
+                return res;
+            }


What's this? When can that happen?

This is the sanity check for the new field. It should never happen, and if so, it means that the index is corrupted.

meiravgri

Fixing abstraction of LevelData improve the code readability and maintainability, well done.
We should rename incomingEdges or totalIncomingLinks or numLinks.
I would consider something like incomingUnidirectionalEdges.
It is very confusing in the current format and the addition of the totalIncomingLinks makes it even harder to follow and to maintain.

meiravgri · 2024-07-14T08:54:33Z

src/VecSim/algorithms/hnsw/hnsw.h

@@ -73,62 +73,11 @@ struct ElementMetaData {
    labelType label;
    elementFlags flags;

-    ElementMetaData(labelType label = SIZE_MAX) noexcept : label(label), flags(IN_PROCESS) {}
+    explicit ElementMetaData(labelType label = SIZE_MAX) noexcept
+        : label(label), flags(IN_PROCESS) {}
 };
 #pragma pack() // restore default packing


#pragma pack()
should also be copied to the graph_data.h?

I don't think so, since we are using the default padding for the LevelData. Keep me honest here @GuyAv46 ;)

src/VecSim/algorithms/hnsw/hnsw.h

src/VecSim/algorithms/hnsw/graph_data.h

src/VecSim/algorithms/hnsw/hnsw_serializer_declarations.h

meiravgri · 2024-07-14T11:41:03Z

src/VecSim/algorithms/hnsw/graph_data.h

+    void increaseTotalIncomingEdgesNum() { this->totalIncomingLinks++; }
+    void decreaseTotalIncomingEdgesNum() { this->totalIncomingLinks--; }


can't we wrap it under connectUnidirectional(node, neighbour) and connectBiDirctional(node, neighbor) (and compliment disconnect methods) ?

tests/benchmark/bm_files.sh

src/VecSim/algorithms/hnsw/hnsw.h

tests/unit/test_hnsw_tiered.cpp

src/VecSim/utils/vec_utils.cpp

alonre24 added 2 commits June 30, 2024 19:33

Refactor + implement in-degree maintained and update sanity test. WIP…

6ecbd15

… - take care of serializer for benchmarks

Move graph data structures to a new file, add new serialization versi…

61d4cb7

…on and adjust loading accordingly.

alonre24 requested a review from GuyAv46 July 1, 2024 13:24

tmp adding full benchmark file path for debug

9897acc

alonre24 added the benchmarks-default label Jul 1, 2024

alonre24 added 5 commits July 2, 2024 12:17

Add test for coverage of the new decoding HNSW version

c72021a

Add the files for the test

5f59fa9

revert file name

5fe358d

revert file name + format

90f24d6

test for serialization v3 only for float32

6abc4eb

GuyAv46 reviewed Jul 4, 2024

View reviewed changes

GuyAv46 added benchmarks-default and removed benchmarks-default labels Jul 4, 2024

alonre24 requested a review from meiravgri July 14, 2024 08:30

meiravgri reviewed Jul 14, 2024

View reviewed changes

alonre24 added 4 commits July 14, 2024 22:14

Addressing Meirav's CR

20fa8f8

merge with main

0a14c7d

format

604415f

remove flakiness, revert formatting

2d6a95c

alonre24 requested a review from meiravgri July 15, 2024 09:00

meiravgri reviewed Jul 15, 2024

View reviewed changes

src/VecSim/algorithms/hnsw/hnsw.h Outdated Show resolved Hide resolved

tests/unit/test_hnsw_tiered.cpp Show resolved Hide resolved

alonre24 requested review from GuyAv46 and meiravgri July 15, 2024 14:50

meiravgri previously approved these changes Jul 15, 2024

View reviewed changes

GuyAv46 reviewed Jul 15, 2024

View reviewed changes

src/VecSim/utils/vec_utils.cpp Outdated Show resolved Hide resolved

src/VecSim/utils/vec_utils.cpp Outdated Show resolved Hide resolved

move remove to vector

b0ec5c0

alonre24 dismissed meiravgri’s stale review via b0ec5c0 July 15, 2024 15:32

GuyAv46 previously approved these changes Jul 15, 2024

View reviewed changes

alonre24 enabled auto-merge July 15, 2024 15:44

alonre24 added this pull request to the merge queue Jul 15, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 15, 2024

include mutex

8db2527

alonre24 dismissed GuyAv46’s stale review via 8db2527 July 16, 2024 08:44

alonre24 enabled auto-merge July 16, 2024 08:54

GuyAv46 approved these changes Jul 16, 2024

View reviewed changes

alonre24 added this pull request to the merge queue Jul 16, 2024

Merged via the queue into main with commit e07f7ad Jul 16, 2024
20 checks passed

alonre24 deleted the keep_incoming_edges_num_hnsw branch July 16, 2024 10:09

This was referenced Sep 1, 2024

Revert in-degree HNSW [MOD-7520] #536

Merged

[MOD-7643] Refactor graph data + handle in-place deletion tiered [0.8] #537

Merged

Refactor HNSW graph data + handle in-place deletion tiered CP [0.7] #540

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor + maintain node's in-degree field in HNSW #478

Refactor + maintain node's in-degree field in HNSW #478

alonre24 commented Jul 1, 2024 •

edited by GuyAv46

Loading

codecov bot commented Jul 1, 2024 •

edited

Loading

GuyAv46 Jul 3, 2024

alonre24 Jul 9, 2024

GuyAv46 Jul 3, 2024

alonre24 Jul 9, 2024

meiravgri Jul 14, 2024

GuyAv46 Jul 3, 2024

GuyAv46 Jul 3, 2024

GuyAv46 Jul 4, 2024

alonre24 Jul 9, 2024

meiravgri left a comment

meiravgri Jul 14, 2024 •

edited

Loading

alonre24 Jul 14, 2024 •

edited

Loading

meiravgri Jul 14, 2024

		void increaseTotalIncomingEdgesNum() { this->totalIncomingLinks++; }
		void decreaseTotalIncomingEdgesNum() { this->totalIncomingLinks--; }

Refactor + maintain node's in-degree field in HNSW #478

Refactor + maintain node's in-degree field in HNSW #478

Conversation

alonre24 commented Jul 1, 2024 • edited by GuyAv46 Loading

codecov bot commented Jul 1, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meiravgri left a comment

Choose a reason for hiding this comment

meiravgri Jul 14, 2024 • edited Loading

Choose a reason for hiding this comment

alonre24 Jul 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alonre24 commented Jul 1, 2024 •

edited by GuyAv46

Loading

codecov bot commented Jul 1, 2024 •

edited

Loading

meiravgri Jul 14, 2024 •

edited

Loading

alonre24 Jul 14, 2024 •

edited

Loading