Update training validation to be handled per algo type #2462

anntians · 2025-01-29T17:33:01Z

Description

This PR is a follow up to another PR #2378. In the other PR we added more detailed error messages to validate training parameters, and this PR builds on those changes by updating the validation to be handled per algo type.

Related Issues

Resolves #2268

Check List

New functionality includes testing.
New functionality has been documented.
API changes companion pull request created.
Commits are signed per the DCO using --signoff.
Public documentation issue/PR created.

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.

anntians · 2025-01-29T17:36:00Z

This PR is a follow up to another PR #2378. Can I get help with adding the skip-changelog and backport main label to this PR please. Thanks!

jmazanec15 · 2025-01-29T23:17:18Z

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

@@ -182,4 +182,30 @@ protected void validateCompressionConflicts(CompressionLevel originalCompression
            throw validationException;
        }
    }
+
+    protected void validateMDivisibleByVectorDimension(


Can we generalize this? M is specific to PQ so shouldnt go in abstract class.

jmazanec15 · 2025-01-29T23:18:24Z

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

+                    .getParameters()
+                    .get(METHOD_ENCODER_PARAMETER);
+
+                if (knnMethodContext.getMethodComponentContext().getParameters().containsKey(METHOD_PARAMETER_NLIST)


Is it possible to move specifics into the HNSW and IVF method classes?

jmazanec15 · 2025-01-29T23:19:00Z

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

+
+        TrainingConfigValidationOutput.TrainingConfigValidationOutputBuilder builder = TrainingConfigValidationOutput.builder();
+
+        // validate ENCODER_PARAMETER_PQ_M is divisible by vector dimension


Flat encoder doesnt have M parameter. Only PQ does. So you can just noop on this

jmazanec15 · 2025-01-29T23:19:46Z

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

+
+        TrainingConfigValidationOutput.TrainingConfigValidationOutputBuilder builder = TrainingConfigValidationOutput.builder();
+
+        // validate ENCODER_PARAMETER_PQ_M is divisible by vector dimension


PQ_M is only valid for PQ encoders. You can noop on this.

jmazanec15 · 2025-02-03T18:52:58Z

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

+                    .getParameters()
+                    .get(METHOD_ENCODER_PARAMETER);
+
+                if (knnMethodContext.getMethodComponentContext().getParameters().containsKey(METHOD_PARAMETER_NLIST)


Can we delegate this to the encoder?

jmazanec15 · 2025-02-03T18:53:09Z

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

+                    .getParameters()
+                    .get(METHOD_ENCODER_PARAMETER);
+
+                if (knnMethodContext.getMethodComponentContext().getParameters().containsKey(METHOD_PARAMETER_NLIST)


Same as above

jmazanec15 · 2025-02-03T18:54:01Z

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

@@ -61,4 +63,10 @@ public CompressionLevel calculateCompressionLevel(
        // TODO: Hard code for now
        return CompressionLevel.x2;
    }
+
+    @Override
+    public TrainingConfigValidationOutput validateEncoderConfig(TrainingConfigValidationInput trainingConfigValidationInput) {


Can you move this to default in Encoder.java? Like /~https://github.com/opensearch-project/k-NN/blob/main/src/main/java/org/opensearch/knn/index/engine/Encoder.java#L20

jmazanec15 · 2025-02-03T18:54:27Z

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

+
+        TrainingConfigValidationOutput.TrainingConfigValidationOutputBuilder builder = TrainingConfigValidationOutput.builder();
+
+        // validate ENCODER_PARAMETER_PQ_M is divisible by vector dimension


We dont need this for QFrameEncoder - this should just noop

jmazanec15 · 2025-02-18T18:10:43Z

@anntians can you rebase to main? Also is this PR ready to be reviewed? I think overall it looks pretty good

jmazanec15 · 2025-02-18T18:09:06Z

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

@@ -182,4 +182,5 @@ protected void validateCompressionConflicts(CompressionLevel originalCompression
            throw validationException;
        }
    }
+


nit: remove this line

jmazanec15 · 2025-02-18T18:10:10Z

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

+
+        if (validationOutput.getValid() != null && !validationOutput.getValid()) {
+            ValidationException validationException = new ValidationException();
+            validationException.addValidationError("Training request ENCODER_PARAMETER_PQ_M is not divisible by vector dimensions");


Can we fix this?

Signed-off-by: AnnTian Shao <anntians@amazon.com>

anntians requested review from heemin32, navneet1v, VijayanB, vamshin, jmazanec15, naveentatikonda, junqiu-lei, martin-gaievski, ryanbogan, luyuncheng, shatejas and 0ctopus13prime as code owners January 29, 2025 17:33

anntians force-pushed the errorMessages branch from c415044 to 9887948 Compare January 29, 2025 20:52

jmazanec15 reviewed Jan 29, 2025

View reviewed changes

anntians force-pushed the errorMessages branch from 06ecaf9 to 697953f Compare January 30, 2025 17:55

jmazanec15 reviewed Feb 3, 2025

View reviewed changes

anntians requested a review from Vikasht34 as a code owner February 5, 2025 21:26

anntians force-pushed the errorMessages branch 2 times, most recently from 1e26bd2 to 2cc1523 Compare February 10, 2025 17:46

jmazanec15 reviewed Feb 18, 2025

View reviewed changes

anntians force-pushed the errorMessages branch 2 times, most recently from 170af13 to 00e597a Compare February 25, 2025 22:24

anntians force-pushed the errorMessages branch 2 times, most recently from ff2c1a5 to c823ce1 Compare February 26, 2025 17:03

anntians changed the base branch from 2.x to main February 26, 2025 17:04

anntians force-pushed the errorMessages branch 2 times, most recently from f267557 to 5941b02 Compare February 26, 2025 17:11

jmazanec15 added the skip-changelog label Feb 26, 2025

AnnTian Shao added 6 commits February 26, 2025 10:21

Update training validation to be handled per algo type

d400a28

Signed-off-by: AnnTian Shao <anntians@amazon.com>

fix bwc tests

8a5205d

Signed-off-by: AnnTian Shao <anntians@amazon.com>

fixes to encoder and method classes

a138e3f

Signed-off-by: AnnTian Shao <anntians@amazon.com>

fixes and moved all validation to encoder

4571060

Signed-off-by: AnnTian Shao <anntians@amazon.com>

Moved error message to within encoder

e161f35

Signed-off-by: AnnTian Shao <anntians@amazon.com>

fix tests

c7f8461

Signed-off-by: AnnTian Shao <anntians@amazon.com>

anntians force-pushed the errorMessages branch from 5941b02 to c7f8461 Compare February 26, 2025 18:21

jmazanec15 approved these changes Feb 26, 2025

View reviewed changes

jed326 added the v3.0.0 label Feb 27, 2025

naveentatikonda approved these changes Feb 27, 2025

View reviewed changes

naveentatikonda merged commit 0d06b23 into opensearch-project:main Feb 27, 2025
38 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update training validation to be handled per algo type #2462

Update training validation to be handled per algo type #2462

anntians commented Jan 29, 2025

anntians commented Jan 29, 2025

jmazanec15 Jan 29, 2025

jmazanec15 Jan 29, 2025

jmazanec15 Jan 29, 2025 •

edited

Loading

jmazanec15 Jan 29, 2025

jmazanec15 Feb 3, 2025

jmazanec15 Feb 3, 2025

jmazanec15 Feb 3, 2025

jmazanec15 Feb 3, 2025

jmazanec15 commented Feb 18, 2025

jmazanec15 Feb 18, 2025

jmazanec15 Feb 18, 2025


		TrainingConfigValidationOutput.TrainingConfigValidationOutputBuilder builder = TrainingConfigValidationOutput.builder();

		// validate ENCODER_PARAMETER_PQ_M is divisible by vector dimension

Update training validation to be handled per algo type #2462

Update training validation to be handled per algo type #2462

Conversation

anntians commented Jan 29, 2025

Description

Related Issues

Check List

anntians commented Jan 29, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmazanec15 Jan 29, 2025 • 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

jmazanec15 commented Feb 18, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmazanec15 Jan 29, 2025 •

edited

Loading