launcher: bump TRT-LLM to 1.3.0rc20, pin vLLM to v0.22.0, fix max_seq…#1982
Conversation
|
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 (4)
📝 WalkthroughWalkthroughLauncher defaults and example Slurm workflows now use updated TensorRT-LLM and vLLM container images. Qwen3.5-4B throughput benchmarks explicitly set a maximum sequence length of ChangesLauncher configuration
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Comment |
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 1
🤖 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 `@tools/launcher/examples/Qwen/Qwen3-8B/hf_streaming_dflash.yaml`:
- Line 93: Pin the vLLM smoke-test container from nightly to v0.22.0 in both
tools/launcher/examples/Qwen/Qwen3-8B/hf_streaming_dflash.yaml:93-93 and
tools/launcher/examples/Qwen/Qwen3-8B/hf_streaming_dflash_multi_node.yaml:96-96
by updating each container reference to the v0.22.0 image, using the multi-arch
variant if that is the established image elsewhere in the PR.
🪄 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: 8a333021-0ee0-4c0b-bc03-754659162969
📒 Files selected for processing (21)
tools/launcher/examples/Qwen/Qwen3-30B-A3B/hf_streaming_dflash_multi_node.yamltools/launcher/examples/Qwen/Qwen3-30B-A3B/hf_streaming_eagle3_multi_node.yamltools/launcher/examples/Qwen/Qwen3-8B/hf_eagle3_dryrun.yamltools/launcher/examples/Qwen/Qwen3-8B/hf_online_eagle3.yamltools/launcher/examples/Qwen/Qwen3-8B/hf_streaming_dflash.yamltools/launcher/examples/Qwen/Qwen3-8B/hf_streaming_dflash_multi_node.yamltools/launcher/examples/Qwen/Qwen3-8B/hf_streaming_eagle3.yamltools/launcher/examples/Qwen/Qwen3-8B/hf_streaming_eagle3_multi_node.yamltools/launcher/examples/Qwen/Qwen3.5-4B/specdec_bench.yamltools/launcher/examples/Qwen/Qwen3.5-4B/specdec_bench_mtp_vllm.yamltools/launcher/examples/moonshotai/Kimi-K2.5/hf_dflash_dryrun.yamltools/launcher/examples/moonshotai/Kimi-K2.5/hf_eagle3_dryrun.yamltools/launcher/examples/moonshotai/Kimi-K2.5/hf_offline_dflash.yamltools/launcher/examples/moonshotai/Kimi-K2.5/hf_streaming_dflash.yamltools/launcher/examples/moonshotai/Kimi-K2.5/hf_streaming_dflash_multi_node.yamltools/launcher/examples/moonshotai/Kimi-K2.5/hf_streaming_eagle3.yamltools/launcher/examples/moonshotai/Kimi-K2.5/hf_streaming_eagle3_multi_node.yamltools/launcher/examples/moonshotai/Kimi-K2.5/specdec_bench.yamltools/launcher/examples/openai/gpt-oss-20b/hf_streaming_dflash_multi_node.yamltools/launcher/examples/openai/gpt-oss-20b/hf_streaming_eagle3_multi_node.yamltools/launcher/slurm_config.py
| slurm_config: | ||
| _factory_: "slurm_factory" | ||
| container: "vllm/vllm-openai:nightly" | ||
| container: vllm/vllm-openai:nightly |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
vLLM smoke-test containers still float on nightly, not pinned to v0.22.0.
The PR objective states vLLM is being pinned to v0.22.0, but these two workflows still reference the unpinned vllm/vllm-openai:nightly tag for their vLLM smoke-test task, making runs non-reproducible.
tools/launcher/examples/Qwen/Qwen3-8B/hf_streaming_dflash.yaml#L93-L93: pincontainertovllm/vllm-openai:v0.22.0(or the multi-arch v0.22.0 image used elsewhere in this PR).tools/launcher/examples/Qwen/Qwen3-8B/hf_streaming_dflash_multi_node.yaml#L96-L96: same fix — pin tovllm/vllm-openai:v0.22.0.
📍 Affects 2 files
tools/launcher/examples/Qwen/Qwen3-8B/hf_streaming_dflash.yaml#L93-L93(this comment)tools/launcher/examples/Qwen/Qwen3-8B/hf_streaming_dflash_multi_node.yaml#L96-L96
🤖 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 `@tools/launcher/examples/Qwen/Qwen3-8B/hf_streaming_dflash.yaml` at line 93,
Pin the vLLM smoke-test container from nightly to v0.22.0 in both
tools/launcher/examples/Qwen/Qwen3-8B/hf_streaming_dflash.yaml:93-93 and
tools/launcher/examples/Qwen/Qwen3-8B/hf_streaming_dflash_multi_node.yaml:96-96
by updating each container reference to the v0.22.0 image, using the multi-arch
variant if that is the established image elsewhere in the PR.
There was a problem hiding this comment.
Hi @ChenhanYu @h-guo18, For the vLLM container, can we use the latest tag—i.e., vllm/vllm-openai:latest—so that it always points to the newest vLLM release version? This way, we wouldn’t need to frequently update the YAML file.
9c8b5af to
1748dbf
Compare
…_len for Qwen3.5-4B Signed-off-by: noeyy-mino <174223378+noeyy-mino@users.noreply.github.com>
Signed-off-by: noeyy-mino <174223378+noeyy-mino@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1982 +/- ##
==========================================
+ Coverage 76.59% 76.62% +0.03%
==========================================
Files 525 525
Lines 59253 59253
==========================================
+ Hits 45385 45404 +19
+ Misses 13868 13849 -19
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
/claude review |
There was a problem hiding this comment.
Claude review passed — no blocking issues found. LGTM
Scope: All 25 changed files reviewed (config-only PR: TRT-LLM/vLLM container tag bumps + max_seq_len fixes). No modelopt/ source touched.
Findings: CRITICAL: 0, IMPORTANT: 0, SUGGESTION: 0
Notable verifications:
- The removed
--runtime_params common/specdec_bench/runtime_params_throughput_32k.yamlreference inspecdec_bench_mtp_vllm.yamlpoints to a file that does not exist anywhere in the repo. Sincerun.pyopens--runtime_paramsunconditionally, the prior config would have crashed on load — replacing it with--max_seq_len 65536is a genuine fix, and--max_seq_lencorrectly maps tomax_model_lenfor the VLLM engine both files use. - The
40960 → 65536bump inspecdec_bench.yamlmatches the stated need to accommodate ~46.6K-token outlier prompts. - Kimi-K2.5 vLLM images moved from the aarch64-specific
v0.22.0-aarch64tag to the multi-archv0.22.0, and the accompanying comment was updated consistently.
Risk level: Low. No functional/algorithm/state/export code changed; changes are container references and benchmark CLI args. Container tag validity (rc20 availability) can't be verified from the diff and is left to CI/runtime.
Note: CodeRabbit's observation that Qwen3-8B dflash configs still reference vllm/vllm-openai:nightly is pre-existing (this PR only removed the surrounding quotes there) and not introduced by these changes.
bump TRT-LLM to 1.3.0rc20, pin vLLM to v0.22.0, fix max_seq_len for Qwen3.5-4B
What does this PR do?
Type of change: Bug fix, new feature
Qwen3-8B, Qwen3-30B-A3B, Kimi-K2.5, and gpt-oss-20b launcher configs.
the multi-arch v0.22.0 tag, which resolves to amd64/arm64 automatically.
65536 to accommodate outlier prompts (~46.6K tokens) that caused
VLLMValidationError and aborted the entire benchmark run.
runtime_params_throughput_32k.yaml; use --max_seq_len 65536 instead
Usage
Testing
N/A
CONTRIBUTING.md: N/AAdditional Information
N/A
Summary by CodeRabbit
1.3.0rc20image.0.22.0image (removing prior architecture-specific variants).