[TRTLLM-13015][feat] drop complex visual_gen CLI example scripts#14632
Conversation
zhenhuaw-me
changed the title
|
/bot run |
📝 WalkthroughWalkthroughThis PR consolidates VisualGen example scripts by migrating from individual CLI-based example scripts (e.g., ChangesVisualGen CLI to YAML Configuration Migration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/integration/defs/examples/test_visual_gen.py (1)
1197-1239: QA list update appears unnecessary for this PR.This refactor changes invocation/config wiring of an existing integration test path, but does not add a new integration test definition requiring scheduled QA-list enrollment.
As per coding guidelines "If the PR only touches unittest/ or narrow unit scope, say explicitly whether QA list updates are unnecessary or optional." and "If the change adds or materially alters an integration test under tests/integration/defs/, call out whether an entry is needed 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/integration/defs/examples/test_visual_gen.py` around lines 1197 - 1239, This change only refactors invocation/config wiring of an existing integration test and does not add or materially alter an integration test that would require QA-list enrollment; update the PR description (or add a one-line comment near the test_wan_t2v_example definition in tests/integration/defs/examples/test_visual_gen.py) stating explicitly that a QA list update is unnecessary for this PR per the coding guidelines so reviewers know you considered QA-list impact.
🤖 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 `@docs/source/models/visual-generation.md`:
- Around line 122-123: Update the docs for parallel_config to use the correct
Attention2D key and combinability: replace mentions of attn2d_row_size and
attn2d_col_size with the single attn2d_size: [row, col] form, and change the
combinability note to state that Attention2D can be composed with Ulysses (it is
only mutually exclusive with ring_size > 1 / Ring Attention). Update the bullets
referencing Attention2D and Ring Attention accordingly so they reflect the
current API symbols (Attention2D, attn2d_size, ring_size) and constraints.
---
Nitpick comments:
In `@tests/integration/defs/examples/test_visual_gen.py`:
- Around line 1197-1239: This change only refactors invocation/config wiring of
an existing integration test and does not add or materially alter an integration
test that would require QA-list enrollment; update the PR description (or add a
one-line comment near the test_wan_t2v_example definition in
tests/integration/defs/examples/test_visual_gen.py) stating explicitly that a QA
list update is unnecessary for this PR per the coding guidelines so reviewers
know you considered QA-list impact.
🪄 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: bff1e367-6ccd-4cb7-8dcf-37a983c3859c
📒 Files selected for processing (9)
docs/source/models/visual-generation.mdexamples/visual_gen/README.mdexamples/visual_gen/models/wan_t2v.pyexamples/visual_gen/visual_gen_flux.pyexamples/visual_gen/visual_gen_ltx2.pyexamples/visual_gen/visual_gen_mgmn_distributed.shexamples/visual_gen/visual_gen_wan_i2v.pyexamples/visual_gen/visual_gen_wan_t2v.pytests/integration/defs/examples/test_visual_gen.py
💤 Files with no reviewable changes (5)
- examples/visual_gen/visual_gen_mgmn_distributed.sh
- examples/visual_gen/visual_gen_ltx2.py
- examples/visual_gen/visual_gen_wan_i2v.py
- examples/visual_gen/visual_gen_wan_t2v.py
- examples/visual_gen/visual_gen_flux.py
|
PR_Github #50545 [ run ] triggered by Bot. Commit: |
|
PR_Github #50545 [ run ] completed with state
|
|
/bot run |
|
PR_Github #50642 [ run ] triggered by Bot. Commit: |
|
PR_Github #50642 [ run ] completed with state |
|
/bot run --stage-list "DGX_B200-4_GPUs-PyTorch-Post-Merge-1,DGX_B200-4_GPUs-PyTorch-Post-Merge-2" |
|
PR_Github #50747 [ run ] triggered by Bot. Commit: |
|
PR_Github #50747 [ run ] completed with state |
|
/bot reuse --comment "pre-merge and the additional stages passing" |
GitHub Bot Help
Provide a user friendly way for developers to interact with a Jenkins server. Run See details below for each supported subcommand. Details
Launch build/test pipelines. All previously running jobs will be killed.
kill
Kill all running builds associated with pull request. skip
Skip testing for latest commit on pull request. reuse-pipeline
Reuse a previous pipeline to validate current commit. This action will also kill all currently running builds associated with the pull request. IMPORTANT NOTE: This is dangerous since lack of user care and validation can cause top of tree to break. |
zhenhuaw-me
force-pushed
the
drop-cli-examples
branch
from
c3823c9 to
e057908
Compare
There was a problem hiding this comment.
Thanks for cleanup.
One heads-up: after this, models/ only has the T2V example, so I2V / FLUX / LTX-2 all offline examples are gone even though the supported-models table still lists them. API coverage is preserved via quickstart_example.py + serve/ + the Python-API tests, and I see the plan is to add them in follow-ups -- just flagging this for visibility. Tiny nit: the README says "Per-model example scripts" (plural) but there's one for now.
Left one inline note on the Attention2D config key.
|
/bot reuse --comment "previous CI passing and the new commits are doc only change" |
GitHub Bot Help
Provide a user friendly way for developers to interact with a Jenkins server. Run See details below for each supported subcommand. Details
Launch build/test pipelines. All previously running jobs will be killed.
kill
Kill all running builds associated with pull request. skip
Skip testing for latest commit on pull request. reuse-pipeline
Reuse a previous pipeline to validate current commit. This action will also kill all currently running builds associated with the pull request. IMPORTANT NOTE: This is dangerous since lack of user care and validation can cause top of tree to break. |
@chang-l These CLI examples never provide API coverage... One of the motivations to drop these examples is to drive the model PICs to add corresponding new sets of examples and protect them with CIs. Ideally, model and feature PICs may add simple model and feature examples and protect with CI. #14685 is another PR to drive the team to that direction. |
Remove the per-model CLI wrappers under examples/visual_gen/ (flux, ltx2, wan_t2v, wan_i2v, plus the multi-node sbatch shell) — these had grown into hundreds of lines of argparse and dispatch logic better suited to local development than maintained in-tree examples. Coverage is preserved via the slim models/wan_t2v.py example and the trtllm-serve flows under serve/. - models/wan_t2v.py: hardcode the integration-test resolution and prompt so it doubles as the smoke fixture for tests below. - tests: port _generate_wan_video to invoke models/wan_t2v.py with an on-the-fly VisualGenArgs YAML written into the output dir (cfg_size picked from torch.cuda.device_count()). Drop stale docstrings. - README + visual-generation.md: rewrite as if the CLI wrappers never existed; switch all CLI-flag references in the optimization sections to the equivalent VisualGenArgs YAML keys. Signed-off-by: Zhenhua Wang <zhenhuaw@nvidia.com>
zhenhuaw-me
force-pushed
the
drop-cli-examples
branch
from
7b785f2 to
11acfd6
Compare
|
/bot run --extra-stage "DGX_B200-4_GPUs-PyTorch-Post-Merge-1, DGX_B200-4_GPUs-PyTorch-Post-Merge-2" |
|
/bot kill |
|
PR_Github #51277 [ run ] triggered by Bot. Commit: |
|
PR_Github #51278 [ kill ] triggered by Bot. Commit: |
|
PR_Github #51277 [ run ] completed with state |
|
PR_Github #51278 [ kill ] completed with state |
|
/bot reuse --comment "Rebased onto current main only changed docs (visual-generation.md, examples/visual_gen/README.md). examples/visual_gen/models/wan_t2v.py and tests/integration/defs/examples/test_visual_gen.py are byte-identical to commit c3823c9 from the previously successful pipeline #50747." |
GitHub Bot Help
Provide a user friendly way for developers to interact with a Jenkins server. Run See details below for each supported subcommand. Details
Launch build/test pipelines. All previously running jobs will be killed.
kill
Kill all running builds associated with pull request. skip
Skip testing for latest commit on pull request. reuse-pipeline
Reuse a previous pipeline to validate current commit. This action will also kill all currently running builds associated with the pull request. IMPORTANT NOTE: This is dangerous since lack of user care and validation can cause top of tree to break. |
|
/bot reuse-pipeline --comment "see details on last comment" |
|
PR_Github #51289 [ reuse-pipeline ] triggered by Bot. Commit: |
|
PR_Github #51289 [ reuse-pipeline ] completed with state |
7 participants
Description
The per-model CLI wrappers under
examples/visual_gen/(visual_gen_flux.py,visual_gen_ltx2.py,visual_gen_wan_t2v.py,visual_gen_wan_i2v.py, and the multi-node sbatch shell) had grown into hundreds of lines of argparse and dispatch logic that's better kept as local dev scripts than maintained in-tree examples. This PR removes them. Coverage of the VisualGen API is preserved by the slimmodels/wan_t2v.pyexample and thetrtllm-serveflows underserve/.Knock-on changes:
examples/visual_gen/models/wan_t2v.py: hardcode the integration-test resolution and prompt so it doubles as the smoke fixture fortest_vbench_dimension_score_wan*.tests/integration/defs/examples/test_visual_gen.py: port_generate_wan_videoto invokemodels/wan_t2v.pywith an on-the-flyVisualGenArgsYAML (cfg_size picked fromtorch.cuda.device_count()); drop stale docstring references to the removed scripts.examples/visual_gen/README.mdanddocs/source/models/visual-generation.md: rewrite as if the CLI wrappers never existed; switch CLI-flag references in the optimization sections to the equivalentVisualGenArgsYAML keys.Net diff: −2293 lines.
Test Coverage
tests/integration/defs/examples/test_visual_gen.py::test_wan_t2v_exampleexercisesmodels/wan_t2v.pyend-to-end with the checked-inconfigs/wan2.2-t2v-fp4-1gpu.yaml(Wan 2.2 A14B NVFP4). Verified locally on GB200 (PASSED, 679s).test_vbench_dimension_score_wan*tests already cover the ported_generate_wan_videoinvocation pattern in CI.test_*_lpips_against_golden) and LTX-2 VBench tests are untouched — they use the Python API directly.PR Checklist
Please review the following before submitting your PR:
PR description clearly explains what and why. If using CodeRabbit's summary, please make sure it makes sense.
PR Follows TRT-LLM CODING GUIDELINES to the best of your knowledge.
Test cases are provided for new code paths (see test instructions)
If PR introduces API changes, an appropriate PR label is added - either
api-compatibleorapi-breaking. Forapi-breaking, includeBREAKINGin the PR title.Any new dependencies have been scanned for license and vulnerabilities
CODEOWNERS updated if ownership changes
Documentation updated as needed
Update tava architecture diagram if there is a significant design change in PR.
The reviewers assigned automatically/manually are appropriate for the PR.
Please check this after reviewing the above items as appropriate for this PR.
GitHub Bot Help
To see a list of available CI bot commands, please comment
/bot help.Summary by CodeRabbit
Release Notes
Documentation
Examples