Add jni interface to use a binary hnsw index with faiss #1747

heemin32 · 2024-06-11T23:59:38Z

Description

Add jni interface to use a binary hnsw index with faiss

SearchParameter is not supported with binary hnsw index in faiss library yet. Therefore, multi vector, pre filtering, and ef-search parameter passing won't work with binary index. Support of SearchParameter in binary index with faiss will be handled either by a PR in faiss repo or faiss custom patch in a separate PR.

Issues Resolved

N/A

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Commits are signed as per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

CHANGELOG.md

jni/src/faiss_wrapper.cpp

src/main/java/org/opensearch/knn/jni/FaissService.java

src/testFixtures/java/org/opensearch/knn/TestUtils.java

shatejas

Reviewed this partly

src/main/java/org/opensearch/knn/jni/JNIService.java

src/test/java/org/opensearch/knn/jni/JNIServiceTests.java

jni/src/faiss_wrapper.cpp

jni/src/jni_util.cpp

heemin32 · 2024-06-13T01:50:30Z

@heemin32 lets fix the GH actions too.

Will do. So far, the error is coming from https://github.com/opensearch-project/k-NN/pull/1747/files#diff-892c5ae9158d35b9f5fa9b67d53d7373ca6d2bf04d1052add99c329d03645409R110 as index description is not passed to the method.

shatejas

Looks good overall. A few things to see what you think

jni/include/faiss_methods.h

shatejas · 2024-06-24T16:47:48Z

jni/src/faiss_wrapper.cpp

+            // Setting the ef_search value equal to what was provided during index creation. SearchParametersHNSW has a default
+            // value of ef_search = 16 which will then be used.
+            hnswParams.efSearch = hnswReader->hnsw.efSearch;
+            hnswParams.sel = idSelector.get();


This will have to change for efsearch and nprobs as both will be merged for 2.16. can you please add a TODO

TODO might not be enough to catch it. Let me create GH issue instead. #1769

jni/tests/faiss_index_service_test.cpp

shatejas · 2024-06-24T16:57:11Z

jni/src/faiss_wrapper.cpp

@@ -247,6 +235,19 @@ jlong knn_jni::faiss_wrapper::LoadIndex(knn_jni::JNIUtilInterface * jniUtil, JNI
    return (jlong) indexReader;
 }

+jlong knn_jni::faiss_wrapper::LoadBinaryIndex(knn_jni::JNIUtilInterface * jniUtil, JNIEnv * env, jstring indexPathJ) {


Should Indexservice have loadIndex? that way we pick the right implementation of loadIndex and the logic is in the right place?

Yes. Didn't add it now because wasn't sure if current approach of using index service will be accepted.
Once this PR is merged, will move the load index method into index service in a separate PR.

src/main/java/org/opensearch/knn/index/IndexUtil.java

src/main/java/org/opensearch/knn/index/KNNIndexShard.java

jmazanec15

I like the refactors and I think it looks good. One thing - can we run cpp unit tests with valgrind on linux to see if we are leaking any memory?

jni/include/commons.h

jmazanec15 · 2024-06-24T19:16:31Z

jni/include/faiss_index_service.h

+class IndexService {
+public:
+    IndexService(std::unique_ptr<FaissMethods> faissMethods);
+    virtual void createIndex(


Should we put parameter definitions?

jni/include/faiss_index_service.h

jmazanec15 · 2024-06-24T19:18:43Z

jni/include/faiss_methods.h

+ *
+ * This class helps to mock faiss methods during unit test
+ */
+class FaissMethods {


Are we missing some methods? i.e. load, free, search,...

I haven't moved all methods yet. We will add incrementally.

Lets add todo

I didn't want to go through code and see what is needed to be in this class and add them as todo. We can add methods as needed base. For example, we don't need to add search method here because search is a method of Index struct which can be mocked without this class.

Signed-off-by: Heemin Kim <heemin@amazon.com>

navneet1v · 2024-06-25T05:52:52Z

jni/src/faiss_index_service.cpp

+    }
+}
+
+IndexService::IndexService(std::unique_ptr<FaissMethods> faissMethods) : faissMethods(std::move(faissMethods)) {}


The std::move is an interesting choice. But what my thinking is why we are using std::move here rather than just using a pointer or do a call by reference if we are concerned about copying of objects.

Call by reference increase a chance of memory leak by bug.

as this is an object, and you used the unique pointer, I doubt its the case. But if we 100% sure that this will not cause any performance issues and other memory leak I am okay to approve the code.

This std::move is passing its ownership. If you don't use it, the ownership won't be transferred to IndexService and the memory can be released while IndexService sill needs it.

jni/src/faiss_wrapper.cpp

src/main/java/org/opensearch/knn/jni/JNIService.java

Signed-off-by: Heemin Kim <heemin@amazon.com>

* Add jni interface to use a binary hnsw index with faiss (#1747) Signed-off-by: Heemin Kim <heemin@amazon.com> * Fix memory leak on test code (#1776) Signed-off-by: Heemin Kim <heemin@amazon.com> --------- Signed-off-by: Heemin Kim <heemin@amazon.com>

…roject#1778) * Add jni interface to use a binary hnsw index with faiss (opensearch-project#1747) Signed-off-by: Heemin Kim <heemin@amazon.com> * Fix memory leak on test code (opensearch-project#1776) Signed-off-by: Heemin Kim <heemin@amazon.com> --------- Signed-off-by: Heemin Kim <heemin@amazon.com>

…roject#1747) Signed-off-by: Heemin Kim <heemin@amazon.com>

…roject#1778) * Add jni interface to use a binary hnsw index with faiss (opensearch-project#1747) Signed-off-by: Heemin Kim <heemin@amazon.com> * Fix memory leak on test code (opensearch-project#1776) Signed-off-by: Heemin Kim <heemin@amazon.com> --------- Signed-off-by: Heemin Kim <heemin@amazon.com>

* Add jni interface to use a binary hnsw index with faiss (#1747) * Fix memory leak on test code (#1776) --------- Signed-off-by: Heemin Kim <heemin@amazon.com>

heemin32 force-pushed the binary branch 5 times, most recently from 1e329c5 to 859c8a9 Compare June 12, 2024 21:44

heemin32 marked this pull request as ready for review June 12, 2024 21:44

heemin32 requested review from navneet1v, VijayanB, vamshin, jmazanec15, naveentatikonda, junqiu-lei, martin-gaievski, ryanbogan and luyuncheng as code owners June 12, 2024 21:44

navneet1v reviewed Jun 12, 2024

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

jni/src/faiss_wrapper.cpp Outdated Show resolved Hide resolved

src/main/java/org/opensearch/knn/jni/FaissService.java Outdated Show resolved Hide resolved

src/main/java/org/opensearch/knn/jni/FaissService.java Show resolved Hide resolved

This comment was marked as resolved.

Sign in to view

naveentatikonda reviewed Jun 12, 2024

View reviewed changes

src/testFixtures/java/org/opensearch/knn/TestUtils.java Show resolved Hide resolved

shatejas reviewed Jun 12, 2024

View reviewed changes

heemin32 force-pushed the binary branch 10 times, most recently from e91ec4a to a0cc58c Compare June 14, 2024 17:03

heemin32 requested a review from navneet1v June 24, 2024 05:25

shatejas reviewed Jun 24, 2024

View reviewed changes

heemin32 mentioned this pull request Jun 24, 2024

[FEATURE] Support of dynamic efsearch and nprobs value during search for binary index #1769

Closed

jmazanec15 reviewed Jun 24, 2024

View reviewed changes

heemin32 force-pushed the binary branch 2 times, most recently from d2e484f to 6345901 Compare June 24, 2024 19:49

heemin32 requested a review from shatejas June 24, 2024 22:11

Add jni interface to use a binary hnsw index with faiss

145d650

Signed-off-by: Heemin Kim <heemin@amazon.com>

heemin32 force-pushed the binary branch from 6345901 to 145d650 Compare June 24, 2024 22:51

heemin32 requested a review from jmazanec15 June 24, 2024 23:35

shatejas approved these changes Jun 25, 2024

View reviewed changes

Vikasht34 approved these changes Jun 25, 2024

View reviewed changes

navneet1v reviewed Jun 25, 2024

View reviewed changes

jni/src/faiss_wrapper.cpp Show resolved Hide resolved

navneet1v reviewed Jun 25, 2024

View reviewed changes

src/main/java/org/opensearch/knn/jni/JNIService.java Show resolved Hide resolved

jmazanec15 approved these changes Jun 25, 2024

View reviewed changes

heemin32 requested a review from navneet1v June 25, 2024 15:49

navneet1v approved these changes Jun 25, 2024

View reviewed changes

heemin32 merged commit fe14b21 into opensearch-project:feature/binary-format Jun 25, 2024
47 of 53 checks passed

heemin32 deleted the binary branch June 26, 2024 03:47

heemin32 added a commit that referenced this pull request Jul 2, 2024

Add jni interface to use a binary hnsw index with faiss (#1747)

37c4a9d

Signed-off-by: Heemin Kim <heemin@amazon.com>

heemin32 added a commit that referenced this pull request Jul 2, 2024

Add jni interface to use a binary hnsw index with faiss (#1747)

57a081e

Signed-off-by: Heemin Kim <heemin@amazon.com>

junqiu-lei pushed a commit to junqiu-lei/k-NN that referenced this pull request Jul 8, 2024

Add jni interface to use a binary hnsw index with faiss (opensearch-p…

04ca90b

…roject#1747) Signed-off-by: Heemin Kim <heemin@amazon.com>

junqiu-lei pushed a commit to junqiu-lei/k-NN that referenced this pull request Jul 9, 2024

Add jni interface to use a binary hnsw index with faiss (opensearch-p…

b3bbe9e

…roject#1747) Signed-off-by: Heemin Kim <heemin@amazon.com>

heemin32 mentioned this pull request Jul 11, 2024

[Backport 2.x]Add jni interface to use a binary hnsw index with faiss #1817

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add jni interface to use a binary hnsw index with faiss #1747

Add jni interface to use a binary hnsw index with faiss #1747

heemin32 commented Jun 11, 2024 •

edited

Loading

This comment was marked as resolved.

shatejas left a comment

heemin32 commented Jun 13, 2024

shatejas left a comment

shatejas Jun 24, 2024

heemin32 Jun 24, 2024

shatejas Jun 24, 2024

heemin32 Jun 24, 2024

jmazanec15 left a comment

jmazanec15 Jun 24, 2024

jmazanec15 Jun 24, 2024

heemin32 Jun 24, 2024

jmazanec15 Jun 24, 2024

heemin32 Jun 24, 2024 •

edited

Loading

navneet1v Jun 25, 2024 •

edited

Loading

heemin32 Jun 25, 2024

navneet1v Jun 25, 2024

heemin32 Jun 25, 2024

Add jni interface to use a binary hnsw index with faiss #1747

Add jni interface to use a binary hnsw index with faiss #1747

Conversation

heemin32 commented Jun 11, 2024 • edited Loading

Description

Issues Resolved

Check List

This comment was marked as resolved.

shatejas left a comment

Choose a reason for hiding this comment

heemin32 commented Jun 13, 2024

shatejas left a 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

jmazanec15 left a 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

heemin32 Jun 24, 2024 • edited Loading

Choose a reason for hiding this comment

navneet1v Jun 25, 2024 • edited Loading

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

heemin32 commented Jun 11, 2024 •

edited

Loading

heemin32 Jun 24, 2024 •

edited

Loading

navneet1v Jun 25, 2024 •

edited

Loading