fix: resolve Pydantic delta serialization warnings in agent streaming#2005
fix: resolve Pydantic delta serialization warnings in agent streaming#2005Vchen7629 wants to merge 7 commits into
Conversation
github-actions
Bot
added
backend
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Walkthrough
ChangesDelta serialization fix
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
bug
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/test_agent_stream_delta.py (1)
13-25: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSolid regression test. Optionally strengthen it by also asserting that the plain
chunk.model_dump()(without exclude) actually triggers the warning, proving this test would catch a regression if the workaround becomes unnecessary or ineffective.Optional companion assertion
with warnings.catch_warnings(): warnings.simplefilter("error") chunk_data = chunk.model_dump(exclude={"delta"}) chunk_data["delta"] = chunk.delta assert chunk_data["delta"] == {"content": "Hello"} assert chunk_data["type"] == "response.output_text.delta" + + +def test_model_dump_without_exclude_raises_the_warning() -> None: + chunk = FakeTextDeltaEvent.model_construct( + delta={"content": "Hello"}, type="response.output_text.delta" + ) + with warnings.catch_warnings(): + warnings.simplefilter("error") + with pytest.raises(UserWarning): + chunk.model_dump()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/test_agent_stream_delta.py` around lines 13 - 25, The regression test around FakeTextDeltaEvent.model_dump() should also prove the warning path is exercised, not just the workaround. In test_model_dump_excluding_delta_avoids_warning_and_preserves_dict_shape, add an assertion that calling chunk.model_dump() without exclude={"delta"} raises the expected warning, while keeping the existing excluded-dump check to verify the fix still preserves the dict shape.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/unit/test_agent_stream_delta.py`:
- Around line 13-25: The regression test around FakeTextDeltaEvent.model_dump()
should also prove the warning path is exercised, not just the workaround. In
test_model_dump_excluding_delta_avoids_warning_and_preserves_dict_shape, add an
assertion that calling chunk.model_dump() without exclude={"delta"} raises the
expected warning, while keeping the existing excluded-dump check to verify the
fix still preserves the dict shape.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0aecb6c6-f811-4327-ae24-eaf120d41e07
📒 Files selected for processing (2)
src/agent.pytests/unit/test_agent_stream_delta.py
…clusion raises the warning Signed-off-by: vchen7629 <[email protected]>
github-actions
Bot
added
bug
github-actions
Bot
added
bug
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/unit/test_agent_stream_delta.py (2)
18-24: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssertion doesn't verify the warning is the expected serialization warning.
assert recordedonly checks that some warning fired, not that it's thePydanticSerializationUnexpectedValue/serializer warning this test is meant to demonstrate. If an unrelated warning were emitted (e.g., a deprecation warning from another import), this assertion would still pass, masking a regression where the actual serialization warning stops firing.🔍 Suggested tightening
with warnings.catch_warnings(record=True) as recorded: warnings.simplefilter("always") chunk.model_dump() - assert recorded, "expected a serialization warning when delta has a type mismatch" + assert any( + "serializer warnings" in str(w.message) for w in recorded + ), "expected a Pydantic serialization warning when delta has a type mismatch"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/test_agent_stream_delta.py` around lines 18 - 24, The test in test_model_dump_without_exclusion_raises_serialization_warning only checks that some warning was emitted, so tighten it to assert the warning is the expected serialization warning from chunk.model_dump. Inspect the warnings captured in recorded and verify the specific warning category/message associated with PydanticSerializationUnexpectedValue rather than relying on assert recorded, so the test fails if an unrelated warning appears or the serializer warning stops firing.
13-34: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftTests validate Pydantic's own behavior, not the actual fix in
agent.py.Both tests independently replicate the
model_dump(exclude={"delta"})+ reattach pattern instead of invokingasync_response_stream(the function this PR actually changes, seesrc/agent.py:174-191). If a future edit toasync_response_streamregresses the exclude/reattach logic (e.g., typo in the exclude key, or forgetting to reattachdelta), these tests will still pass since they don't exercise that code path at all — they only prove that Pydantic'smodel_dumpbehaves as expected in isolation.Consider adding a test that drives an actual chunk through
async_response_stream(with a minimal async generator stub emitting aFakeTextDeltaEvent-like chunk) and asserts on the resulting SSE payload, in addition to (or instead of) these Pydantic-behavior-only unit tests.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/test_agent_stream_delta.py` around lines 13 - 34, The current tests only assert Pydantic’s standalone `model_dump` behavior and do not cover the actual logic changed in `async_response_stream`. Update the tests to drive a real chunk through `async_response_stream` in `src/agent.py` using a minimal async generator stub that yields a `FakeTextDeltaEvent`-like event, then assert the emitted SSE payload includes the expected `delta` and `type` fields. Keep or replace the existing `chunk`-based checks only if they directly exercise `async_response_stream`’s exclude-and-reattach path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/unit/test_agent_stream_delta.py`:
- Around line 18-24: The test in
test_model_dump_without_exclusion_raises_serialization_warning only checks that
some warning was emitted, so tighten it to assert the warning is the expected
serialization warning from chunk.model_dump. Inspect the warnings captured in
recorded and verify the specific warning category/message associated with
PydanticSerializationUnexpectedValue rather than relying on assert recorded, so
the test fails if an unrelated warning appears or the serializer warning stops
firing.
- Around line 13-34: The current tests only assert Pydantic’s standalone
`model_dump` behavior and do not cover the actual logic changed in
`async_response_stream`. Update the tests to drive a real chunk through
`async_response_stream` in `src/agent.py` using a minimal async generator stub
that yields a `FakeTextDeltaEvent`-like event, then assert the emitted SSE
payload includes the expected `delta` and `type` fields. Keep or replace the
existing `chunk`-based checks only if they directly exercise
`async_response_stream`’s exclude-and-reattach path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d04191fa-b886-490e-a4d5-011a4947f40e
📒 Files selected for processing (1)
tests/unit/test_agent_stream_delta.py
Signed-off-by: vchen7629 <[email protected]>
github-actions
Bot
added
bug
github-actions
Bot
added
bug
2 participants
Summary
Port of PR #1924 to target main branch instead of release-0.5.1. Fixes #1821. Resolves repeated
PydanticSerializationUnexpectedValuewarnings logged for every streamed chat chunk. Langflow's/responseSSE always wraps text deltas as{"content": "..."}for every provider, but the openai SDK's response event models declaredelta: str. Whenchunk.model_dump()runs, Pydantic's serializer checksdeltaagainst thatstrtype, finds a dict, and logs a warning for every chunk which pollutes the openrag-backend logsChanges
deltafrommodel_dump()'s serialization (which is what triggers the type-check warning), then reattach the raw, unvalidateddeltavalue back onto the resulting dict. Output shape sent to the frontend/SDK is unchanged and only the noisy serializer warning is avoided.Summary by CodeRabbit
Bug Fixes
Tests