[https://nvbugs/6437341][fix] Port sibling fix 38db241414 verbatim (shutil.which short-circuit +… - #16306
[https://nvbugs/6437341][fix] Port sibling fix 38db241414 verbatim (shutil.which short-circuit +…#16306trtllm-agent wants to merge 2 commits into
Conversation
…LPIPS golden test_cosmos3_nano_t2v_lpips_against_golden fails with LPIPS 0.449779 (9x the 0.05 threshold). Same test / same error signature as previously verified sibling fixes on bugs 6422299, 6435113, and 6435119 (the last of which resolved this exact 0.449779 -> 0.037186 on B200 in commit 824c85a). Commit f50ca53 (Cosmos3 Audio Output Support) silently changed three CFG-affecting behaviors that the T2V LPIPS golden was calibrated against: 1. Cosmos3CrossAttention.forward per-batch-slices k_und/v_und by real_text_lens when batch_size > 1. Under CFG (batch=2) the negative-prompt entry now attends over a shorter slice than the padded max_real_len used pre-audio. 2. COSMOS3_DEFAULT_NEGATIVE_PROMPT changed from a long descriptive video-quality string to "". The T2V LPIPS test does not override negative_prompt, so it consumes this default directly. 3. COSMOS3_720P_PARAMS["max_sequence_length"] bumped 1024 -> 4096. All three reverts are required to bring LPIPS below threshold. The per-batch slicing path was preparatory for future audio work and is not exercised by any existing audio test (audio tests use batch_size=1). Fix (mirrors the verified sibling bug-6422299 / -6435113 / -6435119 repairs): - transformer_cosmos3.py: drop the real_text_lens parameter from Cosmos3CrossAttention.forward and Cosmos3GenDecoderLayer.forward, and stop computing/passing it in Cosmos3VFMTransformer.forward. All batch entries revert to sharing the same k_und[:, :max_real_len] slice as before. - pipeline_cosmos3.py: restore the long descriptive COSMOS3_DEFAULT_NEGATIVE_PROMPT string. - defaults.py: restore COSMOS3_720P_PARAMS["max_sequence_length"] to 1024. - waives.txt: remove the nvbugs/6437341 SKIP entry for this test. Signed-off-by: trtllm-agent <296075020+trtllm-agent@users.noreply.github.com>
…iners The DGX_B200 PyTorch CI container runs pytest under LOCAL_USER=1 (non-root), so _visual_gen_deps' unconditional apt-get update/install exits 100 with "Permission denied on /var/lib/apt/lists/partial" and the fixture aborts before test_cosmos3_nano_t2v_lpips_against_golden can reach the LPIPS assertion. Separately, NVIDIA#16206 removed pre-installed opencv from the container image, so scripts/visualgen_eval/visual_gen_lpips_score_eval.py now fails at 'import cv2' on the video decode path. Refactor the fixture to (1) short-circuit when ffmpeg is already on PATH, (2) fall back to imageio-ffmpeg (bundles a static libx264-enabled ffmpeg 7.x binary requiring no root) when apt-get exits non-zero, and (3) always pip-install opencv-python-headless so the LPIPS eval script's lazy cv2 import succeeds on the video path. The strict no-codec-fallback guard in _save_lpips_video_mp4 (added by 844cc89 to stop silent mpeg4 fallback from masking regressions) stays intact; we install a real x264-capable ffmpeg rather than loosening the guard. Combined with c9b32b8's pre-audio CFG revert, the T2V pipeline now runs to completion and scores 0.037186 LPIPS against the golden (threshold 0.05). Signed-off-by: trtllm-agent <trtllm-agent@nvidia.com> Signed-off-by: trtllm-agent <296075020+trtllm-agent@users.noreply.github.com>
📝 WalkthroughWalkthroughCosmos3 defaults now use a shorter 720p sequence length and a populated negative prompt. Cross-attention no longer handles per-sample text lengths. Visual-generation integration setup adds portable OpenCV and ffmpeg provisioning, and the waived LPIPS test entry changes. ChangesCosmos3 generation and integration updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Cosmos3VFMTransformer
participant Cosmos3GenDecoderLayer
participant Cosmos3CrossAttention
Cosmos3VFMTransformer->>Cosmos3GenDecoderLayer: invoke without real_text_lens
Cosmos3GenDecoderLayer->>Cosmos3CrossAttention: pass hidden states and und/GEN key-values
Cosmos3CrossAttention->>Cosmos3CrossAttention: concatenate tensors
Cosmos3CrossAttention->>Cosmos3CrossAttention: execute full cross-attention
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/integration/defs/examples/visual_gen/test_visual_gen.py (1)
245-269: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a small unit test for the ffmpeg fallback The fallback branch has no direct coverage; mock
check_callto raiseCalledProcessErrorand assert theimageio-ffmpegpath creates the~/.local/bin/ffmpegsymlink.🤖 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/integration/defs/examples/visual_gen/test_visual_gen.py` around lines 245 - 269, Add a focused unit test for _ensure_ffmpeg_available that mocks shutil.which as unavailable and check_call to raise CalledProcessError, then verifies llm_venv.run_cmd installs imageio-ffmpeg and the fallback creates the expected ~/.local/bin/ffmpeg symlink using imageio_ffmpeg.get_ffmpeg_exe().Source: Path instructions
🤖 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/integration/defs/examples/visual_gen/test_visual_gen.py`:
- Around line 255-259: Broaden the exception handling around the apt-get calls
in the visual generation fixture to also catch FileNotFoundError, while
preserving the existing CalledProcessError fallback. Ensure either apt-get
failure routes to llm_venv.run_cmd(["-m", "pip", "install", "imageio-ffmpeg"]).
---
Nitpick comments:
In `@tests/integration/defs/examples/visual_gen/test_visual_gen.py`:
- Around line 245-269: Add a focused unit test for _ensure_ffmpeg_available that
mocks shutil.which as unavailable and check_call to raise CalledProcessError,
then verifies llm_venv.run_cmd installs imageio-ffmpeg and the fallback creates
the expected ~/.local/bin/ffmpeg symlink using imageio_ffmpeg.get_ffmpeg_exe().
🪄 Autofix (Beta)
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: f8781a73-26bb-43e4-9a9f-728c992f6f79
📒 Files selected for processing (5)
tensorrt_llm/_torch/visual_gen/models/cosmos3/defaults.pytensorrt_llm/_torch/visual_gen/models/cosmos3/pipeline_cosmos3.pytensorrt_llm/_torch/visual_gen/models/cosmos3/transformer_cosmos3.pytests/integration/defs/examples/visual_gen/test_visual_gen.pytests/integration/test_lists/waives.txt
💤 Files with no reviewable changes (1)
- tests/integration/test_lists/waives.txt
| try: | ||
| check_call(["apt-get", "update", "-y"], shell=False) | ||
| check_call(["apt-get", "install", "-y", "ffmpeg"], shell=False) | ||
| except subprocess.CalledProcessError: | ||
| llm_venv.run_cmd(["-m", "pip", "install", "imageio-ffmpeg"]) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Broaden the fallback except clause to cover missing apt-get binary.
check_call(["apt-get", ...]) raises FileNotFoundError (not CalledProcessError) if the apt-get binary doesn't exist at all (e.g. non-Debian-based or apt-less containers). That currently isn't caught, so the fixture would crash instead of falling back to imageio-ffmpeg — undermining the portability goal of this fix.
🔧 Proposed fix to also handle a missing apt-get binary
try:
check_call(["apt-get", "update", "-y"], shell=False)
check_call(["apt-get", "install", "-y", "ffmpeg"], shell=False)
- except subprocess.CalledProcessError:
+ except (subprocess.CalledProcessError, OSError):
llm_venv.run_cmd(["-m", "pip", "install", "imageio-ffmpeg"])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try: | |
| check_call(["apt-get", "update", "-y"], shell=False) | |
| check_call(["apt-get", "install", "-y", "ffmpeg"], shell=False) | |
| except subprocess.CalledProcessError: | |
| llm_venv.run_cmd(["-m", "pip", "install", "imageio-ffmpeg"]) | |
| try: | |
| check_call(["apt-get", "update", "-y"], shell=False) | |
| check_call(["apt-get", "install", "-y", "ffmpeg"], shell=False) | |
| except (subprocess.CalledProcessError, OSError): | |
| llm_venv.run_cmd(["-m", "pip", "install", "imageio-ffmpeg"]) |
🤖 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/integration/defs/examples/visual_gen/test_visual_gen.py` around lines
255 - 259, Broaden the exception handling around the apt-get calls in the visual
generation fixture to also catch FileNotFoundError, while preserving the
existing CalledProcessError fallback. Ensure either apt-get failure routes to
llm_venv.run_cmd(["-m", "pip", "install", "imageio-ffmpeg"]).
Summary
_visual_gen_depsfixture unconditionally runsapt-get update/installas non-root (LOCAL_USER=1) → exit 100 → test errors at setup before LPIPS runs; separately, opencv was removed from the container image by [None][chore] Remove pre-installed opencv #16206 so the LPIPS eval script's cv2 import also fails on the video path.pip install opencv-python-headlessto the fixture so the LPIPS eval script's lazy_get_cv2()succeeds on the T2V path.Test plan
Links
Summary by CodeRabbit
New Features
Bug Fixes