Add patch to support multi vector in faiss #1358

heemin32 · 2023-12-20T22:54:43Z

Description

Add patch to support multi vector in faiss

Introduced an extension point in the way of collecting search result in HNSW algorithm. ResultCollectorFactory is passed as part of a search parameter. For each search inside faiss library, it create a new ResultCollector using the factory. The ResultCollector's collect method is used to collect search data.

This enable us to pass custom ResultCollector with which we can dedupe vector on its parent Id so that exact k result of original document can be returned for nested field.

PR for patch file to make it easy to review. https://github.com/heemin32/faiss/pull/1/files

Issues Resolved

#1065

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.

codecov · 2023-12-21T05:00:37Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (2e3ab95) 85.15% compared to head (f9fe53b) 85.04%.
Report is 3 commits behind head on feature/multi-vector.

❗ Current head f9fe53b differs from pull request most recent head 4c936f9. Consider uploading reports for the commit 4c936f9 to get more accurate results

Additional details and impacted files

@@                    Coverage Diff                     @@
##             feature/multi-vector    #1358      +/-   ##
==========================================================
- Coverage                   85.15%   85.04%   -0.12%     
- Complexity                   1216     1248      +32     
==========================================================
  Files                         160      162       +2     
  Lines                        4958     5101     +143     
  Branches                      457      477      +20     
==========================================================
+ Hits                         4222     4338     +116     
- Misses                        538      557      +19     
- Partials                      198      206       +8

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

ryanbogan

LGTM!

jmazanec15 · 2023-12-21T23:11:57Z

Im wondering if for patches we can raise a PR against our personal forks of faiss and link them in the PR so we can review the code changes as opposed to review via git diff. Not sure if there is a better approach. What do you think @heemin32 ?

jmazanec15 · 2023-12-21T23:07:18Z

.github/workflows/CI.yml

@@ -38,6 +38,16 @@ jobs:
        with:
          submodules: true

+      # Git functionality in CMAKE file does not work with given ubuntu image. Therefore, handling it here.


What is meant by this? If we do this, everytime we add a patch we will need to update this.

Correct. Git command in CMAKE file like git submodule update --init -- or git apply does not work for ubuntu. Need to talk with @peterzhuamazon to see if we can fix it by using different ubuntu image.

@heemin32 what was the conclusion for this?

The code is needed as of now to make it work. Will talk to @peterzhuamazon and create an issue in opensearch build repo to resolve the problem outside of this PR.

is that discussion blocking this PR?

jmazanec15 · 2023-12-21T23:07:31Z

.github/workflows/CI.yml

+          cd jni/external/faiss
+          git config --global core.autocrlf input
+          git apply --ignore-space-change --ignore-whitespace --3way ../../patches/faiss/0001-Custom-patch-to-support-multi-vector.patch
+          rm ../../patches/faiss/0001-Custom-patch-to-support-multi-vector.patch


Why remove?

Commented in line #42. # Deleting file at the end to skip git apply inside CMAKE file

.github/workflows/CI.yml

jmazanec15 · 2023-12-21T23:08:21Z

CHANGELOG.md

@@ -28,3 +28,4 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 * Upgrade urllib to 1.26.18 [#1319](https://github.com/opensearch-project/k-NN/pull/1319)
 * Upgrade guava to 32.1.3 [#1319](https://github.com/opensearch-project/k-NN/pull/1319)
 ### Refactoring
+* Add patch to support multi vector in faiss [#1358](https://github.com/opensearch-project/k-NN/pull/1358)


Is this a refactor or a feature?

I put it as refactoring because this PR itself does not provide any new feature to users. However, I added CHANGELOG because it might have a impact in performance of current feature and we want to catch the root cause easily if that happen.

@heemin32 what I would say lets create a feature branch for this change and add all the changes in that feature branch. Then apply a single commit in main branch as part of 1 PR. This will ensure that this feature is packed in 1 PR. This is similar to what I did for Faiss Filtering also. It can avoid comments like this.

Changed to feature branch.

jmazanec15 · 2023-12-21T23:08:51Z

jni/CMakeLists.txt

@@ -79,7 +86,7 @@ list(APPEND TARGET_LIBS ${TARGET_LIB_COMMON})
 # ---------------------------------- NMSLIB ----------------------------------
 if (${CONFIG_NMSLIB} STREQUAL ON OR ${CONFIG_ALL} STREQUAL ON OR ${CONFIG_TEST} STREQUAL ON)
    # Check if nmslib exists
-    find_path(NMS_REPO_DIR NAMES similarity_search PATHS ${CMAKE_CURRENT_SOURCE_DIR}/external/nmslib)
+    find_path(NMS_REPO_DIR NAMES similarity_search PATHS ${CMAKE_CURRENT_SOURCE_DIR}/external/nmslib NO_DEFAULT_PATH)


Why is this needed?

In mac, I have faiss in /opt/homebrew/share and the command fail to pull faiss submodule. I don't have nmslib but thought that there is no harm on doing same for nmslib.

jmazanec15 · 2023-12-21T23:09:23Z

jni/CMakeLists.txt

+    # If it exists, apply patches
+    if (EXISTS ${PATCH_FILE})
+        message(STATUS "Applying custom patches.")
+        execute_process(COMMAND git apply --ignore-space-change --ignore-whitespace --3way ${CMAKE_CURRENT_SOURCE_DIR}/patches/faiss/0001-Custom-patch-to-support-multi-vector.patch WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/external/faiss ERROR_VARIABLE ERROR_MSG RESULT_VARIABLE RESULT_CODE)


Can we loop over patches?

There won't be many patches I hope. Also, explicit applying would be better to make sure that we are applying all intended patches.

+1 we should apply patches explicitly.

jmazanec15 · 2023-12-21T23:10:36Z

jni/patches/faiss/0001-Custom-patch-to-support-multi-vector.patch

@@ -0,0 +1,221 @@
+From 864c1abe7bdced5d306e871ea2bd73e1e35987fd Mon Sep 17 00:00:00 2001


Can we add a README about patches? Or update DEV guide? Want to make sure we know what we are doing on lib upgrades.

Let me add in DEV guide on how to generate patches or how to update patches.

heemin32 · 2023-12-21T23:25:08Z

Im wondering if for patches we can raise a PR against our personal forks of faiss and link them in the PR so we can review the code changes as opposed to review via git diff. Not sure if there is a better approach. What do you think @heemin32 ?

Good Idea. Added in PR description.
Adding here as well for your convenience. https://github.com/heemin32/faiss/pull/1/files

heemin32 · 2023-12-23T00:09:06Z

Test for patch completed successfully. facebookresearch/faiss#3178

navneet1v · 2023-12-27T18:18:56Z

Overall code looks good to me. Please resolve all the comments before I can approve the PR

Signed-off-by: Heemin Kim <[email protected]>

* Add patch to support multi vector in faiss (#1358) Signed-off-by: Heemin Kim <[email protected]> * Initialize id_map as null (#1363) Signed-off-by: Heemin Kim <[email protected]> * Add support of multi vector in jni (#1364) Signed-off-by: Heemin Kim <[email protected]> * Multi vector support for Faiss HNSW (#1371) Apply the parentId filter to the Faiss HNSW search method. This ensures that documents are deduplicated based on their parentId, and the method returns k results for documents with nested fields. Signed-off-by: Heemin Kim <[email protected]> * Add data generation script for nested field (#1388) Signed-off-by: Heemin Kim <[email protected]> * Add perf test for nested field (#1394) Signed-off-by: Heemin Kim <[email protected]> --------- Signed-off-by: Heemin Kim <[email protected]>

Signed-off-by: Heemin Kim <[email protected]>

* Add patch to support multi vector in faiss (opensearch-project#1358) Signed-off-by: Heemin Kim <[email protected]> * Initialize id_map as null (opensearch-project#1363) Signed-off-by: Heemin Kim <[email protected]> * Add support of multi vector in jni (opensearch-project#1364) Signed-off-by: Heemin Kim <[email protected]> * Multi vector support for Faiss HNSW (opensearch-project#1371) Apply the parentId filter to the Faiss HNSW search method. This ensures that documents are deduplicated based on their parentId, and the method returns k results for documents with nested fields. Signed-off-by: Heemin Kim <[email protected]> * Add data generation script for nested field (opensearch-project#1388) Signed-off-by: Heemin Kim <[email protected]> * Add perf test for nested field (opensearch-project#1394) Signed-off-by: Heemin Kim <[email protected]> --------- Signed-off-by: Heemin Kim <[email protected]> (cherry picked from commit 709b448)

* Add patch to support multi vector in faiss (#1358) Signed-off-by: Heemin Kim <[email protected]> * Initialize id_map as null (#1363) Signed-off-by: Heemin Kim <[email protected]> * Add support of multi vector in jni (#1364) Signed-off-by: Heemin Kim <[email protected]> * Multi vector support for Faiss HNSW (#1371) Apply the parentId filter to the Faiss HNSW search method. This ensures that documents are deduplicated based on their parentId, and the method returns k results for documents with nested fields. Signed-off-by: Heemin Kim <[email protected]> * Add data generation script for nested field (#1388) Signed-off-by: Heemin Kim <[email protected]> * Add perf test for nested field (#1394) Signed-off-by: Heemin Kim <[email protected]> --------- Signed-off-by: Heemin Kim <[email protected]> (cherry picked from commit 709b448)

heemin32 requested review from navneet1v, VijayanB, vamshin, jmazanec15, naveentatikonda, junqiu-lei, martin-gaievski and ryanbogan as code owners December 20, 2023 22:54

heemin32 force-pushed the patch branch 13 times, most recently from 81b29e2 to 610c0dc Compare December 21, 2023 04:45

heemin32 added backport 2.x v2.12.0 Refactoring Improve the design, structure, and implementation while preserving its functionality labels Dec 21, 2023

ryanbogan previously approved these changes Dec 21, 2023

View reviewed changes

heemin32 dismissed ryanbogan’s stale review via 6fa9343 December 21, 2023 17:58

heemin32 force-pushed the patch branch from 610c0dc to 6fa9343 Compare December 21, 2023 17:58

jmazanec15 reviewed Dec 21, 2023

View reviewed changes

heemin32 force-pushed the patch branch from 6fa9343 to 7acf5d2 Compare December 22, 2023 00:13

heemin32 mentioned this pull request Dec 22, 2023

Custom patch to support multi-vector heemin32/faiss#1

Closed

heemin32 force-pushed the patch branch from 7acf5d2 to 2e0a53d Compare December 22, 2023 00:36

heemin32 changed the base branch from main to feature/multi-vector December 22, 2023 06:26

heemin32 added the skip-changelog label Dec 22, 2023

heemin32 force-pushed the patch branch from 2e0a53d to dd83779 Compare December 22, 2023 06:28

heemin32 removed Refactoring Improve the design, structure, and implementation while preserving its functionality backport 2.x labels Dec 22, 2023

heemin32 force-pushed the patch branch from dd83779 to 5bc6606 Compare December 22, 2023 18:20

heemin32 requested a review from jmazanec15 December 22, 2023 18:20

heemin32 force-pushed the patch branch 2 times, most recently from de7e818 to 5e8c3eb Compare December 22, 2023 22:18

heemin32 force-pushed the patch branch from 5e8c3eb to f9fe53b Compare December 27, 2023 19:49

Add patch to support multi vector in faiss

4c936f9

Signed-off-by: Heemin Kim <[email protected]>

heemin32 force-pushed the patch branch from f9fe53b to 4c936f9 Compare December 27, 2023 20:40

jmazanec15 approved these changes Dec 27, 2023

View reviewed changes

navneet1v approved these changes Dec 27, 2023

View reviewed changes

heemin32 merged commit 8631654 into opensearch-project:feature/multi-vector Dec 27, 2023
26 of 45 checks passed

heemin32 added a commit to heemin32/k-NN that referenced this pull request Jan 5, 2024

Add patch to support multi vector in faiss (opensearch-project#1358)

44b202f

Signed-off-by: Heemin Kim <[email protected]>

heemin32 added a commit to heemin32/k-NN that referenced this pull request Jan 16, 2024

Add patch to support multi vector in faiss (opensearch-project#1358)

04a46c4

Signed-off-by: Heemin Kim <[email protected]>

heemin32 added a commit that referenced this pull request Jan 16, 2024

Add patch to support multi vector in faiss (#1358)

eb3fbd7

Signed-off-by: Heemin Kim <[email protected]>

heemin32 added a commit that referenced this pull request Jan 19, 2024

Add patch to support multi vector in faiss (#1358)

05e5be1

Signed-off-by: Heemin Kim <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add patch to support multi vector in faiss #1358

Add patch to support multi vector in faiss #1358

heemin32 commented Dec 20, 2023 •

edited

Loading

codecov bot commented Dec 21, 2023 •

edited

Loading

ryanbogan left a comment

jmazanec15 commented Dec 21, 2023

jmazanec15 Dec 21, 2023

heemin32 Dec 21, 2023

navneet1v Dec 27, 2023

heemin32 Dec 27, 2023 •

edited

Loading

navneet1v Dec 27, 2023

heemin32 Dec 27, 2023

jmazanec15 Dec 21, 2023

heemin32 Dec 21, 2023

jmazanec15 Dec 21, 2023

heemin32 Dec 21, 2023

navneet1v Dec 22, 2023

heemin32 Dec 22, 2023

jmazanec15 Dec 21, 2023

heemin32 Dec 21, 2023

jmazanec15 Dec 21, 2023

heemin32 Dec 21, 2023

navneet1v Dec 27, 2023

jmazanec15 Dec 21, 2023

heemin32 Dec 21, 2023

heemin32 commented Dec 21, 2023 •

edited

Loading

heemin32 commented Dec 23, 2023

navneet1v commented Dec 27, 2023

		@@ -0,0 +1,221 @@
		From 864c1abe7bdced5d306e871ea2bd73e1e35987fd Mon Sep 17 00:00:00 2001

Add patch to support multi vector in faiss #1358

Add patch to support multi vector in faiss #1358

Conversation

heemin32 commented Dec 20, 2023 • edited Loading

Description

Issues Resolved

Check List

codecov bot commented Dec 21, 2023 • edited Loading

Codecov Report

ryanbogan left a comment

Choose a reason for hiding this comment

jmazanec15 commented Dec 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heemin32 Dec 27, 2023 • 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heemin32 commented Dec 21, 2023 • edited Loading

heemin32 commented Dec 23, 2023

navneet1v commented Dec 27, 2023

heemin32 commented Dec 20, 2023 •

edited

Loading

codecov bot commented Dec 21, 2023 •

edited

Loading

heemin32 Dec 27, 2023 •

edited

Loading

heemin32 commented Dec 21, 2023 •

edited

Loading