Skip to content
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

Add Pekko streaming support for chat completions #128

Merged
merged 5 commits into from
Nov 8, 2023
Merged

Conversation

DybekK
Copy link
Contributor

@DybekK DybekK commented Nov 7, 2023

This PR adds streaming support for Create chat completion endpoint using Pekko Streams as a streams implementation.

Here are the main changes made in this PR:

  • out-of-the-box streaming support has been added using the Pekko Streams library by extending current OpenAI class.
  • integration tests have been added using PekkoHttpBackend.stub

Minor change:

  • DeserializationException was replaced with more detailed DeserializationOpenAIException during tests

@DybekK DybekK requested a review from kciesielski November 7, 2023 14:37

private def deserializeEvent: Flow[ServerSentEvent, ChatChunkResponse, Any] =
Flow[ServerSentEvent].collect {
case ServerSentEvent(Some(data), _, _, _) if data != DoneEventMessage =>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

** issue ** This makes me thinking - the DoneEventMessage is simply filtered here, and we basically rely on the stream end to be signalised. Maybe we should rather takeWhile instead of collect, which would guarantee that DoneEventMessage completes the stream right away.
For that, we may need a test where there are more messages after DoneEventMessage, it's unexpected but we would be sure that "done" means "done" for us, no matter what happens.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly with fs2 and zio, these modules may also need such an improvement.

Copy link
Contributor Author

@DybekK DybekK Nov 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for that suggestion, I added takeWhile to fs2, zio and pekko.

@DybekK DybekK merged commit ed1fa33 into master Nov 8, 2023
5 checks passed
@DybekK DybekK deleted the pekko-streaming branch November 8, 2023 08:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants