[None][fix] Fix Cosmos3 CUDA event crash when text guardrail blocks prompt - #17510
[None][fix] Fix Cosmos3 CUDA event crash when text guardrail blocks prompt#17510ishovkun wants to merge 3 commits into
Conversation
…rompt When the text guardrail blocks a request, the early-exit path called timer.mark_end() and timer.fill(), but mark_denoise_start() and mark_post_start() were never reached (generation had not started). CudaPhaseTimer.fill() calls elapsed_time() on all four event pairs, so any unrecorded event raised: ValueError: Both events must be recorded before calculating elapsed time. The fix is to return PipelineOutput() directly — there is nothing to time when the request is blocked before generation begins. Signed-off-by: Igor Shovkun <ishovkun@nvidia.com>
30ff54f to
9fb3561
Compare
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe Cosmos3 pipeline now returns an empty ChangesCosmos3 guardrail handling
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Exercises the early return on a bare pipeline instance: the guardrail blocks before the text encoder, transformer and VAE run, so only the config-derived attributes forward() reads on the way there are needed — no weights, no GPU memory, and no unsafe prompt (the safety checker is stubbed to report "unsafe" for any input). Guards the regression directly: against the previous code the call raised "Both events must be recorded before calculating elapsed time" instead of returning an empty PipelineOutput. Signed-off-by: Igor Shovkun <ishovkun@nvidia.com>
brnguyen2
left a comment
brnguyen2
left a comment
There was a problem hiding this comment.
Fix is right: fill() calls elapsed_time() on all four events, and only mark_pre_start() runs before the guardrail check, so the early exit was guaranteed to raise. Returning a default PipelineOutput() gives the correct 0.0 timings. This is the only early return in forward() ahead of the later marks, so no other path has the same problem.
One process note: bug fixes normally carry an NVBug in the title tag rather than [None] — worth filing/linking one if this was reported from a real run.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/unittest/_torch/visual_gen/test_cosmos3_pipeline.py (1)
1135-1137: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the complete empty
PipelineOutputcontract.The
PipelineOutputcontract intensorrt_llm/visual_gen/output.pyat Lines 36-100 also definesaudio,frame_rate, andaudio_sample_rate. Add assertions for these fields so the test detects non-empty audio or metadata on a blocked request.Proposed assertions
assert result.video is None assert result.image is None + assert result.audio is None + assert result.frame_rate is None + assert result.audio_sample_rate is None assert (result.pre_denoise, result.denoise, result.post_denoise) == (0.0, 0.0, 0.0)🤖 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/_torch/visual_gen/test_cosmos3_pipeline.py` around lines 1135 - 1137, Extend the blocked-request assertions in the relevant test to cover the complete empty PipelineOutput contract: assert result.audio is None, result.frame_rate is None, and result.audio_sample_rate is None alongside the existing video, image, and denoising checks.
🤖 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.
Inline comments:
In `@tests/unittest/_torch/visual_gen/test_cosmos3_pipeline.py`:
- Line 1117: Remove the pipeline.rank = 0 assignment from the
Cosmos3OmniMoTPipeline test setup so it does not attempt to write the
getter-only BasePipeline.rank property; rely on the existing default rank value
of 0 when distributed state is uninitialized, allowing forward() to execute.
---
Nitpick comments:
In `@tests/unittest/_torch/visual_gen/test_cosmos3_pipeline.py`:
- Around line 1135-1137: Extend the blocked-request assertions in the relevant
test to cover the complete empty PipelineOutput contract: assert result.audio is
None, result.frame_rate is None, and result.audio_sample_rate is None alongside
the existing video, image, and denoising checks.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: cbb29cc9-b42a-4bff-b7f9-53f541623ca3
📒 Files selected for processing (1)
tests/unittest/_torch/visual_gen/test_cosmos3_pipeline.py
…il test ``rank`` is a BasePipeline property that already resolves to 0 without a distributed init, so assigning it raised AttributeError on the bare instance. ``device`` is likewise a property, served by the transformer stub. Signed-off-by: Igor Shovkun <ishovkun@nvidia.com>
|
/bot run |
|
PR_Github #65399 [ run ] triggered by Bot. Commit: |
|
PR_Github #65399 [ run ] completed with state
|
5 participants
Dev Engineer Review
PipelineOutput()directly.0.0.QA Engineer Review
test_blocked_prompt_returns_empty_output.Description
When the Cosmos3 text guardrail blocks a request, the early-exit path called
timer.mark_end()followed bytimer.fill(PipelineOutput()). However,mark_denoise_start()andmark_post_start()are never reached because theguardrail check fires before any GPU generation work begins.
CudaPhaseTimer.fill()calls
elapsed_time()on all four event pairs, so any unrecorded event raised:This surfaced as a worker crash and a generation error visible to the client:
Fix: return
PipelineOutput()directly — there is nothing to time whenthe request is blocked before generation begins. The timing fields on
PipelineOutputdefault to0.0, which is the correct value for a blocked(zero-generation-work) request.
Test Coverage
No existing test covers the guardrail-blocked early-exit path (the pipeline's
multi-process worker architecture makes it awkward to unit-test directly).
The fix is a two-line deletion with no new branching logic, so the risk is low.
PR Checklist