-
Notifications
You must be signed in to change notification settings - Fork 132
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
Added more detailed error messages for KNN model training #2378
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: AnnTian Shao <anntians@amazon.com>
a9c0dbd
to
ef0e7e5
Compare
Signed-off-by: Tommy Shao <69884021+anntians@users.noreply.github.com>
@@ -134,6 +138,30 @@ protected void getTrainingIndexSizeInKB(TrainingModelRequest trainingModelReques | |||
trainingVectors = trainingModelRequest.getMaximumVectorCount(); | |||
} | |||
|
|||
long minTrainingVectorCount = 1000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you make the 1000 value a class level constant?
|
||
if (trainingVectors < minTrainingVectorCount) { | ||
ValidationException exception = new ValidationException(); | ||
exception.addValidationError("Number of training points should be greater than " + minTrainingVectorCount); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use String.format for concatenation
if (knnMethodContext.getMethodComponentContext().getParameters().containsKey(ENCODER_PARAMETER_PQ_M) | ||
&& knnMethodConfigContext.getDimension() % (Integer) knnMethodContext.getMethodComponentContext() | ||
.getParameters() | ||
.get(ENCODER_PARAMETER_PQ_M) != 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if that parameter is always present or not, but if it's optional then this line can generate the runtime exception in case parameter is not present
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @anntians. I think this is a good first step. What we should do next is move all of the specific checks around the parameters behind the engine method abstraction. See https://github.com/opensearch-project/k-NN/tree/main/src/main/java/org/opensearch/knn/index/engine.
Here is my idea for it: add in a method in KNNLibraryIndexingContext called something like getTrainingConfigValidationSetup() that returns a function that takes as input the number of training vectors (or a more general object) and performs some kind of validation.
Then, we can use this to hide the method specific validations inside the engine abstraction, which will be clean and maintainable. For instance, we can implement checks for IVFPQ in https://github.com/opensearch-project/k-NN/blob/main/src/main/java/org/opensearch/knn/index/engine/faiss/FaissIVFPQEncoder.java, etc.
Description
Previously, a consistent feedback we get around PQ and IVF is that there is limited visibility into the failure cases. Part of this is because the errors are thrown on the Faiss side and we don't return stack traces in Rest response. So, this makes it difficult to use PQ and IVF. Thus, this PR provides improved error messages by adding explicit checks for the most common errors:
[ ] For PQ, explicitly check in OpenSearch an invalid configuration where m does not divide dimension
[ ] For PQ/IVF, check the number of training points matches the minimum clustering criteria defined in faiss
[ ] If there is not enough memory, explicitly say that there is not enough memory.
Adding these 3 checks will cover 90% of the training failures that occur.
Related Issues
Resolves #2268
Check List
--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.