-
Notifications
You must be signed in to change notification settings - Fork 6.5k
Fix AzureOpenAI streaming token usage #19633
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
Fix AzureOpenAI streaming token usage #19633
Conversation
|
I was curious why this check was there, so I went digging Basically, the old agent classes would detect if a response was a final response or a tool call by checking if there was text returned first or a tool call. By returning an empty delta, it was breaking that check Since those old agent classes are deprecated and no longer supported, this should be ok to merge? Although I worry a little bit about making a change like this. I feel like we should not be yielding empty delta's in the first place. I might add check before the yield? |
|
Hey @logan-markewich, agreed with your concern. I added a check for this. If choices are empty and usage is also None, we simply skip that chunk from yielding. Although, usage chunk is the last chunk in the streaming response, but lets be extra careful. |
|
will add test |
|
one thing is to note: I have not written test for async equivalent, i could not find any existing test for any async functions (even checked some other integrations). Anyways, they both have exactly same changes so this will be ok? |
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.
Ok, I changed my mind, I think avoiding yielding is more breaking
Going to go back to the first iteration
Description
The streaming response did not have usage info in case of AzureOpenAI when using
{"stream_options": {"include_usage": True}}. OpenAI works fine but AzureOpenAI had this issue. AzureOpenAI depends on OpenAI base class and previously had following logic:Not sure, why we skipped in case of AzureOpenAI. We can do the same thing as being handled in case of OpenAI for empty choices message. Now, we only have
delta = ChoiceDelta()in case of empty choices message.Before:
After
Version Bump?
Did I bump the version in the
pyproject.tomlfile of the package I am updating? (Except for thellama-index-corepackage)Type of Change
Please delete options that are not relevant.
How Has This Been Tested?
I tested using the following snippet:
Suggested Checklist:
uv run make format; uv run make lintto appease the lint gods