[ML] Change format for Unified Chat error responses #121396
Conversation
Unified Chat Completion error responses now forward code, type, and param to in the response payload. `reason` has been renamed to `message`.
|
Hi @prwhelan, I've created a changelog YAML for you. |
|
Pinging @elastic/ml-core (Team:ML) |
There was a problem hiding this comment.
Great changes Pat!
I think we'll also want to change this for EIS since it leverages the unified format as well:
Let me know if we already have coverage for this but could we add a new integration test that spins up a mock web server to mock an openai error response? That way we can make a request to a live ES node and ensure that the response all the way back to the rest client is what we're expecting.
I'm thinking something like this: https://github.com/elastic/elasticsearch/blob/main/x-pack/plugin/inference/qa/inference-service-tests/src/javaRestTest/java/org/elasticsearch/xpack/inference/MockElasticInferenceServiceAuthorizationServer.java#L21
|
|
||
| public UnifiedChatCompletionException(RestStatus status, String message, String type, @Nullable String code, @Nullable String param) { | ||
| super(message, status); | ||
| this.message = message; |
There was a problem hiding this comment.
Should we add some Objects.requireNonNull for the values that are required?
| public static UnifiedChatCompletionException fromThrowable(Throwable t) { | ||
| if (t instanceof UnifiedChatCompletionException e) { | ||
| return e; | ||
| } else if (unwrapCause(t) instanceof UnifiedChatCompletionException e) { |
There was a problem hiding this comment.
Below we implement the unwrapCause(). It doesn't look like UnifiedChatCompletionException implements ElasticsearchWrapperException in the inheritance chain. Could we use ExceptionHelper.unwrapCause() and kind of like this:
public static UnifiedChatCompletionException fromThrowable2(Throwable t) {
var unwrappedCause = ExceptionsHelper.unwrapCause(t);
if (unwrappedCause instanceof UnifiedChatCompletionException e) {
return e;
} else {
return maybeError(t).map(error -> {
...
});
}
}
There was a problem hiding this comment.
Ah, I think this method was a holdover from when UnifiedChatCompletionException was a ElasticsearchWrapperException. We can remove it
💔 Backport failed
You can use sqren/backport to manually backport by running |
Unified Chat Completion error responses now forward code, type, and param to in the response payload. `reason` has been renamed to `message`. Notes: - `XContentFormattedException` is a `ChunkedToXContent` so that the REST listener can call `toXContentChunked` to format the output structure. By default, the structure forwards to our existing ES exception structure. - `UnifiedChatCompletionException` will override the structure to match the new unified format. - The Rest, Transport, and Stream handlers all check the exception to verify it is a UnifiedChatCompletionException. - OpenAI response handler now reads all the fields in the error message and forwards them to the user. - In the event that a `Throwable` is a `Error`, we rethrow it on another thread so the JVM can catch and handle it. We also stop surfacing the JVM details to the user in the error message (but it's still logged for debugging purposes).
Unified Chat Completion error responses now forward code, type, and param to in the response payload. `reason` has been renamed to `message`. Notes: - `XContentFormattedException` is a `ChunkedToXContent` so that the REST listener can call `toXContentChunked` to format the output structure. By default, the structure forwards to our existing ES exception structure. - `UnifiedChatCompletionException` will override the structure to match the new unified format. - The Rest, Transport, and Stream handlers all check the exception to verify it is a UnifiedChatCompletionException. - OpenAI response handler now reads all the fields in the error message and forwards them to the user. - In the event that a `Throwable` is a `Error`, we rethrow it on another thread so the JVM can catch and handle it. We also stop surfacing the JVM details to the user in the error message (but it's still logged for debugging purposes).
Unified Chat Completion error responses now forward code, type, and param to in the response payload. `reason` has been renamed to `message`. Notes: - `XContentFormattedException` is a `ChunkedToXContent` so that the REST listener can call `toXContentChunked` to format the output structure. By default, the structure forwards to our existing ES exception structure. - `UnifiedChatCompletionException` will override the structure to match the new unified format. - The Rest, Transport, and Stream handlers all check the exception to verify it is a UnifiedChatCompletionException. - OpenAI response handler now reads all the fields in the error message and forwards them to the user. - In the event that a `Throwable` is a `Error`, we rethrow it on another thread so the JVM can catch and handle it. We also stop surfacing the JVM details to the user in the error message (but it's still logged for debugging purposes).
Unified Chat Completion error responses now forward code, type, and param to in the response payload.
reasonhas been renamed tomessage.Notes:
XContentFormattedExceptionis aChunkedToXContentso that the REST listener can calltoXContentChunkedto format the output structure. By default, the structure forwards to our existing ES exception structure.UnifiedChatCompletionExceptionwill override the structure to match the new unified format.Throwableis aError, we rethrow it on another thread so the JVM can catch and handle it. We also stop surfacing the JVM details to the user in the error message (but it's still logged for debugging purposes).