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

Adds ef_search as a query parameter for Lucene, FAISS and NMSLIB #1783

Merged
merged 1 commit into from
Jul 2, 2024
Merged

Adds ef_search as a query parameter for Lucene, FAISS and NMSLIB #1783

merged 1 commit into from
Jul 2, 2024

Conversation

shatejas
Copy link
Contributor

@shatejas shatejas commented Jul 2, 2024 •
edited
Loading

Currently ef_search is set at index level, this change gives the ability to have query time ef-search parameter without manipulating the index settings. query time value supersedes the value from index in FAISS and NMSLIB. For Lucene, max of k and ef_search is used as ef_search value

Description

  • Squashed commits from feature branch
    • Commits not included: Radial search and Nprobes related. Those will be squashed and put on top of these
  • Changed version from 3 to 2.16 for ef_search and method_parameters

Issues Resolved

1537

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.

@shatejas shatejas force-pushed the main branch 3 times, most recently from 37ef976 to 9db146b Compare July 2, 2024 01:54
jmazanec15
jmazanec15 reviewed Jul 2, 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.

Few comments on cpp side. But overall looks good. Going to look at Java side later.

jni/src/faiss_wrapper.cpp Outdated Show resolved Hide resolved
jni/src/nmslib_wrapper.cpp Outdated Show resolved Hide resolved
jni/src/nmslib_wrapper.cpp Outdated Show resolved Hide resolved
jni/src/nmslib_wrapper.cpp Outdated Show resolved Hide resolved
jni/tests/faiss_wrapper_test.cpp Outdated Show resolved Hide resolved
jni/tests/faiss_wrapper_unit_test.cpp Show resolved Hide resolved
jmazanec15
jmazanec15 reviewed Jul 2, 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.

Reviewed Java side. Looks good.

shatejas
shatejas commented Jul 2, 2024
View reviewed changes
Copy link
Contributor Author

@shatejas shatejas left a comment

Choose a reason for hiding this comment

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

This is simply merging the feature branch after squashing the commits. Just FYI if it wasn't obvious from the description

jni/src/nmslib_wrapper.cpp Outdated Show resolved Hide resolved
jni/src/nmslib_wrapper.cpp Outdated Show resolved Hide resolved
@shatejas
Adds ef_search as a query parameter for Lucene, FAISS and NMSLIB
d097029 
Currently ef_search is set at index level, this change gives the ability
to have query time ef-search parameter without manipulating the index
settings. query time value supersedes the value from index in FAISS and
NMSLIB. For Lucene, max of k and ef_search is used as ef_search value

Signed-off-by: Tejas Shah <[email protected]>
@shatejas shatejas requested a review from heemin32 July 2, 2024 19:15
@heemin32 heemin32 merged commit 989ad7d into opensearch-project:main Jul 2, 2024
49 of 55 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-1783-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 989ad7d3df39a38cb0afc828ae98741676064ee0
# Push it to GitHub
git push --set-upstream origin backport/backport-1783-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-1783-to-2.x.

shatejas added a commit to shatejas/k-NN that referenced this pull request Jul 3, 2024
@shatejas
Adds ef_search as a query parameter for Lucene, FAISS and NMSLIB (ope…
9aec972 
…nsearch-project#1783)

Currently ef_search is set at index level, this change gives the ability
to have query time ef-search parameter without manipulating the index
settings. query time value supersedes the value from index in FAISS and
NMSLIB. For Lucene, max of k and ef_search is used as ef_search value

Signed-off-by: Tejas Shah <[email protected]>
(cherry picked from commit 989ad7d)
shatejas added a commit to shatejas/k-NN that referenced this pull request Jul 3, 2024
@shatejas
Adds ef_search as a query parameter for Lucene, FAISS and NMSLIB (ope…
17e9d89 
…nsearch-project#1783)

Currently ef_search is set at index level, this change gives the ability
to have query time ef-search parameter without manipulating the index
settings. query time value supersedes the value from index in FAISS and
NMSLIB. For Lucene, max of k and ef_search is used as ef_search value

Signed-off-by: Tejas Shah <[email protected]>
(cherry picked from commit 989ad7d)
shatejas added a commit to shatejas/k-NN that referenced this pull request Jul 3, 2024
@shatejas
Adds ef_search as a query parameter for Lucene, FAISS and NMSLIB (ope…
385dd86 
…nsearch-project#1783)

Currently ef_search is set at index level, this change gives the ability
to have query time ef-search parameter without manipulating the index
settings. query time value supersedes the value from index in FAISS and
NMSLIB. For Lucene, max of k and ef_search is used as ef_search value

Signed-off-by: Tejas Shah <[email protected]>
(cherry picked from commit 989ad7d)
shatejas added a commit to shatejas/k-NN that referenced this pull request Jul 3, 2024
@shatejas
Adds ef_search as a query parameter for Lucene, FAISS and NMSLIB (ope…
03b0577 
…nsearch-project#1783)

Currently ef_search is set at index level, this change gives the ability
to have query time ef-search parameter without manipulating the index
settings. query time value supersedes the value from index in FAISS and
NMSLIB. For Lucene, max of k and ef_search is used as ef_search value

Signed-off-by: Tejas Shah <[email protected]>
(cherry picked from commit 989ad7d)
shatejas added a commit to shatejas/k-NN that referenced this pull request Jul 3, 2024
@shatejas
Adds ef_search as a query parameter for Lucene, FAISS and NMSLIB (ope…
4579788 
…nsearch-project#1783)

Currently ef_search is set at index level, this change gives the ability
to have query time ef-search parameter without manipulating the index
settings. query time value supersedes the value from index in FAISS and
NMSLIB. For Lucene, max of k and ef_search is used as ef_search value

Signed-off-by: Tejas Shah <[email protected]>
shatejas added a commit to shatejas/k-NN that referenced this pull request Jul 3, 2024
@shatejas
Adds ef_search as a query parameter for Lucene, FAISS and NMSLIB (ope…
5cc0163 
…nsearch-project#1783)

Currently ef_search is set at index level, this change gives the ability
to have query time ef-search parameter without manipulating the index
settings. query time value supersedes the value from index in FAISS and
NMSLIB. For Lucene, max of k and ef_search is used as ef_search value

Signed-off-by: Tejas Shah <[email protected]>
shatejas added a commit to shatejas/k-NN that referenced this pull request Jul 3, 2024
@shatejas
Adds ef_search as a query parameter for Lucene, FAISS and NMSLIB (ope…
cbc2875 
…nsearch-project#1783)

Currently ef_search is set at index level, this change gives the ability
to have query time ef-search parameter without manipulating the index
settings. query time value supersedes the value from index in FAISS and
NMSLIB. For Lucene, max of k and ef_search is used as ef_search value

Signed-off-by: Tejas Shah <[email protected]>
shatejas added a commit to shatejas/k-NN that referenced this pull request Jul 3, 2024
@shatejas
Adds ef_search as a query parameter for Lucene, FAISS and NMSLIB (ope…
5a6a05d 
…nsearch-project#1783)

Currently ef_search is set at index level, this change gives the ability
to have query time ef-search parameter without manipulating the index
settings. query time value supersedes the value from index in FAISS and
NMSLIB. For Lucene, max of k and ef_search is used as ef_search value

Signed-off-by: Tejas Shah <[email protected]>
shatejas added a commit to shatejas/k-NN that referenced this pull request Jul 3, 2024
@shatejas
Adds ef_search as a query parameter for Lucene, FAISS and NMSLIB (ope…
74a4c56 
…nsearch-project#1783)

Currently ef_search is set at index level, this change gives the ability
to have query time ef-search parameter without manipulating the index
settings. query time value supersedes the value from index in FAISS and
NMSLIB. For Lucene, max of k and ef_search is used as ef_search value

Signed-off-by: Tejas Shah <[email protected]>
shatejas added a commit to shatejas/k-NN that referenced this pull request Jul 3, 2024
@shatejas
Adds ef_search as a query parameter for Lucene, FAISS and NMSLIB (ope…
4809d86 
…nsearch-project#1783)

Currently ef_search is set at index level, this change gives the ability
to have query time ef-search parameter without manipulating the index
settings. query time value supersedes the value from index in FAISS and
NMSLIB. For Lucene, max of k and ef_search is used as ef_search value

Signed-off-by: Tejas Shah <[email protected]>
shatejas added a commit to shatejas/k-NN that referenced this pull request Jul 3, 2024
@shatejas
Adds ef_search as a query parameter for Lucene, FAISS and NMSLIB (ope…
76db36f 
…nsearch-project#1783)

Currently ef_search is set at index level, this change gives the ability
to have query time ef-search parameter without manipulating the index
settings. query time value supersedes the value from index in FAISS and
NMSLIB. For Lucene, max of k and ef_search is used as ef_search value

Signed-off-by: Tejas Shah <[email protected]>
navneet1v pushed a commit that referenced this pull request Jul 4, 2024
@shatejas
Adds ef_search as a query parameter for Lucene, FAISS and NMSLIB (#1783…
946b99f 
…) (#1791)

Currently ef_search is set at index level, this change gives the ability
to have query time ef-search parameter without manipulating the index
settings. query time value supersedes the value from index in FAISS and
NMSLIB. For Lucene, max of k and ef_search is used as ef_search value

Signed-off-by: Tejas Shah <[email protected]>
@vibrantvarun vibrantvarun mentioned this pull request Jul 4, 2024
Closed
luyuncheng pushed a commit to luyuncheng/k-NN-1 that referenced this pull request Jul 7, 2024
@shatejas @luyuncheng
Adds ef_search as a query parameter for Lucene, FAISS and NMSLIB (ope…
ec69718 
…nsearch-project#1783)

Currently ef_search is set at index level, this change gives the ability
to have query time ef-search parameter without manipulating the index
settings. query time value supersedes the value from index in FAISS and
NMSLIB. For Lucene, max of k and ef_search is used as ef_search value

Signed-off-by: Tejas Shah <[email protected]>
luyuncheng pushed a commit to luyuncheng/k-NN-1 that referenced this pull request Jul 7, 2024
@shatejas @luyuncheng
Adds ef_search as a query parameter for Lucene, FAISS and NMSLIB (ope…
7e95e16 
…nsearch-project#1783)

Currently ef_search is set at index level, this change gives the ability
to have query time ef-search parameter without manipulating the index
settings. query time value supersedes the value from index in FAISS and
NMSLIB. For Lucene, max of k and ef_search is used as ef_search value

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

@heemin32 heemin32 heemin32 approved these changes

@jmazanec15 jmazanec15 jmazanec15 approved these changes

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

@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

@junqiu-lei junqiu-lei Awaiting requested review from junqiu-lei junqiu-lei 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

Assignees
No one assigned
Labels
backport 2.x v2.16.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@shatejas @heemin32 @jmazanec15