- Notifications
You must be signed in to change notification settings - Fork 497
Support comments in streaming response #259
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
Support comments in streaming response #259
Conversation
|
| @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 |
| @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 |
| @nezhyborets Ready for review 👍 Works well in my testing with OpenRouter and OpenAI, and handles comments well in a real-world scenario. |
There was a problem hiding this 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 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 { // 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 /// 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. |
| @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? |
| @SplittyDev I'm also thinking if we should mention our "support different providers" efforts in Readme. Maybe something like Supporting different providers We've added some provider-specific changes to provide a better support: 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 |
| @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:
Anthropic is a whole different can of worms:
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 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 |
| @SplittyDev I think we can go with the easiest one first, i.e. add citations for Perplexity. As for |
…-comments-merge Support comments in streaming response



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 multipledata: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
https://html.spec.whatwg.org/multipage/server-sent-events.html#parsing-an-event-stream ↩