Add POST _unified for the inference API #3313

jonathan-buttner · 2024-12-16T19:44:54Z

This PR adds the _unified route for the Inference API. This coincides with the elasticsearch PR: elastic/elasticsearch#117589

github-actions · 2024-12-16T19:46:15Z

Following you can find the validation results for the APIs you have changed.

API	Status	Request	Response
`inference.delete`	⚪	Missing test	Missing test
`inference.get`	🟢	1/1	1/1
`inference.inference`	⚪	Missing test	Missing test
`inference.put`	⚪	Missing test	Missing test
`inference.stream_inference`	🟠	Missing type	Missing type
`inference.unified_inference`	⚪	Missing test	Missing test

You can validate these APIs yourself by using the make validate target.

jonathan-buttner · 2024-12-16T19:46:26Z

specification/_json_spec/inference.unified_inference.json

+    "stability": "stable",
+    "visibility": "public",
+    "headers": {
+      "accept": ["text/event-stream"],


This is what we have for the streaming api here: https://github.com/elastic/elasticsearch-specification/blob/main/specification/_json_spec/inference.stream_inference.json#L10

Not sure if this is correct if it's indicating that the post request accepts an event stream or that the response returns one 🤔

Both HTTP headers are used in client requests:

Accept tells Elasticsearch that returning event streams is OK

Content-Type is about the request's body, which is indeed JSON here.

In other words, this is correct, thank you!

jonathan-buttner · 2024-12-16T19:47:18Z

specification/inference/unified_inference/UnifiedRequest.ts

+
+/**
+ * Perform inference on the service using the Unified Schema
+ * @rest_spec_name inference.unified_inference


unified_inference seemed to follow stream_inference. I'm open to other ideas though.

No strong opinions, this seems OK.

jonathan-buttner · 2024-12-16T19:47:39Z

specification/inference/unified_inference/UnifiedRequest.ts

+/**
+ * Perform inference on the service using the Unified Schema
+ * @rest_spec_name inference.unified_inference
+ * @availability stack since=8.18.0 stability=stable visibility=public


@davidkyle Double check that this is what we want

This looks good to me.

github-actions · 2024-12-16T19:50:25Z

Following you can find the validation results for the APIs you have changed.

API	Status	Request	Response
`inference.delete`	⚪	Missing test	Missing test
`inference.get`	🟢	1/1	1/1
`inference.inference`	⚪	Missing test	Missing test
`inference.put`	⚪	Missing test	Missing test
`inference.stream_inference`	🟠	Missing type	Missing type
`inference.unified_inference`	⚪	Missing test	Missing test

You can validate these APIs yourself by using the make validate target.

pquentin

Thanks! I answered your questions, but did not look at the contents yet. Do we have YAML tests in Elasticsearch for this feature? It would help to validate the requests.

pquentin · 2024-12-17T06:36:34Z

specification/_json_spec/inference.unified_inference.json

+    "stability": "stable",
+    "visibility": "public",
+    "headers": {
+      "accept": ["text/event-stream"],


Both HTTP headers are used in client requests:

Accept tells Elasticsearch that returning event streams is OK

Content-Type is about the request's body, which is indeed JSON here.

In other words, this is correct, thank you!

pquentin · 2024-12-17T06:37:17Z

specification/inference/unified_inference/UnifiedRequest.ts

+
+/**
+ * Perform inference on the service using the Unified Schema
+ * @rest_spec_name inference.unified_inference


No strong opinions, this seems OK.

pquentin · 2024-12-17T06:37:18Z

specification/inference/unified_inference/UnifiedRequest.ts

+/**
+ * Perform inference on the service using the Unified Schema
+ * @rest_spec_name inference.unified_inference
+ * @availability stack since=8.18.0 stability=stable visibility=public


This looks good to me.

maxhniebergall

LGTM

l-trotta · 2024-12-18T09:47:51Z

specification/inference/unified_inference/UnifiedRequest.ts

@@ -0,0 +1,214 @@
+/*


~~we usually try to have only the request class in the corresponding Request file, and all other types we put in the types folder above (this is just to make it easier to maintain)~~

nevermind that, could we just move the main Request type to the top of the file?

specification/inference/unified_inference/UnifiedRequest.ts

specification/inference/_types/Results.ts

specification/inference/unified_inference/UnifiedRequest.ts

l-trotta · 2024-12-18T12:36:24Z

@jonathan-buttner I have a question about the response format: is there a way to get it in json format, or is it just text/stream?

jonathan-buttner · 2024-12-18T13:28:54Z

@jonathan-buttner I have a question about the response format: is there a way to get it in json format, or is it just text/stream?

Thanks for the review! Currently it's only text/stream but in the near future we'd like to add a non-streaming version. Is that possible to do using the same route (.../_unified)? Or do we need to create a new one?

jonathan-buttner · 2024-12-18T13:30:14Z

Thanks! I answered your questions, but did not look at the contents yet. Do we have YAML tests in Elasticsearch for this feature? It would help to validate the requests.

Thanks for the review! Not yet, let me see if I can add some.

l-trotta · 2024-12-18T13:49:29Z

@jonathan-buttner I have a question about the response format: is there a way to get it in json format, or is it just text/stream?

Thanks for the review! Currently it's only text/stream but in the near future we'd like to add a non-streaming version. Is that possible to do using the same route (.../_unified)? Or do we need to create a new one?

the thing is: if the return type is just text then the response body should be just string, because the whole body structure is needed just for json, not other types of responses. @pquentin like we were talking about hot threads remember?

jonathan-buttner · 2024-12-18T13:53:26Z

@jonathan-buttner I have a question about the response format: is there a way to get it in json format, or is it just text/stream?

Thanks for the review! Currently it's only text/stream but in the near future we'd like to add a non-streaming version. Is that possible to do using the same route (.../_unified)? Or do we need to create a new one?

the thing is: if the return type is just text then the response body should be just string, because the whole body structure is needed just for json, not other types of responses. @pquentin like we were talking about hot threads remember?

Oh sorry I might have misunderstood. Here's what Postman parses out:

Where each one of those messages is valid json:

{
    "id": "chatcmpl-AfVcn7De1ibDPKB7iBy83oEhpHxC9",
    "choices": [],
    "model": "gpt-4o-2024-08-06",
    "object": "chat.completion.chunk",
    "usage": {
        "completion_tokens": 6,
        "prompt_tokens": 63,
        "total_tokens": 69
    }
}

Except for the [DONE] message I suppose.

Do we still want the response to be only text?

pquentin · 2024-12-18T14:34:08Z

Postman makes it look mainly like JSON, but those are server-sent events. Is this unified API going to be used for multiple providers? OpenAI, Anthropic Claude, and Google Gemini all use server-sent events but then provide really different structures: https://til.simonwillison.net/llms/streaming-llm-apis.

If yes, then clients would need to know what provider this is (in the content-type header?). There would not be a need to map the options in the spec; it's OK to treat it as an opaque string or buffer. Also, I'm afraid that more complete discussions around this will have to wait for next year.

jonathan-buttner · 2024-12-18T14:46:50Z

Postman makes it look mainly like JSON, but those are server-sent events. Is this unified API going to be used for multiple providers? OpenAI, Anthropic Claude, and Google Gemini all use server-sent events but then provide really different structures: https://til.simonwillison.net/llms/streaming-llm-apis.

If yes, then clients would need to know what provider this is (in the content-type header?). There would not be a need to map the options in the spec; it's OK to treat it as an opaque string or buffer. Also, I'm afraid that more complete discussions around this will have to wait for next year.

Postman makes it look mainly like JSON, but those are server-sent events.

Yeah that makes sense

Is this unified API going to be used for multiple providers? OpenAI, Anthropic Claude, and Google Gemini all use server-sent events but then provide really different structures: https://til.simonwillison.net/llms/streaming-llm-apis.

Sorry I should have provided more details when opening the PR. The hope with this API is to provide a consistent request and response schema that will be the same across multiple providers. Internally we'll handle transforming the request and response to match the individual providers and our schema.

When we get a request for Anthropic we'll translate our "unified" schema into a request for Anthropic specifically and then as we get responses from Anthropic we'll translate it back into the format that fits the response schema we're proposing here.

… into ml-unified-spec

l-trotta · 2025-01-09T12:25:08Z

Hey @jonathan-buttner! the team had a discussion about this and we agreed to keep the result as a simple ArrayBuffer, and leave it to the users to parse it using the tools they prefer. This means adding a new type in the Binary.ts file:

// Streaming endpoints response
export type StreamResult = ArrayBuffer

and using that as the whole response, so UnifiedResponse.ts becomes:

export class Response {
  body: StreamResult
}

the specific classes modeled only for the response can therefore be deleted.
Every other comment I made regarding the request are still valid, let me know if there's something I could explain better or if you need any help with this.
Thank you :)

… into ml-unified-spec

github-actions · 2025-01-10T19:52:36Z

Following you can find the validation results for the APIs you have changed.

API	Status	Request	Response
`inference.unified_inference`	⚪	Missing test	Missing test

You can validate these APIs yourself by using the make validate target.

github-actions · 2025-01-13T11:27:18Z

Following you can find the validation results for the APIs you have changed.

API	Status	Request	Response
`inference.unified_inference`	⚪	Missing test	Missing test

You can validate these APIs yourself by using the make validate target.

l-trotta

LGTM! thank you

* Adding the unified api * Fixing formatting * Addressing feedback and removing response --------- Co-authored-by: Laura Trotta <[email protected]> (cherry picked from commit 2a57733)

* Adding the unified api * Fixing formatting * Addressing feedback and removing response --------- Co-authored-by: Laura Trotta <[email protected]> (cherry picked from commit 2a57733) Co-authored-by: Jonathan Buttner <[email protected]>

Adding the unified api

ea37f16

jonathan-buttner added the specification label Dec 16, 2024

jonathan-buttner requested review from davidkyle and maxhniebergall December 16, 2024 19:44

jonathan-buttner requested a review from a team as a code owner December 16, 2024 19:44

jonathan-buttner commented Dec 16, 2024

View reviewed changes

Fixing formatting

5b80fae

pquentin reviewed Dec 17, 2024

View reviewed changes

maxhniebergall reviewed Dec 17, 2024

View reviewed changes