-
Notifications
You must be signed in to change notification settings - Fork 13.5k
server: fix SSE and OpenAI compatibility for error messages when streaming #16109
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
server: fix SSE and OpenAI compatibility for error messages when streaming #16109
Conversation
tools/server/server.cpp
Outdated
| } | ||
| }, [&](const json & error_data) { | ||
| server_sent_event(sink, "error", error_data); | ||
| server_sent_event(sink, "data", json{{"error", error_data}}); |
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.
Seems like we no longer need the second argument of server_sent_event, as it will always be "data"
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.
Yeah that makes sense.
Should this be kept in a way to make it compatible with the actual event field of the SSE spec (in a line above the data - if specified) or should that parameter be completely removed and simply always only use the data line?
e.g.
event: error
data: {error: {....}}
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.
I think most parsers will accept format data: {error: {....}}, anything outside of that always caused problems for downstream in the past, like the data: [DONE] for example.
|
cc @allozaur you may need to update web ui code too (not urgent) |
…aming (ggml-org#16109) * server: fix SSE and OpenAI compatibility for error messages when streaming * server: remove obsolete event parameter and use required data fieldname instead
…aming (ggml-org#16109) * server: fix SSE and OpenAI compatibility for error messages when streaming * server: remove obsolete event parameter and use required data fieldname instead
…aming (ggml-org#16109) * server: fix SSE and OpenAI compatibility for error messages when streaming * server: remove obsolete event parameter and use required data fieldname instead
This fixes compatibility with the SSE specification by not sending error messages out with an incompatible fieldname.
Therefore this also ensures proper exceptions when working for example with OpenAI client libraries, instead of being handled as an empty message.
Closes:
#16104