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

Streaming fixes. #558

Merged
merged 4 commits into from
May 18, 2024
Merged

Streaming fixes. #558

merged 4 commits into from
May 18, 2024

Conversation

SebastianStehle
Copy link
Contributor

Hi,

I solved a few issues around streaming:

  1. 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

  2. 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.

  3. Chat completion response did not contain the same header information as the normal API. I have added the parsing here.

  4. 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.

@kayhantolga kayhantolga changed the base branch from master to dev May 17, 2024 12:08
@kayhantolga
Copy link
Member

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.
Please check the logs from your sample(at issue). You'll see that the role is sent only once, but we have index 0 and index 1. The role is lost after index 0. (I've removed some of the parameters from json for simplicity.)

data: {"id":"c", "choices":[{"index":0,"delta":{"role":"assistant","content":null},"logprobs":null,"finish_reason":null}]}
data: {"id":"c", "choices":[{"index":0,"delta":{"tool_calls":[{"index":0,"id":"call_qhX0gWXn4hMJhy1eICxq3Oxy","type":"function","function":{"name":"add","arguments":""}}]},"logprobs":null,"finish_reason":null}]}
data: {"id":"c", "choices":[{"index":0,"delta":{"tool_calls":[{"index":0,"function":{"arguments":"{\"lh"}}]},"logprobs":null,"finish_reason":null}]}
data: {"id":"c", "choices":[{"index":0,"delta":{"tool_calls":[{"index":0,"function":{"arguments":"s\": 1"}}]},"logprobs":null,"finish_reason":null}]}
data: {"id":"c", "choices":[{"index":0,"delta":{"tool_calls":[{"index":0,"function":{"arguments":"0, \"rh"}}]},"logprobs":null,"finish_reason":null}]}
data: {"id":"c", "choices":[{"index":0,"delta":{"tool_calls":[{"index":0,"function":{"arguments":"s\": "}}]},"logprobs":null,"finish_reason":null}]}
data: {"id":"c", "choices":[{"index":0,"delta":{"tool_calls":[{"index":0,"function":{"arguments":"12}"}}]},"logprobs":null,"finish_reason":null}]}
data: {"id":"c", "choices":[{"index":0,"delta":{"tool_calls":[{"index":1,"id":"call_A9ttvzn94yeMCWY5HeNY9tFX","type":"function","function":{"name":"add","arguments":""}}]},"logprobs":null,"finish_reason":null}]}
data: {"id":"c", "choices":[{"index":0,"delta":{"tool_calls":[{"index":1,"function":{"arguments":"{\"lh"}}]},"logprobs":null,"finish_reason":null}]}
data: {"id":"c", "choices":[{"index":0,"delta":{"tool_calls":[{"index":1,"function":{"arguments":"s\": 1"}}]},"logprobs":null,"finish_reason":null}]}
data: {"id":"c", "choices":[{"index":0,"delta":{"tool_calls":[{"index":1,"function":{"arguments":"2, \"rh"}}]},"logprobs":null,"finish_reason":null}]}
data: {"id":"c", "choices":[{"index":0,"delta":{"tool_calls":[{"index":1,"function":{"arguments":"s\": "}}]},"logprobs":null,"finish_reason":null}]}
data: {"id":"c", "choices":[{"index":0,"delta":{"tool_calls":[{"index":1,"function":{"arguments":"10}"}}]},"logprobs":null,"finish_reason":null}]}
data: {"id":"c", "choices":[{"index":0,"delta":{},"logprobs":null,"finish_reason":"tool_calls"}]}
data: [DONE]
data: {"id":"c", "choices":[{"index":0,"delta":{"role":"assistant","content":""},"logprobs":null,"finish_reason":null}]}
data: {"id":"c", "choices":[{"index":0,"delta":{"content":"The"},"logprobs":null,"finish_reason":null}]}
data: {"id":"c", "choices":[{"index":0,"delta":{"content":" result"},"logprobs":null,"finish_reason":null}]}
data: {"id":"c", "choices":[{"index":0,"delta":{"content":" of"},"logprobs":null,"finish_reason":null}]}
data: {"id":"c", "choices":[{"index":0,"delta":{"content":" "},"logprobs":null,"finish_reason":null}]}
data: {"id":"c", "choices":[{"index":0,"delta":{"content":"10"},"logprobs":null,"finish_reason":null}]}
data: {"id":"c", "choices":[{"index":0,"delta":{"content":" plus"},"logprobs":null,"finish_reason":null}]}
data: {"id":"c", "choices":[{"index":0,"delta":{"content":" "},"logprobs":null,"finish_reason":null}]}
data: {"id":"c", "choices":[{"index":0,"delta":{"content":"12"},"logprobs":null,"finish_reason":null}]}
data: {"id":"c", "choices":[{"index":0,"delta":{"content":" is"},"logprobs":null,"finish_reason":null}]}
data: {"id":"c", "choices":[{"index":0,"delta":{"content":" "},"logprobs":null,"finish_reason":null}]}
data: {"id":"c", "choices":[{"index":0,"delta":{"content":"22"},"logprobs":null,"finish_reason":null}]}
data: {"id":"c", "choices":[{"index":0,"delta":{"content":"."},"logprobs":null,"finish_reason":null}]}
data: {"id":"c", "choices":[{"index":0,"delta":{},"logprobs":null,"finish_reason":"stop"}]}
data: [DONE]

@kayhantolga kayhantolga merged commit 05d8db0 into betalgo:dev May 18, 2024
@SebastianStehle
Copy link
Contributor Author

I see. I would probably just maintain a "latestRole" variable in the streaming method and set the role to that one.

@SebastianStehle
Copy link
Contributor Author

Btw: I think there are more streaming methods with no error handling.

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.

None yet

2 participants