Skip to content

Conversation

@SplittyDev
Copy link
Contributor

@SplittyDev SplittyDev commented Feb 4, 2025

What

This PR adjusts the streaming response parsing logic to gracefully deal with comments.
Supersedes #252.

Why

Server Sent Events, the standard used by the OpenAI API and compatible APIs, support comments by definition1, but the parsing code did not deal with them gracefully. Instead, it caused a decoding error, trying to parse the comment as JSON.

Even though the official OpenAI API does not seem to be using comments as far as I can tell, other OpenAI-compatible APIs are using them (e.g. OpenRouter), and they are part of the standard, so should probably be supported anyway.

Honestly, that part of the code probably needs a bigger overhaul in the future, because the parsing logic there is far from robust, and far from correct. It's correct enough to handle the specific way that OpenAI is using Server Sent Events, but it ignores many parts of the spec, such as multi-line events that can have multiple data: parts while still being the same event.

The PR now includes a refactoring of the stream data processing code, and I think that mostly addresses these claims I made earlier. Probably still needs some stricter compliance testing to make sure we handle edge-cases well, but it's more than good enough for me in its current state.

Affected Areas

Only the stream response parsing is affected.

Mentions

Huge thanks to @nezhyborets for contributing tests and refactoring!

Footnotes

  1. https://html.spec.whatwg.org/multipage/server-sent-events.html#parsing-an-event-stream

@SplittyDev
Copy link
Contributor Author

@nezhyborets I've closed the other PR. I've rolled back my main to the commit you based your PR on and cherry-picked your commit. I'll have another quick look to see whether the tests have to be adjusted

@SplittyDev
Copy link
Contributor Author

@nezhyborets Alright, I don't think I need to bring over any of the changes I made in my second commit. They only concern tests, and it seems you're already testing chunks and comments in your StreamInterpreter tests.

I'll bring this over to my app and make sure it properly handles some edge cases, and I'll be right back in a minute

@SplittyDev
Copy link
Contributor Author

@nezhyborets Ready for review 👍 Works well in my testing with OpenRouter and OpenAI, and handles comments well in a real-world scenario.

Copy link
Collaborator

@nezhyborets nezhyborets left a comment

Choose a reason for hiding this comment

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

Great job, thanks!

@nezhyborets nezhyborets merged commit c1a8a0b into MacPaw:main Feb 4, 2025
5 checks passed
@SplittyDev
Copy link
Contributor Author

SplittyDev commented Feb 4, 2025

@nezhyborets Are you interested in adding further compatibility with other providers, or should this be a strict OpenAI-specific implementation?

This PR was totally in scope because it improves the implementation of the event standard used by OpenAI, but some other things I'm planning to add to my fork are not quite as clearly in scope for upstream contributions.

For example, in my fork I'm adding support for some more exotic features, such as Perplexity's citations. I can contribute that stuff upstream, but I don't know whether you want to stray that far from the OpenAI format.

For example, Perplexity adds a citations key as part of the response:

{ // example response "id": "410e6fb9-8325-1f25-adf8-73718ba6074d", "model": "sonar", "created": 1738684359, "citations": [ "https://example.org/citation1", "https://example.org/citation2" ], "choices": [ /* ... */ ] }

This is generally quite compatible with OpenAI, because the citations field can simply be optional:

/// The model used for the chat completion. public let model: String + /// A list of citations for the completion. + public let citations: [String]?

But it might be a bit confusing for people who plan to use it strictly with OpenAI and expect the field to actually be used in some cases, which in the case of OpenAI will never be true, unless they decide to add citations in the future.

Please let me know whether you're interested in adding support for stuff like that.

@nezhyborets
Copy link
Collaborator

nezhyborets commented Feb 5, 2025

@SplittyDev thanks for the suggestion and willing to contribute! The "strategy" we've discussed with @Krivoblotsky is to support different providers.

The solution to just add the field (and synthesized CodingKey) might not be the best one as other providers may have different key name for the same data. But I also don't see a better, more universal solution. Let's just add the field and see how it goes. You're right that adding an optional field doesn't seem to add a potential harm.

Would you do the PR?

@nezhyborets
Copy link
Collaborator

nezhyborets commented Feb 5, 2025

@SplittyDev I'm also thinking if we should mention our "support different providers" efforts in Readme. Maybe something like


Supporting different providers
This lib supports different providers that have OpenAI compatible API, like Openrouter, Gemini, Perplexity.

We've added some provider-specific changes to provide a better support:
Perplexity: citations field added to ChatResult.


It might be helpful not just for people, but also for us to track such little provider-specific injections, so that later it's easier to review them and maybe come up with something more universal

@SplittyDev
Copy link
Contributor Author

@nezhyborets Thanks for letting me know! I'll prepare some PRs as I add more features.

One thing to note is that compatibility with other providers generally isn't great. OpenRouter is well-supported because it very closely mimics the OpenAI API (other than the comments in the event stream), but some other providers have several challenges that have to be addressed in different ways.

Here's a few off the top of my head:

  • Perplexity includes a citations field in responses
  • Google AI Studio doesn't use /v1, but /v1beta/openai
  • DeepSeek officially doesn't use /v1, but supports it for compatibility reasons
  • Other providers might require additional headers too

Anthropic is a whole different can of worms:

  • Uses a slightly different request and response format
  • Has the system prompt as a top-level string instead of a role
  • Does not allow multiple assistant messages without a user message in-between
  • Requires special headers, without which it refuses to run
  • ...and probably some more stuff I haven't even noticed yet

So far, in my fork I've added proper support for Perplexity citations, and for arbitrary headers.

Google AI Studio compatibility is a bit more complicated because the basePath does not include the /v1, which is currently hardcoded for all endpoints. Supporting this requires an extensive refactoring, and is possibly a breaking change because people might use the basePath already without including /v1.

Anthropic is even more complicated, because of their different format. For Anthropic, the request and response types cannot be reused at all, and I'm not sure how this should even be addressed. I would personally just opt for not supporting Anthropic, but I guess the other option is to have an Anthropic client that works similar to the OpenAI one, but uses other types. That would almost justify its own separate library though, so I don't think that makes sense.

@nezhyborets
Copy link
Collaborator

nezhyborets commented Feb 7, 2025

@SplittyDev I think we can go with the easiest one first, i.e. add citations for Perplexity.

As for v1, you're right that it might be a breaking change. But we've actually just recently introduced basePath, so maybe it won't be that much of a problem if we move v1 from paths to basePath now... before more people adopt it

wangqi pushed a commit to wangqi/OpenAI that referenced this pull request Feb 18, 2025
…-comments-merge Support comments in streaming response
@SplittyDev SplittyDev deleted the feat/support-streaming-comments-merge branch February 23, 2025 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants