[ML] Switch TextExpansionQueryBuilder and TextEmbeddingQueryVectorBuilder to return 400 instead of 500 errors #135800
Conversation
jonathan-buttner
added
>bug
:ml
|
Hi @jonathan-buttner, I've created a changelog YAML for you. |
|
|
||
| // This is a hack so that we can control when the client returns valid and invalid results | ||
| // to test both the success and failure paths | ||
| private final AtomicBoolean shouldProduceDenseVectorResults = new AtomicBoolean(); |
There was a problem hiding this comment.
This test class inherits a lot of functionality. It's unclear to me how to create a test that relies on the results being invalid without affecting all the other tests that will run and depend on simulateMethod. If you have ideas I'm happy to make changes.
There was a problem hiding this comment.
A yaml test would be more appropriate here imo. We need an end to end test
jimczi
left a comment
jimczi
left a comment
There was a problem hiding this comment.
Thanks for tackling this @jonathan-buttner
I left some comments
| + modelId | ||
| + "] a compatible model?" | ||
| + "] a compatible model?", | ||
| RestStatus.BAD_REQUEST |
There was a problem hiding this comment.
Let's use an IllegalArgumentException similar to what we do in SparseVectorQueryBuilder?
| + modelId | ||
| + "] a text embedding model?" | ||
| + "] a text embedding model?", | ||
| RestStatus.BAD_REQUEST |
|
|
||
| // This is a hack so that we can control when the client returns valid and invalid results | ||
| // to test both the success and failure paths | ||
| private final AtomicBoolean shouldProduceDenseVectorResults = new AtomicBoolean(); |
There was a problem hiding this comment.
A yaml test would be more appropriate here imo. We need an end to end test
| allowed_warnings: | ||
| - "text_expansion is deprecated. Use sparse_vector instead." | ||
| - match: { error.root_cause.0.type: "illegal_argument_exception" } | ||
| - match: { error.root_cause.0.reason: "expected a result of type [text_expansion_result] received [text_embedding_result]. Is [dense-inference-id] a compatible model?" } |
There was a problem hiding this comment.
nit: why do we need to use a semantic text field to get this error? That feels unrelated since it's not even used in the query.
We can also move it to resources/rest-api-spec/test/ml/search_knn_query_vector_builder.yml ?
There was a problem hiding this comment.
Yeah good point, I'll move it and remove the dependency
There was a problem hiding this comment.
Hmm I can't seem to get it to work with a regular dense vector field:
XPackRestIT > test {p0=ml/search_knn_query_vector_builder/Text expansion query against semantic_text field using a dense vector model returns an failure} FAILED
java.lang.AssertionError: Failure at [ml/search_knn_query_vector_builder:112]: expected [400] status code but api [search] returned [403 Forbidden] [{"error":{"root_cause":[{"type":"status_exception","reason":"Trained model [text_embedding_model] is configured for task [text_embedding] but called with task [text_expansion]"
I'm going to leave it in the semantic text tests.
There was a problem hiding this comment.
That looks like another bug?
There was a problem hiding this comment.
Do you mean we should return a 400 in that situation too instead of a 403? Yeah I typically would think the 403 would be for permissions issues.
|
Pinging @elastic/ml-core (Team:ML) |
3 participants
This PR switches two
IllegalStateExceptionto beElasticsearchStatusExceptionthat explicitly return a 400. This is for the text expansion query builder and the text embedding query vector builder classes.IllegalStateExceptiongets translated into a 500 error response which does not represent what actually occurred. This would arise when a user attempts to perform a text expansion query or text embedding vector query when the index mapping is not configured to support that embedding type.