fix(streaming): avoid duplicate usage metadata chunk#2079
Conversation
Greptile SummaryThis PR fixes a duplicate-usage-metadata bug in
|
| Filename | Overview |
|---|---|
| nemoguardrails/streaming.py | Adds two lines after enqueuing a non-final metadata chunk to pop usage from current_metadata, preventing it from being re-emitted verbatim in the final END_OF_STREAM metadata chunk. |
| tests/test_streaming_handler.py | New test test_raw_usage_metadata_chunk_is_not_repeated_on_final_chunk verifies the fix. Assertions correctly check the final chunk via chunks[-1], but the usage_chunks filter relies on truthiness rather than an explicit is not None check. |
Sequence Diagram
%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant P as Provider
participant SH as StreamingHandler
participant Q as Queue
participant C as Consumer
P->>SH: push_chunk("Hello")
SH->>SH: "_process("Hello") — current_metadata={}"
SH->>Q: "enqueue {text:"Hello"} (no metadata)"
P->>SH: "push_chunk("", metadata={"usage": {...}})"
SH->>SH: "current_metadata.update({"usage": {...}})"
SH->>SH: "_process("") — current_metadata={"usage":{...}}"
SH->>Q: "enqueue {text:"", metadata:{"usage":{...}}}"
SH->>SH: current_metadata.pop("usage") ← FIX
P->>SH: push_chunk(None) → END_OF_STREAM
SH->>SH: "_process(END_OF_STREAM) — current_metadata={}"
SH->>Q: "enqueue {text:"", metadata:{response_metadata:None, usage_metadata:None}}"
Q-->>C: "{text:"Hello"}"
Q-->>C: "{text:"", metadata:{"usage":{...}}}"
Q-->>C: "{text:"", metadata:{response_metadata:None, usage_metadata:None}}"
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant P as Provider
participant SH as StreamingHandler
participant Q as Queue
participant C as Consumer
P->>SH: push_chunk("Hello")
SH->>SH: "_process("Hello") — current_metadata={}"
SH->>Q: "enqueue {text:"Hello"} (no metadata)"
P->>SH: "push_chunk("", metadata={"usage": {...}})"
SH->>SH: "current_metadata.update({"usage": {...}})"
SH->>SH: "_process("") — current_metadata={"usage":{...}}"
SH->>Q: "enqueue {text:"", metadata:{"usage":{...}}}"
SH->>SH: current_metadata.pop("usage") ← FIX
P->>SH: push_chunk(None) → END_OF_STREAM
SH->>SH: "_process(END_OF_STREAM) — current_metadata={}"
SH->>Q: "enqueue {text:"", metadata:{response_metadata:None, usage_metadata:None}}"
Q-->>C: "{text:"Hello"}"
Q-->>C: "{text:"", metadata:{"usage":{...}}}"
Q-->>C: "{text:"", metadata:{response_metadata:None, usage_metadata:None}}"
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
tests/test_streaming_handler.py:747
The filter uses a truthiness check on the `usage` dict. If all token counts were zero, the dict `{"input_tokens": 0, "output_tokens": 0, "total_tokens": 0}` would still be truthy (non-empty dict), so the current test data passes — but relying on truthiness here is subtly misleading. An explicit `is not None` guard is more accurate and intention-revealing.
```suggestion
usage_chunks = [chunk for chunk in chunks if chunk.get("metadata", {}).get("usage") is not None]
```
Reviews (1): Last reviewed commit: "fix(streaming): avoid duplicate usage me..." | Re-trigger Greptile
| await streaming_handler.push_chunk(None) | ||
|
|
||
| chunks = await streaming_consumer.get_chunks() | ||
| usage_chunks = [chunk for chunk in chunks if chunk.get("metadata", {}).get("usage")] |
There was a problem hiding this comment.
The filter uses a truthiness check on the
usage dict. If all token counts were zero, the dict {"input_tokens": 0, "output_tokens": 0, "total_tokens": 0} would still be truthy (non-empty dict), so the current test data passes — but relying on truthiness here is subtly misleading. An explicit is not None guard is more accurate and intention-revealing.
| usage_chunks = [chunk for chunk in chunks if chunk.get("metadata", {}).get("usage")] | |
| usage_chunks = [chunk for chunk in chunks if chunk.get("metadata", {}).get("usage") is not None] |
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/test_streaming_handler.py
Line: 747
Comment:
The filter uses a truthiness check on the `usage` dict. If all token counts were zero, the dict `{"input_tokens": 0, "output_tokens": 0, "total_tokens": 0}` would still be truthy (non-empty dict), so the current test data passes — but relying on truthiness here is subtly misleading. An explicit `is not None` guard is more accurate and intention-revealing.
```suggestion
usage_chunks = [chunk for chunk in chunks if chunk.get("metadata", {}).get("usage") is not None]
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe stream handler now removes raw ChangesUsage metadata reset
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Description
avoid duplicate usage metadata chunk. addressing #1977 (comment)
Related Issue(s)
#1977 (comment)
Verification
AI Assistance
Checklist
Summary by CodeRabbit
Bug Fixes
Tests