Add validation for pq m parameter before training starts #1713

ryanbogan · 2024-05-21T19:39:10Z

Description

Currently, if the dimension of a model is not divisible by the pq m parameter (code count) provided in the training request, training fails with a detailed message in the logs. In addition, the model state is update to failed and the model error is set to "Failed to execute training. May be caused by an invalid method definition or not enough memory to perform training.".

This PR adds a validation check for the above case before training starts or any memory is allocated. The model state is still set to failed, but the error message is more specific.

Issues Resolved

#1075

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.

Signed-off-by: Ryan Bogan <[email protected]>

src/main/java/org/opensearch/knn/training/TrainingJob.java

src/main/java/org/opensearch/knn/common/KNNConstants.java

Signed-off-by: Ryan Bogan <[email protected]>

src/main/java/org/opensearch/knn/index/KNNMethod.java

VijayanB · 2024-05-28T19:05:10Z

src/main/java/org/opensearch/knn/index/KNNMethod.java

+            );
+        }
+
+        ValidationException methodValidation = methodComponent.validateWithData(


do we have to validate further even if we don't support space type?

Even though validation will ultimately fail if space type is not supported, more error messages can be added to the errors based on any potential problems with the method component context

src/main/java/org/opensearch/knn/index/Parameter.java

Signed-off-by: Ryan Bogan <[email protected]>

src/main/java/org/opensearch/knn/index/Parameter.java

src/test/java/org/opensearch/knn/index/FaissIT.java

src/main/java/org/opensearch/knn/training/TrainingDataSpec.java

Signed-off-by: Ryan Bogan <[email protected]>

codecov · 2024-05-28T20:04:06Z

Codecov Report

Attention: Patch coverage is 78.57143% with 24 lines in your changes are missing coverage. Please review.

Project coverage is 84.96%. Comparing base (9a52b2b) to head (844f1ef).
Report is 6 commits behind head on main.

❗ Current head 844f1ef differs from pull request most recent head a12022b

Please upload reports for the commit a12022b to get more accurate results.

Files	Patch %	Lines
.../main/java/org/opensearch/knn/index/Parameter.java	77.61%	12 Missing and 3 partials ⚠️
.../main/java/org/opensearch/knn/index/KNNMethod.java	68.75%	4 Missing and 1 partial ⚠️
...java/org/opensearch/knn/index/MethodComponent.java	88.23%	1 Missing and 1 partial ⚠️
...main/java/org/opensearch/knn/index/util/Faiss.java	50.00%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #1713      +/-   ##
============================================
- Coverage     85.02%   84.96%   -0.07%     
- Complexity     1463     1486      +23     
============================================
  Files           178      178              
  Lines          5898     6026     +128     
  Branches        598      626      +28     
============================================
+ Hits           5015     5120     +105     
- Misses          632      649      +17     
- Partials        251      257       +6

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

jmazanec15 · 2024-05-28T23:12:52Z

src/main/java/org/opensearch/knn/index/util/Faiss.java

@@ -109,9 +109,6 @@ class Faiss extends NativeLibrary {
            .build()
    );

-    // TODO: To think about in future: for PQ, if dimension is not divisible by code count, PQ will fail. Right now,


nice - finally getting rid of it

src/main/java/org/opensearch/knn/training/TrainingDataSpec.java

Signed-off-by: Ryan Bogan <[email protected]>

* Add validation for pq code count before training starts Signed-off-by: Ryan Bogan <[email protected]> * Add integration test Signed-off-by: Ryan Bogan <[email protected]> * Add unit tests Signed-off-by: Ryan Bogan <[email protected]> * Clean up code Signed-off-by: Ryan Bogan <[email protected]> * Remove unnecessary lines Signed-off-by: Ryan Bogan <[email protected]> * Add changelog entry Signed-off-by: Ryan Bogan <[email protected]> * Change framework to add validation with data Signed-off-by: Ryan Bogan <[email protected]> * Remove unused error message Signed-off-by: Ryan Bogan <[email protected]> * Add unit tests Signed-off-by: Ryan Bogan <[email protected]> * Change space type check name for readability Signed-off-by: Ryan Bogan <[email protected]> * Add javadocs Signed-off-by: Ryan Bogan <[email protected]> * Modify validation error wording and add json structure to tests Signed-off-by: Ryan Bogan <[email protected]> * Change TrainingDataSpec to VectorSpaceInfo Signed-off-by: Ryan Bogan <[email protected]> * Add unit tests Signed-off-by: Ryan Bogan <[email protected]> --------- Signed-off-by: Ryan Bogan <[email protected]> (cherry picked from commit 3701d19)

* Add validation for pq code count before training starts Signed-off-by: Ryan Bogan <[email protected]> * Add integration test Signed-off-by: Ryan Bogan <[email protected]> * Add unit tests Signed-off-by: Ryan Bogan <[email protected]> * Clean up code Signed-off-by: Ryan Bogan <[email protected]> * Remove unnecessary lines Signed-off-by: Ryan Bogan <[email protected]> * Add changelog entry Signed-off-by: Ryan Bogan <[email protected]> * Change framework to add validation with data Signed-off-by: Ryan Bogan <[email protected]> * Remove unused error message Signed-off-by: Ryan Bogan <[email protected]> * Add unit tests Signed-off-by: Ryan Bogan <[email protected]> * Change space type check name for readability Signed-off-by: Ryan Bogan <[email protected]> * Add javadocs Signed-off-by: Ryan Bogan <[email protected]> * Modify validation error wording and add json structure to tests Signed-off-by: Ryan Bogan <[email protected]> * Change TrainingDataSpec to VectorSpaceInfo Signed-off-by: Ryan Bogan <[email protected]> * Add unit tests Signed-off-by: Ryan Bogan <[email protected]> --------- Signed-off-by: Ryan Bogan <[email protected]> (cherry picked from commit 3701d19) Co-authored-by: Ryan Bogan <[email protected]>

* Fix flaky test in Faiss JNI range search (#1705) Signed-off-by: Junqiu Lei <[email protected]> * Support script score when doc value is disabled and fix misusing DISI (#1696) * Revert "Revert 'Support script score when doc value is disabled' (#1662)" This reverts commit bd2f403. Signed-off-by: panguixin <[email protected]> * fix misusing doc value Signed-off-by: panguixin <[email protected]> * add changelog Signed-off-by: panguixin <[email protected]> --------- Signed-off-by: panguixin <[email protected]> * --- (#1712) updated-dependencies: - dependency-name: requests dependency-type: direct:production ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Update threshold value after new result is added (#1715) Signed-off-by: Heemin Kim <[email protected]> * Use the Lucene Distance Calculation Function in Script Scoring for doing exact search (#1699) * Use the Lucene Distance Calculation Function in Script Scoring for doing exact search Signed-off-by: Ryan Bogan <[email protected]> * Add Changelog entry Signed-off-by: Ryan Bogan <[email protected]> * Fix failing test Signed-off-by: Ryan Bogan <[email protected]> * fix test Signed-off-by: Ryan Bogan <[email protected]> * Fix test bug and remove unnecessary validation Signed-off-by: Ryan Bogan <[email protected]> * Remove cosineSimilOptimized Signed-off-by: Ryan Bogan <[email protected]> * Revert "Remove cosineSimilOptimized" This reverts commit f872d83. Signed-off-by: Ryan Bogan <[email protected]> --------- Signed-off-by: Ryan Bogan <[email protected]> * Add validation for pq m parameter before training starts (#1713) * Add validation for pq code count before training starts Signed-off-by: Ryan Bogan <[email protected]> * Add integration test Signed-off-by: Ryan Bogan <[email protected]> * Add unit tests Signed-off-by: Ryan Bogan <[email protected]> * Clean up code Signed-off-by: Ryan Bogan <[email protected]> * Remove unnecessary lines Signed-off-by: Ryan Bogan <[email protected]> * Add changelog entry Signed-off-by: Ryan Bogan <[email protected]> * Change framework to add validation with data Signed-off-by: Ryan Bogan <[email protected]> * Remove unused error message Signed-off-by: Ryan Bogan <[email protected]> * Add unit tests Signed-off-by: Ryan Bogan <[email protected]> * Change space type check name for readability Signed-off-by: Ryan Bogan <[email protected]> * Add javadocs Signed-off-by: Ryan Bogan <[email protected]> * Modify validation error wording and add json structure to tests Signed-off-by: Ryan Bogan <[email protected]> * Change TrainingDataSpec to VectorSpaceInfo Signed-off-by: Ryan Bogan <[email protected]> * Add unit tests Signed-off-by: Ryan Bogan <[email protected]> --------- Signed-off-by: Ryan Bogan <[email protected]> * Updating the BWC test config after 2.14 release (#1724) Signed-off-by: Navneet Verma <[email protected]> --------- Signed-off-by: Junqiu Lei <[email protected]> Signed-off-by: panguixin <[email protected]> Signed-off-by: dependabot[bot] <[email protected]> Signed-off-by: Heemin Kim <[email protected]> Signed-off-by: Ryan Bogan <[email protected]> Signed-off-by: Navneet Verma <[email protected]> Co-authored-by: Junqiu Lei <[email protected]> Co-authored-by: panguixin <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Heemin Kim <[email protected]> Co-authored-by: Ryan Bogan <[email protected]> Co-authored-by: Navneet Verma <[email protected]>

…project#1713) * Add validation for pq code count before training starts Signed-off-by: Ryan Bogan <[email protected]> * Add integration test Signed-off-by: Ryan Bogan <[email protected]> * Add unit tests Signed-off-by: Ryan Bogan <[email protected]> * Clean up code Signed-off-by: Ryan Bogan <[email protected]> * Remove unnecessary lines Signed-off-by: Ryan Bogan <[email protected]> * Add changelog entry Signed-off-by: Ryan Bogan <[email protected]> * Change framework to add validation with data Signed-off-by: Ryan Bogan <[email protected]> * Remove unused error message Signed-off-by: Ryan Bogan <[email protected]> * Add unit tests Signed-off-by: Ryan Bogan <[email protected]> * Change space type check name for readability Signed-off-by: Ryan Bogan <[email protected]> * Add javadocs Signed-off-by: Ryan Bogan <[email protected]> * Modify validation error wording and add json structure to tests Signed-off-by: Ryan Bogan <[email protected]> * Change TrainingDataSpec to VectorSpaceInfo Signed-off-by: Ryan Bogan <[email protected]> * Add unit tests Signed-off-by: Ryan Bogan <[email protected]> --------- Signed-off-by: Ryan Bogan <[email protected]>

ryanbogan added 7 commits May 20, 2024 11:27

Add validation for pq code count before training starts

e88c0dd

Signed-off-by: Ryan Bogan <[email protected]>

Add integration test

d3a8e04

Signed-off-by: Ryan Bogan <[email protected]>

Add unit tests

d92faca

Signed-off-by: Ryan Bogan <[email protected]>

Clean up code

cc0c6aa

Signed-off-by: Ryan Bogan <[email protected]>

Remove unnecessary lines

7405f23

Signed-off-by: Ryan Bogan <[email protected]>

Add changelog entry

bf79f41

Signed-off-by: Ryan Bogan <[email protected]>

Rebase with main

5de0b41

Signed-off-by: Ryan Bogan <[email protected]>

ryanbogan marked this pull request as ready for review May 21, 2024 20:46

ryanbogan requested review from heemin32, navneet1v, VijayanB, vamshin, jmazanec15, naveentatikonda, junqiu-lei, martin-gaievski and luyuncheng as code owners May 21, 2024 20:46

jmazanec15 reviewed May 21, 2024

View reviewed changes

src/main/java/org/opensearch/knn/training/TrainingJob.java Outdated Show resolved Hide resolved

junqiu-lei reviewed May 22, 2024

View reviewed changes

src/main/java/org/opensearch/knn/common/KNNConstants.java Outdated Show resolved Hide resolved

ryanbogan added 3 commits May 24, 2024 12:28

Change framework to add validation with data

4132375

Signed-off-by: Ryan Bogan <[email protected]>

Remove unused error message

f4a8c56

Signed-off-by: Ryan Bogan <[email protected]>

Add unit tests

0707c7f

Signed-off-by: Ryan Bogan <[email protected]>