-
-
Notifications
You must be signed in to change notification settings - Fork 530
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
Streaming fixes. #558
Streaming fixes. #558
Conversation
I've found the cause of the issue. I made slight adjustments to your temporary solution, but we may need to revise the code for a more suitable fix. I've made a note of this in a TODO comment.
|
I see. I would probably just maintain a "latestRole" variable in the streaming method and set the role to that one. |
Btw: I think there are more streaming methods with no error handling. |
Hi,
I solved a few issues around streaming:
Chat completion has a parameter to also return usage data. I think this should be added:
https://github.com/betalgo/openai/compare/master...SebastianStehle:openai:streaming-fixes?expand=1#diff-7723ad521684e749392e631a6e44d0ce3f33cdae5c9af2466e7bc52428dd7c37R50
Chat completion and completions has actually no error handling in place. This was also the root cause of my issue Stream returns no message after tool call. #556.
Chat completion response did not contain the same header information as the normal API. I have added the parsing here.
Something weird. OpenAI does not set the role of the response chunks to assistant. Therefore you cannot add the message directly to the context as you would do in a non-streaming scenario. Therefore I added a workaround for that to be the 2 endpoints more similar: https://github.com/betalgo/openai/compare/master...SebastianStehle:openai:streaming-fixes?expand=1#diff-d34076ca61932bc94550bf6bf3aeba9c9a2cf9508251ca2182e11683d9c98a5fR141
I have also extended the playground to make actual tool calls. I think the playground should be converted to an integration tests project. You do not have to add that to the CI, but it would be great to have something better.
Btw: I also added 2 places of a old school null comparison
if (block != null)
.I hope we can get that out asap.