Repository navigation
[None][chore] remove orphaned process_streaming_events helper - #16262
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review. WalkthroughThe public ChangesResponses streaming utility
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Other Suggested reviewers: Merge Risk: ⚪ Minimal · up to The streaming endpoint does not depend on the removed helper; no actionable merge risk remains beyond normal checks. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/unittest/llmapi/apps/test_responses_empty_stream.py (2)
26-49: 📐 Maintainability & Code Quality | 🔵 TrivialNo QA list update is needed.
This change only adds a narrow unit test under
tests/unittest/; no integration or release schedule is affected.As per path instructions, unittest-only changes do not require an entry under
tests/integration/test_lists/qa/.🤖 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/unittest/llmapi/apps/test_responses_empty_stream.py` around lines 26 - 49, The comment confirms that no implementation change or QA list update is required. Keep the narrow unit test test_process_streaming_events_handles_empty_stream under tests/unittest/, and do not add an entry under tests/integration/test_lists/qa/.Source: Path instructions
21-28: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAnnotate the async test helpers.
Add return annotations to
_empty_streamandrunto satisfy the Python typing guidelines.Proposed fix
+from collections.abc import AsyncGenerator + -async def _empty_stream(): +async def _empty_stream() -> AsyncGenerator[None, None]: return yield # pragma: no cover - makes this an async generator ... - async def run(): + async def run() -> None:As per coding guidelines, “Always annotate functions.”
🤖 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/unittest/llmapi/apps/test_responses_empty_stream.py` around lines 21 - 28, Add return type annotations to the async helper functions _empty_stream and the nested run inside test_process_streaming_events_handles_empty_stream, using the appropriate async-generator and coroutine return types consistent with their implementations.Source: Coding guidelines
🤖 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/unittest/llmapi/apps/test_responses_empty_stream.py`:
- Around line 26-49: The comment confirms that no implementation change or QA
list update is required. Keep the narrow unit test
test_process_streaming_events_handles_empty_stream under tests/unittest/, and do
not add an entry under tests/integration/test_lists/qa/.
- Around line 21-28: Add return type annotations to the async helper functions
_empty_stream and the nested run inside
test_process_streaming_events_handles_empty_stream, using the appropriate
async-generator and coroutine return types consistent with their
implementations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 643f0d76-2cb7-487f-b0bd-990420148a9d
📒 Files selected for processing (2)
tensorrt_llm/serve/responses_utils.pytests/unittest/llmapi/apps/test_responses_empty_stream.py
|
/bot run |
|
PR_Github #59279 [ run ] triggered by Bot. Commit: |
|
PR_Github #59279 [ run ] completed with state
|
karljang
left a comment
karljang
left a comment
There was a problem hiding this comment.
Thanks for the fix. Could you confirm a live caller uses this helper and whether an empty stream needs a terminal event? Please also register the regression test in CI, adding cpu_only if targeting a CPU-only stage.
process_streaming_events has had no callers since the Responses API streaming path moved to post-processors in NVIDIA#9908: the live path drives ResponsesStreamingProcessor directly (openai_server.py) and builds the final event in responses_api_streaming_post_processor from the current GenerationResult when rsp._done, so the helper's latent UnboundLocalError on an empty stream (final_res referenced without assignment) cannot occur there. Delete the dead function instead of patching it. Signed-off-by: lonexreb <reach2shubhankar@gmail.com>
a980a70 to
0a3f92a
Compare
lonexreb
changed the title
|
@karljang thanks for the pointed questions — they changed the outcome. Answers in order:
If you'd rather keep the helper for some planned use, happy to revert to the guard-fix revision + register the test with |
svc-trtllm-gh-bot
added
the
Community want to contribute
|
Automatically added "ci: full pre-merge approved" because this PR has satisfied the required GitHub review approvals. Unresolved review conversations and other required checks remain independent merge requirements. |
|
/bot run |
|
PR_Github #76492 [ run ] triggered by Bot. Commit: |
|
PR_Github #76492 [ run ] completed with state
|
|
/bot run |
|
PR_Github #77056 [ run ] triggered by Bot. Commit: |
|
PR_Github #77056 [ run ] completed with state |
… paths - A chat template is now asked whether it renders a developer message by rendering one, rather than by finding the word "developer" in its source. A template that names the role only to reject it, or mentions it in a comment, no longer keeps the role and loses the message. When a template renders neither a developer nor a system message the roles are left alone. Results are cached per template. - At the end of a stream, the text the whole-text parse reads beyond the streamed message is released even when the parse stripped leading whitespace the stream already sent; a real divergence is logged instead of dropped silently. - An encrypted_content part is read as text only when it is not one long base64 token, so a real ciphertext (OpenAI's Fernet tokens) is not put into the prompt. - The unreachable storing stream finalizer (get_final_response and process_streaming_events, which main removed in NVIDIA#16262) is deleted, so it cannot be wired back in its stale form. Streamed responses are not stored. - Generation-first scheduling of a Responses request is tested, streaming and not. - The router's character-versus-token weighting is explained where it is computed. Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
- responses_utils.py: main removed process_streaming_events (NVIDIA#16262), which this branch had already deleted together with get_final_response; this branch's side is kept, and main's dict-shaped function_call input handling merged cleanly. - release-notes.md: both new API-change entries are kept. Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
Description
Originally this PR guarded
process_streaming_eventsagainst an empty stream (final_resreferenced without assignment). Review asked whether a live caller exists — and the answer changed the right fix.There is no live caller.
git grep process_streaming_eventsacross current main returns only the definition (responses_utils.py:2618). The Responses API streaming path moved to post-processors in #9908:openai_server.pyconstructsResponsesStreamingProcessorper request andresponses_api_streaming_post_processor(postprocess_handlers.py:1014) emits per-chunk events and builds the final event from the currentGenerationResultwhenrsp._done— so the live path has no equivalent unbound-variable hazard, and an aborted stream simply ends the generator at the server layer.This PR therefore now deletes the dead function instead of patching it: it is unreachable, carries a latent crash, and patching unreachable code just preserves confusion. The regression test from the earlier revision is dropped along with it (nothing left to test), which also resolves the CI-registration request.
Test Plan
git grep process_streaming_events→ no references remain.ruff(legacy config) andyapfclean on the touched file.Dev Engineer Review
process_streaming_eventswas removed fromtensorrt_llm/serve/responses_utils.py. The remainingResponsesStreamingProcessoris still used by the serving code. The change removes a callable function, so downstream users that import it may need to update their code. No in-repository callers of the removed function were found.QA Engineer Review
No test changes.
Per-File QA Perspective
tensorrt_llm/serve/responses_utils.py: Verify that Responses streaming still works through theResponsesStreamingProcessorpath. The removed function is no longer available to callers.