[BUG FIX] Add space type default and ef search parameter in warmup by jmazanec15 · Pull Request #276 · opensearch-project/k-NN

jmazanec15 · 2022-02-09T23:13:11Z

Description

Warmup is used to load native library files into memory so that there are no initial latency penalties during search. The load process for search and warmup are very similar.

Currently, warmup api has a similar bug to #267. It does not default to a space type. For customers upgrading to 1.2 from ES 7.1 or 7.4 this will cause warmup failure. This only applies to 1.2. 1.1 and 1.0 do not have this bug in warmup.

Additionally, for nmslib, we allow the ef_search parameter to be set after indexing. To do this, we pass the parameter in during loading. In 1.2, we did not pass this parameter in. This will cause default ef_search to be used during warmup. To clean this up a bit, I abstracted generating load parameters into a utility function and used the same one for both search as well as warmup.

Issues Resolved

#255

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.

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. Signed-off-by: John Mazanec <jmazane@amazon.com>

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 <jmazane@amazon.com>

Signed-off-by: John Mazanec <jmazane@amazon.com>

VijayanB · 2022-02-09T23:18:12Z

+     * @param indexName Name of OpenSearch index that the segment files belong to
+     * @return load parameters that will be passed to the JNI.
+     */
+    public static Map<String, Object> getLoadParameters(SpaceType spaceType, KNNEngine knnEngine, String indexName) {


may be: getParametersAtLoading ?

Makes sense. Will update.

junqiu-lei · 2022-02-09T23:25:03Z

            for (FieldInfo fieldInfo : reader.getFieldInfos()) {
                if (fieldInfo.attributes().containsKey(KNNVectorFieldMapper.KNN_FIELD)) {
-                    SpaceType spaceType = SpaceType.getSpace(fieldInfo.attributes().get(SPACE_TYPE));
+                    String spaceTypeName = fieldInfo.attributes().getOrDefault(SPACE_TYPE, SpaceType.L2.getValue());


How about to add a default value here to make more readable?

Suggested change

String spaceTypeName = fieldInfo.attributes().getOrDefault(SPACE_TYPE, SpaceType.L2.getValue());

String spaceTypeDefaultVaule = SpaceType.L2.getValue();

String spaceTypeName = fieldInfo.attributes().getOrDefault(SPACE_TYPE, spaceTypeDefaultVaule);

I would make default value a constant

That makes sense, I will update.

Either way is fine, but i am leaning towards what you have :) since method itself gives the hint that it is get or Default :)

Right, I think I will just keep what I have. So, we do have SpaceType.DEFAULT, but in this case I do not want to use that because I know for sure that this should be L2. I will update with a comment.

martin-gaievski · 2022-02-09T23:43:04Z

+     * @return load parameters that will be passed to the JNI.
+     */
+    public static Map<String, Object> getLoadParameters(SpaceType spaceType, KNNEngine knnEngine, String indexName) {
+        Map<String, Object> loadParameters = Maps.newHashMap(ImmutableMap.of(


Are we going to mutate this map later? Seems that at the end we return mutable object from this method.

Suggested change

Map<String, Object> loadParameters = Maps.newHashMap(ImmutableMap.of(

final Map<String, Object> loadParameters;

if (KNNEngine.NMSLIB.equals(knnEngine)) {

loadParameters = ImmutableMap.of(

SPACE_TYPE, spaceType.getValue(),

HNSW_ALGO_EF_SEARCH, KNNSettings.getEfSearchParam(indexName)

);

} else {

loadParameters = ImmutableMap.of(SPACE_TYPE, spaceType.getValue());

}

return loadParameters;

or we can just return UnModifiableMap.

Prefer the unmodifiable map approach. Will go with that.

martin-gaievski · 2022-02-09T23:44:49Z

            for (FieldInfo fieldInfo : reader.getFieldInfos()) {
                if (fieldInfo.attributes().containsKey(KNNVectorFieldMapper.KNN_FIELD)) {
-                    SpaceType spaceType = SpaceType.getSpace(fieldInfo.attributes().get(SPACE_TYPE));
+                    String spaceTypeName = fieldInfo.attributes().getOrDefault(SPACE_TYPE, SpaceType.L2.getValue());


I would make default value a constant

…spacetype-fix

Signed-off-by: John Mazanec <jmazane@amazon.com>

codecov-commenter · 2022-02-10T06:05:32Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.38%. Comparing base (18e6e35) to head (fd8bd5e).
⚠️ Report is 802 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##               main     #276   +/-   ##
=========================================
  Coverage     83.38%   83.38%           
- Complexity      884      885    +1     
=========================================
  Files           127      127           
  Lines          3833     3834    +1     
  Branches        361      361           
=========================================
+ Hits           3196     3197    +1     
  Misses          475      475           
  Partials        162      162

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

VijayanB

LGTM. Thanks!

) 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 <jmazane@amazon.com> (cherry picked from commit 2fb2ad1)

github-actions · 2022-02-10T18:26:35Z

The backport to 1.2 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-1.2 1.2
# Navigate to the new working tree
cd .worktrees/backport-1.2
# Create a new branch
git switch --create backport/backport-276-to-1.2
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 2fb2ad116bc11bde3eab5695aed65392943c08ae
# Push it to GitHub
git push --set-upstream origin backport/backport-276-to-1.2
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-1.2

Then, create a pull request where the base branch is 1.2 and the compare/head branch is backport/backport-276-to-1.2.

…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 <jmazane@amazon.com> (cherry picked from commit 2fb2ad1)

) (#285) 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 <jmazane@amazon.com> (cherry picked from commit 2fb2ad1)

…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 <jmazane@amazon.com>

jmazanec15 added 3 commits February 9, 2022 13:36

Add javadocs and remove unused import

769b263

Signed-off-by: John Mazanec <jmazane@amazon.com>

jmazanec15 requested a review from a team February 9, 2022 23:13

VijayanB reviewed Feb 9, 2022

View reviewed changes

junqiu-lei reviewed Feb 9, 2022

View reviewed changes

martin-gaievski reviewed Feb 9, 2022

View reviewed changes

jmazanec15 added 2 commits February 9, 2022 21:43

Merge branch 'main' of github.com:opensearch-project/k-NN into shard-…

56565e3

…spacetype-fix

Minor refactoring

fd8bd5e

Signed-off-by: John Mazanec <jmazane@amazon.com>

jmazanec15 requested review from VijayanB, junqiu-lei and martin-gaievski February 10, 2022 06:11

VijayanB approved these changes Feb 10, 2022

View reviewed changes

jmazanec15 added backport 1.2 label to add to PRs to auto backport backport 1.x labels Feb 10, 2022

martin-gaievski approved these changes Feb 10, 2022

View reviewed changes

jmazanec15 merged commit 2fb2ad1 into opensearch-project:main Feb 10, 2022

github-actions bot mentioned this pull request Feb 10, 2022

[Backport 1.x] [BUG FIX] Add space type default and ef search parameter in warmup #279

Merged

	String spaceTypeName = fieldInfo.attributes().getOrDefault(SPACE_TYPE, SpaceType.L2.getValue());
	String spaceTypeDefaultVaule = SpaceType.L2.getValue();
	String spaceTypeName = fieldInfo.attributes().getOrDefault(SPACE_TYPE, spaceTypeDefaultVaule);

-        Map<String, Object> loadParameters = Maps.newHashMap(ImmutableMap.of(
+        final Map<String, Object> loadParameters;
+        if (KNNEngine.NMSLIB.equals(knnEngine)) {
+              loadParameters = ImmutableMap.of(
+                            SPACE_TYPE, spaceType.getValue(),
+                            HNSW_ALGO_EF_SEARCH, KNNSettings.getEfSearchParam(indexName)
+              );
+        } else {
+              loadParameters = ImmutableMap.of(SPACE_TYPE, spaceType.getValue());
+        }
+        return loadParameters;

Conversation

jmazanec15 commented Feb 9, 2022

Description

Issues Resolved

Check List

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Feb 10, 2022 • edited by codecov bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

VijayanB left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Feb 10, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-commenter commented Feb 10, 2022 •

edited by codecov bot

Loading