-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Python: Fix #3970: Python: [Bug]: AgentResponse.value is None when streaming workflow
#4286
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -935,6 +935,13 @@ def _propagate_conversation_id(update: AgentResponseUpdate) -> AgentResponseUpda | |
| session.service_session_id = conv_id | ||
| return update | ||
|
|
||
| def _stream_finalizer(updates: Sequence[AgentResponseUpdate]) -> AgentResponse: | ||
| # Read response_format from merged chat_options (includes default_options) | ||
| # so that response_format set via the constructor is respected. | ||
| ctx = ctx_holder["ctx"] | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor robustness note: if
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit (non-blocking): |
||
| response_format = ctx["chat_options"].get("response_format") if ctx else None | ||
| return self._finalize_response_updates(updates, response_format=response_format) | ||
|
|
||
| return ( | ||
| ResponseStream | ||
| .from_awaitable(_get_stream()) | ||
|
|
@@ -943,9 +950,7 @@ def _propagate_conversation_id(update: AgentResponseUpdate) -> AgentResponseUpda | |
| map_chat_to_agent_update, | ||
| agent_name=self.name, | ||
| ), | ||
| finalizer=partial( | ||
| self._finalize_response_updates, response_format=options.get("response_format") if options else None | ||
| ), | ||
| finalizer=_stream_finalizer, | ||
| ) | ||
| .with_transform_hook(_propagate_conversation_id) | ||
| .with_result_hook(_post_hook) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -97,6 +97,32 @@ async def test_chat_client_agent_run_streaming(client: SupportsChatGetResponse) | |
| assert result.text == "test streaming response another update" | ||
|
|
||
|
|
||
| async def test_chat_client_agent_run_streaming_response_format_via_default_options( | ||
| client: SupportsChatGetResponse, | ||
| ) -> None: | ||
| """response_format set in default_options must be available on the streamed AgentResponse.value (issue #3970).""" | ||
| from pydantic import BaseModel | ||
|
|
||
| class Greeting(BaseModel): | ||
| greeting: str | ||
|
|
||
| json_text = '{"greeting":"hello"}' | ||
| client.streaming_responses = [ # type: ignore[attr-defined] | ||
| [ChatResponseUpdate(contents=[Content.from_text(json_text)], role="assistant")], | ||
| ] | ||
|
|
||
| agent = Agent(client=client, default_options={"response_format": Greeting}) | ||
| stream = agent.run("Hello", stream=True) | ||
| async for _ in stream: | ||
| pass | ||
| result = await stream.get_final_response() | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good: the test covers the exact bug scenario (default_options response_format in streaming). Consider adding a sibling test that passes response_format directly in run() options to ensure the refactored finalizer handles both paths. |
||
|
|
||
| assert result.text == json_text | ||
| assert result.value is not None | ||
| assert isinstance(result.value, Greeting) | ||
| assert result.value.greeting == "hello" | ||
|
|
||
|
|
||
| async def test_chat_client_agent_create_session(client: SupportsChatGetResponse) -> None: | ||
| agent = Agent(client=client) | ||
| session = agent.create_session() | ||
|
|
||
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.
This defensive
if ctx else Nonecheck is not exercised by any test. Consider whether ctx can realistically be None here; if not, dropping the guard (or adding a test that triggers it) would improve clarity.