Openai api sse review #172
Conversation
| private static final String DEFAULT_BASE_URL = "https://api.openai.com"; | ||
| private static final String DEFAULT_EMBEDDING_MODEL = "text-embedding-ada-002"; | ||
| private static final String SSE_DONE = "[DONE]"; | ||
| private static final Predicate<String> SSE_DONE_PREDICATE = "[DONE]"::equals; |
There was a problem hiding this comment.
I presume that the Predicate is just a code style improvement, not due to issues with the existing conditions?
There was a problem hiding this comment.
The documentation at https://platform.openai.com/docs/api-reference/chat/create explicitly states that it ends with '[DONE]', so in my opinion, using 'equals' would be clearer than 'contains.
| @JsonProperty("delta") Delta delta | ||
| ) { | ||
| // The delta used in ChatCompletionChunk is not identical to ChatCompletion's ChatCompletionMessage, so a Delta record needs to be defined separately. | ||
| public record Delta( |
There was a problem hiding this comment.
I'm afraid that this is incorrect. The message format is the same in both the chat completion object and the chat completion chunk object and it includes the tool_calls hierarchy as well.
There was a problem hiding this comment.
I misunderstood. Thank you for letting me know.
|
|
||
| this.objectMapper = new ObjectMapper(); | ||
| // Use the same ObjectMapper for WebClient's response as the one used in RestClient. | ||
| this.objectMapper = Jackson2ObjectMapperBuilder.json().build(); |
There was a problem hiding this comment.
This is interesting, need to dig a bit more.
The recent streaming response parsing failure was due to the unexpected (for me) appearance of the LogProbs in the chunk response. Not sure if i missed it or OpenAI introduced it silently. Hope it is the former.
I deliberately didn't configure the objectmapper to ignore unknown fields so i can catch changes. But didn't expect they will come without announcement and also didn't consider that the webclient and restclient converters are likely ignoring such fields.
There was a problem hiding this comment.
I encountered an error during code review. It appears that LogProbs were added at some point. After checking OpenAI's API specification at https://platform.openai.com/docs/api-reference/chat/object, I noticed the presence of LogProbs. However, in the response for the ChatCompletion - Choice record when the stream is false, LogProbs are not included. To address this issue, I realized that a similar approach is needed for WebClient, similar to the ObjectMapper in RestClient, to ignore unknown fields. Therefore, I added the same configuration in the commit.
Instead of adding LogProbs to ChatCompletion - Choice and ChatCompletionChunk record, considering the continuous changes and potential additions or deprecations in OpenAI's spec, I believe it's more beneficial for developers using Spring AI to configure the ObjectMapper to ignore unknown fields. This way, they won't have to deal with too many specific options. Hence, I implemented the ObjectMapper configuration in the current commit.
There was a problem hiding this comment.
We already added the LogProbs in another commit.
But with this PR I'll configure the object mapper to tolerate unknown properties.
Would live out the Jackson2ObjectMapperBuilder for now.
|
Rebased, squashed and merged at b54bb8b |
2 participants
@tzolov, I took a close look at the code and ran the beanStreamOutputParserRecords test in OpenAIClientIT. I'm sorry for the delayed response; I ran into an issue and needed some time to address it.
I made some changes, including defining the Delta for ChatCompletionChunk within the class to handle OpenAI's SSE for ChatCompletionChunk objects. I also adjusted the ObjectMapper for generating response JSON to align with the one used in RestClient.
Additionally, I refactored the code related to ending the Flux Stream and put it into a separate commit.
Please check out the draft pull request, and feel free to reach out if you have any questions or need further clarification.