Fix mode/comp params so parameter overrides work #2083

jmazanec15 · 2024-09-10T05:09:47Z

Description

In #1949, the intention was to allow users to override params when specifying the mode/compression. If this cannot be done, then it will make it wont be possible to tune indexing characteristics when mode is set, and is thus a broken experience. PR fixes this so that users can override params when using mode/compression params.

The main classes are:

KNNVectorFieldMapper
AbstractMethodResolver
KNNMethodContext
MethodComponentContext
EngineResolver
SpaceTypeResolver

Check List

Commits are signed 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.

heemin32 · 2024-09-10T15:34:37Z

src/main/java/org/opensearch/knn/index/engine/KNNMethodContext.java

+
+        this.knnEngine = knnMethodContext.knnEngine;
+        this.spaceType = knnMethodContext.spaceType;
+        this.isEngineConfigured = true;


How do we know if engine configured or not? The engine in passed knnMethodContext might not have been configured.

It will only be not configured if it is built from parsing. Otherwise, it will be configured.

+1 its creating ambiguity, why is this needed? why can't we rely on nulls?

Issue is with respect to BWC. Currently, in the parse method, we default to nmslib. So, we cannot swap this to null or else it might break BWC. For SpaceType, which is similarly defined, @heemin32 added an "SpaceType.UNDEFINED" option. I decided to not do this and just set whether it was explicitly configured on parse in order to check if we need to update.

probably add a comment why its introduced and cannot be null

heemin32 · 2024-09-10T15:41:27Z

src/main/java/org/opensearch/knn/index/engine/MethodComponentContext.java

+                if (entry.getValue() instanceof MethodComponentContext) {
+                    parameters.put(entry.getKey(), new MethodComponentContext((MethodComponentContext) entry.getValue()));
+                } else {
+                    parameters.put(entry.getKey(), entry.getValue());


Curious what other types will be there. Is it okay not to copy them?

They'll be one of the following: https://github.com/opensearch-project/k-NN/blob/main/src/main/java/org/opensearch/knn/index/engine/Parameter.java. So it will be okay

src/main/java/org/opensearch/knn/plugin/rest/RestTrainModelHandler.java

PR adds capability to override parameters when specifying mode and compression. In order to do this, I add functionality for creating a deep copy of KNNMethodContext and MethodComponentContext so that we wouldnt overwrite user provided config. Then, re-arranged some of the parameter resolution logic. Signed-off-by: John Mazanec <[email protected]>

shatejas · 2024-09-10T16:43:31Z

src/main/java/org/opensearch/knn/index/mapper/KNNVectorFieldMapper.java

-
-            if (isModeConfigured && builder.vectorDataType.getValue() != VectorDataType.FLOAT) {
+        private void validateModeAndCompressionForDataType(KNNVectorFieldMapper.Builder builder) {
+            boolean isModeOrCompressionConfigured = builder.mode.isConfigured() || builder.compressionLevel.isConfigured();


Why fail for mode = in_memory and/or compression = 1x for float?

not for float - for non float we dont support setting it. I guess I can change this to support in_memory/1x for binary and byte to be no-op. Is this what you meant?

so if vector data type is float, mode is in-memory and compression is 1x, doesn't that mean it should the current code path without any quantization? what am I missing?

Oh nvm I think I missed the ! in the condition below

src/main/java/org/opensearch/knn/index/engine/SpaceTypeResolver.java

src/main/java/org/opensearch/knn/index/engine/EngineResolver.java

shatejas · 2024-09-10T17:00:30Z

src/main/java/org/opensearch/knn/index/mapper/LuceneFieldMapper.java

            .getKnnVectorSimilarityFunction()
            .getVectorSimilarityFunction();

        this.fieldType = vectorDataType.createKnnVectorFieldType(knnMappingConfig.getDimension(), vectorSimilarityFunction);

        if (this.hasDocValues) {
-            this.vectorFieldType = buildDocValuesFieldType(knnMethodContext.getKnnEngine());
+            this.vectorFieldType = buildDocValuesFieldType(resolvedKnnMethodContext.getKnnEngine());


There is some ambiguity when to use resolved and when to use original. I am not sure if there is a way to resolve that, there is a comment around it, I think its best to add guidelines in that comment

Sure, Ill add a comment

src/main/java/org/opensearch/knn/index/engine/KNNEngine.java

shatejas · 2024-09-10T17:39:36Z

src/main/java/org/opensearch/knn/index/engine/KNNLibrary.java

@@ -14,7 +14,7 @@
 /**
 * KNNLibrary is an interface that helps the plugin communicate with k-NN libraries
 */
-public interface KNNLibrary {
+public interface KNNLibrary extends MethodResolver {


Do we really need to implement this? Would prefer punting it for now and rethink for later because no other methods are through interfaces in this interface

Not sure what you mean. Typically, the requests are routed to the correct library via the knn engine. So, by implementing on the engine and library, we can efficiently route it.

Its odd when KNNLibrary doesn't follow the pattern for other functions which are stand alone but can be broken down into interfaces

shatejas · 2024-09-10T17:57:45Z

src/main/java/org/opensearch/knn/index/engine/KNNMethodContext.java

+
+        this.knnEngine = knnMethodContext.knnEngine;
+        this.spaceType = knnMethodContext.spaceType;
+        this.isEngineConfigured = true;


+1 its creating ambiguity, why is this needed? why can't we rely on nulls?

src/main/java/org/opensearch/knn/index/engine/MethodComponentContext.java

src/main/java/org/opensearch/knn/index/engine/faiss/FaissHNSWPQEncoder.java

src/main/java/org/opensearch/knn/index/engine/faiss/FaissIVFPQEncoder.java

Signed-off-by: John Mazanec <[email protected]>

PR adds capability to override parameters when specifying mode and compression. In order to do this, I add functionality for creating a deep copy of KNNMethodContext and MethodComponentContext so that we wouldnt overwrite user provided config. Then, re-arranged some of the parameter resolution logic. Signed-off-by: John Mazanec <[email protected]> (cherry picked from commit 270ac6a)

jmazanec15 added the skip-changelog label Sep 10, 2024

jmazanec15 requested review from heemin32, navneet1v, VijayanB, vamshin, naveentatikonda, junqiu-lei, martin-gaievski, ryanbogan and luyuncheng as code owners September 10, 2024 05:09

jmazanec15 changed the title ~~Add method parameter override for mode/comp~~ Fix mode/comp params to support parameter overrides Sep 10, 2024

jmazanec15 force-pushed the support-method-overrides branch from 497ba8f to 3347eb0 Compare September 10, 2024 13:40

jmazanec15 changed the title ~~Fix mode/comp params to support parameter overrides~~ Fix mode/comp params so parameter overrides work Sep 10, 2024

heemin32 reviewed Sep 10, 2024

View reviewed changes

jmazanec15 force-pushed the support-method-overrides branch from 3347eb0 to 1eebcf6 Compare September 10, 2024 16:30

shatejas reviewed Sep 10, 2024

View reviewed changes

jmazanec15 requested review from heemin32 and shatejas September 10, 2024 18:28

jmazanec15 force-pushed the support-method-overrides branch from 51dce61 to 3973235 Compare September 10, 2024 20:51

heemin32 previously approved these changes Sep 10, 2024

View reviewed changes

jmazanec15 added backport 2.x backport 2.17 labels Sep 10, 2024

jmazanec15 dismissed heemin32’s stale review via 95b7cda September 10, 2024 20:55

jmazanec15 force-pushed the support-method-overrides branch from 3973235 to 95b7cda Compare September 10, 2024 20:55

Adding tests and cleaning up

bc8cae5

Signed-off-by: John Mazanec <[email protected]>

jmazanec15 force-pushed the support-method-overrides branch from 95b7cda to bc8cae5 Compare September 10, 2024 20:58

heemin32 approved these changes Sep 10, 2024

View reviewed changes

navneet1v approved these changes Sep 10, 2024

View reviewed changes

jmazanec15 merged commit 270ac6a into opensearch-project:main Sep 10, 2024
27 of 29 checks passed

opensearch-trigger-bot bot mentioned this pull request Sep 10, 2024

[Backport 2.x] Fix mode/comp params so parameter overrides work #2084

Merged

opensearch-trigger-bot bot mentioned this pull request Sep 10, 2024

[Backport 2.17] Fix mode/comp params so parameter overrides work #2085

Merged

buddharajusahil mentioned this pull request Nov 26, 2024

Allow method parameter override for training based indices (solves issue #2246) #2290

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix mode/comp params so parameter overrides work #2083

Fix mode/comp params so parameter overrides work #2083

jmazanec15 commented Sep 10, 2024 •

edited

Loading

heemin32 Sep 10, 2024

jmazanec15 Sep 10, 2024

shatejas Sep 10, 2024

jmazanec15 Sep 10, 2024

shatejas Sep 10, 2024

heemin32 Sep 10, 2024

jmazanec15 Sep 10, 2024

shatejas Sep 10, 2024 •

edited

Loading

jmazanec15 Sep 10, 2024

shatejas Sep 10, 2024

shatejas Sep 10, 2024

shatejas Sep 10, 2024

jmazanec15 Sep 10, 2024

shatejas Sep 10, 2024

jmazanec15 Sep 10, 2024

shatejas Sep 10, 2024

shatejas Sep 10, 2024

Fix mode/comp params so parameter overrides work #2083

Fix mode/comp params so parameter overrides work #2083

Conversation

jmazanec15 commented Sep 10, 2024 • edited Loading

Description

Check List

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shatejas Sep 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmazanec15 commented Sep 10, 2024 •

edited

Loading

shatejas Sep 10, 2024 •

edited

Loading