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

Robust SSE parsing #336

Closed
wants to merge 3 commits into from
Closed

Robust SSE parsing #336

wants to merge 3 commits into from

Conversation

burke
Copy link

@burke burke commented Oct 4, 2023

I took @atesgoral's work in #332 and ran with it a bit

A few things fixed up or adjusted here:

  1. OpenAI always sends "data: [DONE]\n\n" at the end of responses; we can assert that was received to detect aborted connections.
  2. OpenAI never sends "error: ...\n\n" messages: errors are passed inside the JSON object attached to the "data: {...}\n\n".
  3. I'm not aware of OpenAI ever sending malformed JSON, so erroring on that (new behaviour) feels correct to me.

I also made a few other changes along the way:

  1. Mostly switched from require_relative to require
  2. Changed up-front nested class/module loading to autoloads for (trivial runtime performance gain for consumers and) making it possible to refer to OpenAI::Error in OpenAI::SSE without having to put the requires at the bottom of openai.rb.

Let me know what you think, I'm happy to keep adjusting here.

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?

atesgoral and others added 3 commits September 26, 2023 22:49
A few things fixed up or adjusted here:

1. OpenAI always sends "data: [DONE]\n\n" at the end of responses; we
   can assert that was received to detect aborted connections.
2. OpenAI never sends "error: ...\n\n" messages: errors are passed
   inside the JSON object attached to the "data: {...}\n\n".
3. I'm not aware of OpenAI ever sending malformed JSON, so erroring on
   that (new behaviour) feels correct to me.

I also made a few other changes along the way:

1. Mostly switched from require_relative to require
2. Changed up-front nested class/module loading to autoloads for
   (trivial runtime performance gain for consumers and) making it
   possible to refer to OpenAI::Error in OpenAI::SSE without having to
   put the requires at the bottom of openai.rb.

Let me know what you think, I'm happy to keep adjusting here.
@vizakenjack
Copy link

vizakenjack commented Oct 9, 2023

There is an issue that should be fixed (both in original ruby-openai and your pull request).

How to reproduce: try to send a pretty long text that won't fit into context. There will be a message:

buffer = "{\n \"error\": {\n \"message\": \"This model's maximum context length is 4097 tokens. However, you requested 4235 tokens (2235 in the messages, 2000 in the completion). Please reduce the length of the messages or completion.\",\n \"type\": \"invalid_request_error\",\n \"param\": \"messages\",\n \"code\": \"context_length_exceeded\"\n }\n}\n"

I had fix it in my own fork

@atesgoral
Copy link
Contributor

@vizakenjack That error sounds like just hitting token limits. What was your fix?

Copy link

@fabioxgn fabioxgn left a comment

Choose a reason for hiding this comment

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

@alexrudall can we get this merged? I'm having an issue where the chunks are being split in half and the regex on to_json_stream does not match the partial chunk, so it's ignored.

This branch fixes that issue.

This is what I'm getting in to_json_stream:

chunk
=> "98,\"model\":\"gpt-4-0613\",\"choices\":[{\"index\":0,\"delta\":{\"role\":\"assistant\",\"content\":\"\"},\"finish_reason\":null}]}\n\ndata: {\"id\":\"chatcmpl-8FTgAGg922FFFVvUVdLp8raDbny4j\",\"object\":\"chat.completion.chunk\",\"crea"

This does not match chunk.scan(/(?:data|error): (\{.*\})/i) so this chunk is ignored.

@alexrudall
Copy link
Owner

alexrudall commented Oct 30, 2023 via email

@fabioxgn
Copy link

Hey @fabioxgn, have you tried v5.2? That should fix your issue. (I still probably will merge this)

On Mon, 30 Oct 2023 at 21:14, Fábio Gomes @.> wrote: @.* approved this pull request. @alexrudall https://github.com/alexrudall can we get this merged? I'm having an issue where the chunks are being split in half and the regex on to_json_stream does not match the partial chunk, so it's ignored. This branch fixes that issue. This is what I'm getting in to_json_stream: chunk => "98,"model":"gpt-4-0613","choices":[{"index":0,"delta":{"role":"assistant","content":""},"finish_reason":null}]}\n\ndata: {"id":"chatcmpl-8FTgAGg922FFFVvUVdLp8raDbny4j","object":"chat.completion.chunk","crea" This does not match chunk.scan(/(?:data|error): ({.})/i) so this chunk is ignored. — Reply to this email directly, view it on GitHub <#336 (review)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABWXYXWF4KFHATLMQSGB52DYCAKBRAVCNFSM6AAAAAA5TGJYC6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTOMBVGA4DKMZRHA . You are receiving this because you were mentioned.Message ID: @.**>

Hi @alexrudall, yes, that fixed my issue. Thank you!

@alexrudall
Copy link
Owner

Big thanks for your work on this @burke - SSE parsing is now much improved / fixed with @atesgoral's PR, so I think this one is not needed. Definitely open to the autoload changes if you want to cherrypick them to a new PR. Thanks!

@alexrudall alexrudall closed this Nov 6, 2023
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.

5 participants