-
Notifications
You must be signed in to change notification settings - Fork 25.7k
refactor: enhance semantic_text inference error msg #131519
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
Conversation
Mikep86
left a comment
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 for the quick change! One note about an additional test change we need.
...ava/org/elasticsearch/xpack/inference/action/filter/ShardBulkInferenceActionFilterTests.java
Show resolved
Hide resolved
…ticsearch into expand_inference_error_msg
| itemIndex, | ||
| new InferenceException( | ||
| "Insufficient memory available to update source on document [" + indexRequest.getIndexRequest().id() + "]", | ||
| "Unable to insert inference results into document [" |
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.
❓ Do we want to also mention update source here, or is it fine unifying the error msg?
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.
In this case, I think it's fine that we have a unified error message. The fact that this error is thrown prior to us actually updating source is an implementation detail we don't want to leak.
|
Pinging @elastic/search-eng (Team:SearchOrg) |
|
Pinging @elastic/search-relevance (Team:Search - Relevance) |
Mikep86
left a comment
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, thanks for the quick work!
| itemIndex, | ||
| new InferenceException( | ||
| "Insufficient memory available to update source on document [" + indexRequest.getIndexRequest().id() + "]", | ||
| "Unable to insert inference results into document [" |
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.
In this case, I think it's fine that we have a unified error message. The fact that this error is thrown prior to us actually updating source is an implementation detail we don't want to leak.
...ava/org/elasticsearch/xpack/inference/action/filter/ShardBulkInferenceActionFilterTests.java
Show resolved
Hide resolved
kderusso
left a comment
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!
|
Thanks for the prompt review both! Quick question when you get the chance: should this be backported to 9.1 and 8.19? |
|
@mromaios I don't think this should be backported as it's an enhancement. We could stretch this to say it's a bug fix, but it's borderline. |
|
Thanks @Mikep86, following the enhancement route. |
This PR rephrases the InferenceError message that can occur while ingesting
semantic_textfields to include additional debugging details.