-
Notifications
You must be signed in to change notification settings - Fork 24.6k
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
[ML]add InferenceAction request query validation #110147
[ML]add InferenceAction request query validation #110147
Conversation
💚 CLA has been signed |
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.
Hi @Huaixinww
Thank you for the PR! It would be great, if you could sign the contributor agreement to move forward on the review. For changes in the inference api you can also add the two label :ml
and Team:ML
to request a review from the ML team.
if (taskType.equals(TaskType.RERANK)) { | ||
if (query == null) { | ||
var e = new ActionRequestValidationException(); | ||
e.addValidationError("missing query"); |
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.
e.addValidationError("missing query"); | |
e.addValidationError(format("Field [query] cannot be null for task type [%s]", TaskType.RERANK)); |
} | ||
if (query.isEmpty()) { | ||
var e = new ActionRequestValidationException(); | ||
e.addValidationError("query is empty"); |
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 think we can also use something like e.addValidationError(format("Field [query] cannot be empty for task type [%s]", TaskType.RERANK));
similar to my comment above
@@ -71,6 +72,84 @@ public void testParsing() throws IOException { | |||
} | |||
} | |||
|
|||
public void testValidation() throws IOException { |
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 think this test could be split into several smaller tests testValidation_TextEmbedding_Null
, testValidation_TextEmbedding_Empty
, testValidation_Rerank_Null
etc.
Pinging @elastic/ml-core (Team:ML) |
Hi @timgrein |
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.
LGTM
Thank you for finding the bug and fixing it
@elasticmachine test this please |
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.
Thank you for the changes! LGTM 🚢
When testing Cohere's rerank function, we discovered that if a query is not provided, cohereRerankRequest will throw a NPE.
The validate() method of InferenceAction checks the input but does not validate the query, causing cohereRerankRequest to throw a NPE because it requires the query to be non-null.