Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 2 commits into from
Jul 3, 2024
Merged

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

merged 2 commits into from
Jul 3, 2024

Conversation

heemin32
Copy link
Collaborator

Description

It is safe to merge to main and 2.x as there is no user impact with this PR

Issues Resolved

#1767

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.

junqiu-lei
junqiu-lei previously approved these changes Jun 30, 2024
View reviewed changes
jmazanec15
jmazanec15 reviewed Jul 1, 2024
View reviewed changes
Copy link
Member

@jmazanec15 jmazanec15 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overall, looks pretty good. One minor comment on ChangeLog

CHANGELOG.md Outdated Show resolved Hide resolved
@shatejas
Copy link
Collaborator

shatejas commented Jul 1, 2024 •
edited
Loading

Why is there a need to merge in main when the feature is not ready/users cannot use it? Can't you rebase the changes from main in the feature branch?

@heemin32
Copy link
Collaborator Author

heemin32 commented Jul 1, 2024 •
edited
Loading

Why is there a need to merge in main when the feature is not ready/users cannot use it? Can't you rebase the changes from main in the feature branch?

This PR can be seen as an isolated works from any feature; Opening up binary index method of faiss through JNI, and JNI layer refactoring.

@heemin32 heemin32 changed the title JNI layer interface to support Faiss binary index Added the support for Binary Index in kNNEngine. Jul 1, 2024
@heemin32 heemin32 changed the title Added the support for Binary Index in kNNEngine. Added the support for Binary Index in kNNEngine Jul 1, 2024
@heemin32 heemin32 force-pushed the feature/binary-format branch 2 times, most recently from 3898397 to 4befe2b Compare July 1, 2024 21:32
@heemin32 heemin32 changed the title Added the support for Binary Index in kNNEngine Add jni interface to use a binary hnsw index with faiss Jul 1, 2024
@heemin32 heemin32 requested review from jmazanec15 and junqiu-lei July 1, 2024 21:32
navneet1v
navneet1v previously approved these changes Jul 1, 2024
View reviewed changes
jmazanec15
jmazanec15 previously approved these changes Jul 1, 2024
View reviewed changes
@heemin32 heemin32 dismissed stale reviews from jmazanec15 and navneet1v via f8a2f9f July 2, 2024 23:35
@heemin32 heemin32 force-pushed the feature/binary-format branch 6 times, most recently from 10a0938 to 34f8ec4 Compare July 3, 2024 13:48
@heemin32 heemin32 force-pushed the feature/binary-format branch from 34f8ec4 to dafd79b Compare July 3, 2024 14:41
@heemin32
Copy link
Collaborator Author

heemin32 commented Jul 3, 2024

Rebased from main branch

@heemin32 heemin32 requested review from navneet1v and jmazanec15 July 3, 2024 15:09
@heemin32 heemin32 merged commit a913082 into main Jul 3, 2024
92 of 102 checks passed
@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.x 2.x
# Navigate to the new working tree
cd .worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-1778-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 a91308235d2b5652a8a75ae5f43501b805c82234
# Push it to GitHub
git push --set-upstream origin backport/backport-1778-to-2.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-1778-to-2.x.

luyuncheng pushed a commit to luyuncheng/k-NN-1 that referenced this pull request Jul 7, 2024
@heemin32 @luyuncheng
Add jni interface to use a binary hnsw index with faiss (opensearch-p…
6685ecf 
…roject#1778)

* Add jni interface to use a binary hnsw index with faiss (opensearch-project#1747)

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

* Fix memory leak on test code (opensearch-project#1776)

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

---------

Signed-off-by: Heemin Kim <[email protected]>
luyuncheng pushed a commit to luyuncheng/k-NN-1 that referenced this pull request Jul 7, 2024
@heemin32 @luyuncheng
Add jni interface to use a binary hnsw index with faiss (opensearch-p…
083f668 
…roject#1778)

* Add jni interface to use a binary hnsw index with faiss (opensearch-project#1747)

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

* Fix memory leak on test code (opensearch-project#1776)

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

---------

Signed-off-by: Heemin Kim <[email protected]>
heemin32 added a commit to heemin32/k-NN that referenced this pull request Jul 11, 2024
@heemin32
Add jni interface to use a binary hnsw index with faiss (opensearch-p…
1a10d06 
…roject#1778)

* Add jni interface to use a binary hnsw index with faiss (opensearch-project#1747)

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

* Fix memory leak on test code (opensearch-project#1776)

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

---------

Signed-off-by: Heemin Kim <[email protected]>
@heemin32 heemin32 mentioned this pull request Jul 11, 2024
Merged
heemin32 added a commit that referenced this pull request Jul 11, 2024
@heemin32
Add jni interface to use a binary hnsw index with faiss (#1778) (#1817)
8b63d1d 
* Add jni interface to use a binary hnsw index with faiss (#1747)



* Fix memory leak on test code (#1776)



---------

Signed-off-by: Heemin Kim <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmazanec15 jmazanec15 jmazanec15 approved these changes

@navneet1v navneet1v navneet1v approved these changes

@VijayanB VijayanB Awaiting requested review from VijayanB VijayanB is a code owner

@vamshin vamshin Awaiting requested review from vamshin vamshin is a code owner

@naveentatikonda naveentatikonda Awaiting requested review from naveentatikonda naveentatikonda is a code owner

@martin-gaievski martin-gaievski Awaiting requested review from martin-gaievski martin-gaievski is a code owner

@ryanbogan ryanbogan Awaiting requested review from ryanbogan ryanbogan is a code owner

@luyuncheng luyuncheng Awaiting requested review from luyuncheng luyuncheng is a code owner

@junqiu-lei junqiu-lei Awaiting requested review from junqiu-lei junqiu-lei is a code owner

Assignees
No one assigned
Labels
backport 2.x skip-changelog
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@heemin32 @shatejas @navneet1v @jmazanec15 @junqiu-lei