Bump faiss lib commit to 32f0e8cf92cd2275b60364517bb1cce67aa29a55 #1443
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
heemin32
requested review from
navneet1v,
VijayanB,
vamshin,
jmazanec15,
naveentatikonda,
junqiu-lei,
martin-gaievski and
ryanbogan
as code owners
heemin32
force-pushed
the
faiss-conflict
branch
from
b14f238 to
7a77532
Compare
|
@heemin32 Can we hold this PR from merging into main branch and 2.x until 2.12 code freeze ? |
heemin32
changed the title
heemin32
changed the title
heemin32
force-pushed
the
faiss-conflict
branch
from
7a77532 to
6069b90
Compare
We can. |
heemin32
added
backport 2.x
v2.13.0
Maintenance
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1443 +/- ##
============================================
+ Coverage 85.02% 85.05% +0.03%
Complexity 1277 1277
============================================
Files 167 167
Lines 5207 5207
Branches 493 493
============================================
+ Hits 4427 4429 +2
+ Misses 572 570 -2
Partials 208 208 ☔ View full report in Codecov by Sentry. |
navneet1v
reviewed
Feb 2, 2024
Comment on lines
+513
to
-518
| std::unique_ptr<faiss::IDGrouperBitmap> buildIDGrouperBitmap(knn_jni::JNIUtilInterface * jniUtil, JNIEnv *env, jintArray parentIdsJ, std::vector<uint64_t>* bitmap) { | ||
| int *parentIdsArray = jniUtil->GetIntArrayElements(env, parentIdsJ, nullptr); | ||
| int parentIdsLength = jniUtil->GetJavaIntArrayLength(env, parentIdsJ); | ||
| auto* parent_id_filter = new FixedBitSet(parentIdsArray, parentIdsLength); |
There was a problem hiding this comment.
we should pass parentIdsArray and parentIdsLength directly to this function rather than pass JNIUtilInterface, JNIEnv, parentIdsJ
There was a problem hiding this comment.
faiss_wrapper already has methods which uses JNI objects. Therefore, I moved the core logic into a new file faiss_util which will contain only methods agnostic to JNI.
heemin32
force-pushed
the
faiss-conflict
branch
3 times, most recently
from
e3bca97 to
896b58a
Compare
heemin32
force-pushed
the
faiss-conflict
branch
4 times, most recently
from
2794e0c to
a4f22e1
Compare
heemin32
force-pushed
the
faiss-conflict
branch
2 times, most recently
from
da6f77d to
1b09e12
Compare
Signed-off-by: Heemin Kim <[email protected]>
heemin32
force-pushed
the
faiss-conflict
branch
from
1b09e12 to
c674058
Compare
naveentatikonda
approved these changes
Feb 13, 2024
jmazanec15
approved these changes
Feb 13, 2024
opensearch-trigger-bot bot
pushed a commit
that referenced
this pull request
Feb 13, 2024
Signed-off-by: Heemin Kim <[email protected]> (cherry picked from commit ced1ff2)
heemin32
added a commit
that referenced
this pull request
Feb 13, 2024
Signed-off-by: Heemin Kim <[email protected]> (cherry picked from commit ced1ff2) Co-authored-by: Heemin Kim <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
In #1398, we introduced a custom patch on faiss library to support multi-vector feature. After that, faiss refactored HNSW by generalizing ResultHandler which caused a conflict with the previous custom patch and blocking us from updating faiss library beyond commit# 32f0e8cf92cd2275b60364517bb1cce67aa29a55.
This PR is to resolve the conflict and keep the supporting of multi-vector feature as it is on top of faiss's commit number 32f0e8cf92cd2275b60364517bb1cce67aa29a55 so that we are unblocked for future faiss library update.
Patch file is from facebookresearch/faiss#3227
Issues Resolved
#1409
Check List
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.