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.

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

[Backport 1.2] [BUG FIX] Add space type default and ef search parameter in warmup #285

Merged

Conversation

jmazanec15
Copy link
Member

Description

Sets the default value of space type to L2 in KNNIndexShard.
KNNIndexShard is used during warmup to load segments into memory. For
indices created in ES 7.1 and 7.4, they will not have this value set
because the only space we supported was l2. So, we need to hardcode the
defaults here.

For nmslib, the ef_search parameter is configurable at load time. So, it
needs to be passed as a parameter in both the search load phase as well
as warmup. This commit adds it to the warmup phase and abstracts load
parameters to a helper function so that it can be consistent for both
search and warmup.

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.

…pensearch-project#276)

Sets the default value of space type to L2 in KNNIndexShard.
KNNIndexShard is used during warmup to load segments into memory. For
indices created in ES 7.1 and 7.4, they will not have this value set
because the only space we supported was l2. So, we need to hardcode the
defaults here.

For nmslib, the ef_search parameter is configurable at load time. So, it
needs to be passed as a parameter in both the search load phase as well
as warmup. This commit adds it to the warmup phase and abstracts load
parameters to a helper function so that it can be consistent for both
search and warmup.

Signed-off-by: John Mazanec <[email protected]>
(cherry picked from commit 2fb2ad1)
@martin-gaievski martin-gaievski self-requested a review February 10, 2022 20:36
Copy link
Member

@martin-gaievski martin-gaievski left a comment

Choose a reason for hiding this comment

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

Code looks good, please check the security findings from checks, looks like OpenSearch related issues

@codecov-commenter
Copy link

Codecov Report

Merging #285 (12f97ee) into 1.2 (374eb46) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##                1.2     #285   +/-   ##
=========================================
  Coverage     83.21%   83.22%           
- Complexity      864      865    +1     
=========================================
  Files           123      123           
  Lines          3778     3779    +1     
  Branches        358      358           
=========================================
+ Hits           3144     3145    +1     
  Misses          474      474           
  Partials        160      160           
Impacted Files Coverage Δ
.../main/java/org/opensearch/knn/index/IndexUtil.java 56.66% <100.00%> (+3.93%) ⬆️
...n/java/org/opensearch/knn/index/KNNIndexShard.java 93.02% <100.00%> (+0.16%) ⬆️
.../main/java/org/opensearch/knn/index/KNNWeight.java 75.00% <100.00%> (-1.63%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 374eb46...12f97ee. Read the comment docs.

@jmazanec15
Copy link
Member Author

jmazanec15 commented Feb 10, 2022

Jackson fix here: opensearch-project/OpenSearch#1982. Doesnt look to be backported.

Junit seems to be fixed here: opensearch-project/OpenSearch#1888. Should be backported to 1.2. Not sure why 1.2 isnt picking this up here.

@jmazanec15 jmazanec15 merged commit af3c80a into opensearch-project:1.2 Feb 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants