FAISS HNSW #746
FAISS HNSW #746
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #746 +/- ##
=========================================
+ Coverage 0 61.79% +61.79%
=========================================
Files 0 84 +84
Lines 0 6123 +6123
=========================================
+ Hits 0 3784 +3784
- Misses 0 2339 +2339 |
alexanderguzhva
force-pushed
the
upgrade_faiss_hnsw_4
branch
4 times, most recently
from
edc1cbd to
dec20d0
Compare
There was a problem hiding this comment.
Just a first glance comments. More detailed review will follow.
Can I ask some summary about the code structure for faster reviewing?
- What is the Wrappers and why we need them
- The relationship between Wrappers in Knowhere & Faiss
- Difference between the overrided HNSW and Faiss'
Thanks
alexanderguzhva
force-pushed
the
upgrade_faiss_hnsw_4
branch
10 times, most recently
from
2eaf4bd to
551e435
Compare
Signed-off-by: Alexandr Guzhva <alexanderguzhva@gmail.com>
Signed-off-by: Alexandr Guzhva <alexanderguzhva@gmail.com>
Signed-off-by: Alexandr Guzhva <alexanderguzhva@gmail.com>
Signed-off-by: Alexandr Guzhva <alexanderguzhva@gmail.com>
Signed-off-by: Alexandr Guzhva <alexanderguzhva@gmail.com>
Signed-off-by: Alexandr Guzhva <alexanderguzhva@gmail.com>
Signed-off-by: Alexandr Guzhva <alexanderguzhva@gmail.com>
Signed-off-by: Alexandr Guzhva <alexanderguzhva@gmail.com>
Signed-off-by: Alexandr Guzhva <alexanderguzhva@gmail.com>
Signed-off-by: Alexandr Guzhva <alexanderguzhva@gmail.com>
alexanderguzhva
force-pushed
the
upgrade_faiss_hnsw_4
branch
4 times, most recently
from
069bdcd to
bc6b6a8
Compare
…tion Signed-off-by: Alexandr Guzhva <alexanderguzhva@gmail.com>
alexanderguzhva
force-pushed
the
upgrade_faiss_hnsw_4
branch
from
bc6b6a8 to
924049a
Compare
|
How is it possible to run the benchmark /UT for this PR? |
|
@liorf95 We have benchmarks that are done in https://github.com/zilliztech/vdbbench, they will be published as soon as the code is reviewed. Basically, at this moment:
Alternatively, if you'd like to test the code yourself, please use hnswlib flat for 768 dim 1M dataset (current) faiss hnsw flat for 768 dim 1M dataset (candidate) |
Wow!Thanks a lot for all that helpful information. |
|
@liorf95 |
| for (int64_t i = 0; i < rows; i++) { | ||
| const int64_t id = ids[i]; | ||
| assert(id >= 0 && id < index->ntotal); | ||
| index->reconstruct(id, tmp.get()); |
There was a problem hiding this comment.
What if the datatype is FP16 but index type is HNSWSQ4 without refinement or FP16 refinement?
There was a problem hiding this comment.
if HNSWSQ is used and no refinement is available, then the reconstruction will be inaccurate.
Please let me know the requirements for this particular method. What is it expected to return exactly?
There was a problem hiding this comment.
GetVectorById need to return an error code if there is no raw data available (If the data type is FP16, then the raw data need to be FP16).
So we need a raw data existence check there.
There was a problem hiding this comment.
Created an issue #778 to track and unblock this
| // the following structure is a hack, because GCC cannot properly | ||
| // de-virtualize a plain BitsetViewIDSelector. |
There was a problem hiding this comment.
This reminds me that Knowhere's IVF-series indexes are using the virtual selector. Will the performance also be a concern there?
There was a problem hiding this comment.
Also in the faiss_hnsw.cc, we use BitsetViewIdSelector to do search
There was a problem hiding this comment.
the performance for selector impact may have an impact in brture force search, because it implies 90+% of filtering. In other cases, the cost of calling a virtual method is negligible compares to the cost of distance computations.
There was a problem hiding this comment.
Saying that it is also a concern.
- IVF can have super high filtering rate but still uses virtual method.
- To my experience the, HNSW validation is not negligible. We noticed 10% performance regression in cardinal if we try to do one more deference for that.
There was a problem hiding this comment.
Created an issue #779 to track this and unblock this PR
| namespace knowhere { | ||
|
|
||
| // a wrapper that overrides a distance computer | ||
| IndexWrapperCosine::IndexWrapperCosine(faiss::Index* index, const float* inverse_l2_norms_in) |
There was a problem hiding this comment.
Why we need a wrapper instead of putting logics in IndexHNSWxxxxCosine?
There was a problem hiding this comment.
Ultimately, it was needed for refine and to enable/disable refine for the search wirh a config parameter.
| namespace cppcontrib { | ||
| namespace knowhere { | ||
|
|
||
| IndexBruteForceWrapper::IndexBruteForceWrapper(Index* underlying_index) : |
There was a problem hiding this comment.
Why we have two BruteForce Wrappers? one in Knowhere and one in Faiss. And looks like nowhere call this one
There was a problem hiding this comment.
I expect to merge one, a generic one into Faiss, if they accept it. Another one is a specialized version for our needs. The other one might be removed in Faiss guys reject it.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alexanderguzhva, liliu-z The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Let's check this big change in first to unblock the following work. |
Introducing modified FAISS HNSW version, which outperforms current hnswlib HNSW implementation.
The code is somewhat dirty and is subject to further possibly significant changes.
/kind improvement