Skip to content

OpenAI: Add support for getting token usage when using streaming #920

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

Closed

Conversation

didalgolab
Copy link
Contributor

Fixes #814

I'm aware of PR #829, but I would like to propose a more complete solution here.

This PR:

  • Adds support for the stream_options chat completion request property.
  • Prevents sending stream_options if the call instead of the stream method is used (otherwise, OpenAI API returns an error).
  • Adds an integration test comparing token usage obtained with the call and stream methods.

@tzolov tzolov self-assigned this Jun 22, 2024
@tzolov tzolov added this to the 1.0.0-M2 milestone Jun 22, 2024
@tzolov
Copy link
Contributor

tzolov commented Jun 23, 2024

Thanks @didalgolab ,
I wonder if the include usage shouldn't be enabled by default. In which case the question of opting out became relevant.
We can introduce a shortcut withStreamUsage(boolean) that will set the StreamOptions.INCLUDE_USAGE.
Will try to do this while merging your PR.

@didalgolab
Copy link
Contributor Author

didalgolab commented Jun 23, 2024

I wondered about that too. I'd love to make it enabled by default! However, there is an issue with using OpenAiApi with different providers (e.g., Groq, OpenRouter...) in such case. As far as I know, none of them support stream_options, although some stream token usage out of the box without any opt-in. Enabling the include_usage option by default would make the default chat options incompatible with different providers' endpoints, requiring opt-out in the code each time. If you are okay with that, I am too.

Or perhaps OpenAiChatModel could detect if the default baseUrl is used and enable or disable stream_options in the default options accordingly, although I must admit I generally dislike conditional logic like that.

Feel free to share any thoughts or suggestions about this.

Adding a shortcut withStreamUsage(boolean) is an excellent idea.

@tzolov
Copy link
Contributor

tzolov commented Jun 24, 2024

@didalgolab, I've added the Builder shortcut along with a field-less boolean getter/setters to provide astream-usage property.
Updated the docs, tests and did some small amendments.

extended, rebased, squashed and merged at 20e4b56

@tzolov tzolov closed this Jun 24, 2024
tzolov added a commit that referenced this pull request Jun 24, 2024
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.

Add Support for get usage tokens
2 participants