-
Notifications
You must be signed in to change notification settings - Fork 17k
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
openai[patch]: support streaming token counts in AzureChatOpenAI #30494
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
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.
Pull Request Overview
This PR adds support for streaming token counts in AzureChatOpenAI by introducing a stream_usage attribute and updating how stream_options are handled in BaseChatOpenAI. Key changes include:
- Adding a stream_usage attribute with documentation to control the inclusion of usage metadata during streaming.
- Modifying the _stream and _astream methods to determine and set stream_options based on stream_usage.
- Updating integration tests for AzureChatOpenAI by adding the "stream_usage" parameter and removing xfail’d streaming metadata tests.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
File | Description |
---|---|
libs/partners/openai/tests/integration_tests/chat_models/test_azure_standard.py | Updated test parameters to enable stream_usage and removed obsolete tests for usage metadata streaming. |
libs/partners/openai/langchain_openai/chat_models/base.py | Introduced a stream_usage attribute and refactored stream handling by consolidating _should_stream_usage logic and routing requests based on API type. |
Comments suppressed due to low confidence (2)
libs/partners/openai/tests/integration_tests/chat_models/test_azure_standard.py:35
- The removal of the xfail’d test for usage metadata streaming reduces test coverage for stream_usage behavior. Consider adding or updating tests to verify that streaming token counts behave as expected for AzureChatOpenAI.
@pytest.mark.xfail(reason="Not yet supported.")
libs/partners/openai/langchain_openai/chat_models/base.py:2268
- The redundant _should_stream_usage and _stream method implementations were removed in favor of a consolidated approach. Please verify that the new unified logic covers all stream usage scenarios, especially for API calls where stream_options may be unsupported.
def _should_stream_usage(self, stream_usage: Optional[bool] = None, **kwargs: Any) -> bool:
def _stream( | ||
self, | ||
messages: List[BaseMessage], | ||
stop: Optional[List[str]] = None, | ||
run_manager: Optional[CallbackManagerForLLMRun] = None, | ||
*, | ||
stream_usage: Optional[bool] = None, |
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.
consider documenting why it's Optional[bool] rather than bool, it'll throw off many a developer
Co-authored-by: Eugene Yurtsev <[email protected]>
When OpenAI originally released
stream_options
to enable token usage during streaming, it was not supported in AzureOpenAI. It is now supported.Like the OpenAI SDK, ChatOpenAI does not return usage metadata during streaming by default (which adds an extra chunk to the stream). The OpenAI SDK requires users to pass
stream_options={"include_usage": True}
. ChatOpenAI implements a convenience argumentstream_usage: Optional[bool]
, and an attributestream_usage: bool = False
.Here we extend this to AzureChatOpenAI by moving the
stream_usage
attribute andstream_usage
kwarg (on_(a)stream
) from ChatOpenAI to BaseChatOpenAI.Additional consideration: we must be sensitive to the number of users using BaseChatOpenAI to interact with other APIs that do not support the
stream_options
parameter.Suppose OpenAI in the future updates the default behavior to stream token usage. Currently, BaseChatOpenAI only passes
stream_options
ifstream_usage
is True, so there would be no way to disable this new default behavior.To address this, we could update the
stream_usage
attribute toOptional[bool] = None
, but this is technically a breaking change (as currently values of False are not passed to the client). IMO: if / when this change happens, we could accompany it with this update in a minor bump.Related previous PRs: