[None][fix] enable DSv4 overlap slot headroom without MTP - #17282
[None][fix] enable DSv4 overlap slot headroom without MTP#17282reasonsolo wants to merge 1 commit into
Conversation
|
/bot run --disable-fail-fast |
|
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 (3)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughDeepSeek-V4 overlap headroom now applies for non-pipeline-parallel execution whenever overlap scheduling is enabled. The model engine and tests no longer use speculative-decoding or MTP configuration for this decision. ChangesDSv4 overlap headroom
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/unittest/_torch/executor/test_seq_slot_sizing.py (1)
50-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd annotations to the modified test function.
test_dsv4_overlap_headroom_gatehas untyped parameters and no return annotation. Add precise parameter annotations and-> None.Proposed change
def test_dsv4_overlap_headroom_gate( - model_type, pp_size, disable_overlap, expected -) : + model_type: str, + pp_size: int, + disable_overlap: bool, + expected: bool, +) -> None:As per coding guidelines, annotate every function.
🤖 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/executor/test_seq_slot_sizing.py` around lines 50 - 52, Update the test_dsv4_overlap_headroom_gate function signature with precise annotations for model_type, pp_size, disable_overlap, and expected, and add a -> None return annotation. Preserve the existing parameterization and test behavior.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.
Inline comments:
In `@tests/unittest/_torch/executor/test_seq_slot_sizing.py`:
- Around line 42-56: Complete the coverage follow-up for
test_dsv4_overlap_headroom_gate by running pytest tests/unittest/ and reporting
the results; preserve the existing parameterized cases and test behavior.
---
Nitpick comments:
In `@tests/unittest/_torch/executor/test_seq_slot_sizing.py`:
- Around line 50-52: Update the test_dsv4_overlap_headroom_gate function
signature with precise annotations for model_type, pp_size, disable_overlap, and
expected, and add a -> None return annotation. Preserve the existing
parameterization and test behavior.
🪄 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: 5a15a08d-d92f-4ad4-b6eb-603d9dd513b8
📒 Files selected for processing (3)
tensorrt_llm/_torch/pyexecutor/_util.pytensorrt_llm/_torch/pyexecutor/model_engine.pytests/unittest/_torch/executor/test_seq_slot_sizing.py
|
PR_Github #63941 [ run ] triggered by Bot. Commit: |
Signed-off-by: Lizhi Zhou <lizhiz@oci-aga-slurm-1-login-01.cm.cluster>
d55d013 to
e2f7e71
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
/bot run --disable-fail-fast |
|
PR_Github #63947 [ run ] triggered by Bot. Commit: |
|
PR_Github #63941 [ run ] completed with state |
|
PR_Github #63947 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #64021 [ run ] triggered by Bot. Commit: |
|
PR_Github #64021 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #64041 [ run ] triggered by Bot. Commit: |
brnguyen2
left a comment
brnguyen2
left a comment
There was a problem hiding this comment.
Approving — the comments below are optional touch-ups, not blockers.
Reasoning holds up: the 2× transient demand comes from the overlap scheduler's slot lifetime, which is orthogonal to draft length, so the MTP condition in #16279 was over-narrow. Callers are consistent — sampler args ([_util.py:2948](https://github.com/NVIDIA/TensorRT-LLM/pull/17282/files#diff-5f0e349b724e8ad501e0ae859c992801e631217a8d52acfa40e24a8fe95bbbcbR2948)) and guided decoder (py_executor_creator.py:787) both size off model_engine.max_num_seq_slots, so widening the gate propagates everywhere it should.
Two things worth noting before merge:
- Test coverage is still gate-boolean only.
test_seq_slot_sizing.pyasserts what the predicate returns, not that a saturated pool + ADP dummy under overlap no longer raisesNoFreeSlotsError. If you have a repro from the hang, an executor-level test (even with a stub engine, liketest_py_executor.py:1323does) would pin the actual invariant. - The PR body describes the failure well; please put that text in the
## Descriptionsection of the template rather than leaving it above, and fill in## Test Coveragewith what you ran on real DSv4 (draft_len 0, overlap on).
| model_type: Optional[str], mapping: Mapping, | ||
| disable_overlap_scheduler: bool) -> bool: | ||
| """Gate extra sequence slots to the non-PP DSv4 overlap path.""" | ||
| return (should_enable_dsv4_adp_dummy_fixes(model_type, mapping) |
There was a problem hiding this comment.
The mechanism you describe in the PR body — terminal requests holding slots while the V2 scheduler backfills their seats — is model-agnostic. Every non-PP model with overlap scheduling has the same latent 2× demand; DSv4 just happens to be the one where ADP dummy requests saturate the pool often enough to hit it. Keeping the gate model-scoped is a defensible risk call for a fix PR, but it means the same hang is still reachable elsewhere. Is there a follow-up ticket to validate and generalize? Worth naming it in the docstring so this doesn't sit as a permanent special case.
| # decoding. PP remains on its established path. | ||
| self._enable_dsv4_adp_dummy_fixes = (should_enable_dsv4_adp_dummy_fixes( | ||
| model_type, mapping)) | ||
| self._enable_dsv4_overlap_headroom = ( |
There was a problem hiding this comment.
This now doubles the slot pool for every non-PP DSv4 run, including plain no-spec-decode serving, which doubles the sampler's per-slot state (TorchSampler.Args.max_num_sequences) and the guided-decoder allocation. Previously only MTP configs paid that. Did you measure the host/device memory delta on a large max_batch_size config? If it's non-trivial, worth a line in the PR description so it isn't a surprise in perf triage.
|
PR_Github #64041 [ run ] completed with state
|
6 participants
With overlap scheduling, requests completed in the previous iteration still hold their sequence slots when the next iteration calls prepare_resources(). Meanwhile, the scheduler has already removed those terminal requests from its capacity budget and may schedule replacement requests or ADP dummy requests.
This creates a transient requirement of up to 2 * max_batch_size sequence slots.
DeepSeek-V4 already had this overlap headroom, but it was incorrectly restricted to one-model MTP configurations. With draft length 0, the slot pool remained at max_batch_size, allowing dummy allocation to fail when the pool was saturated.
Once some rank event loops exited with NoFreeSlotsError, the remaining ranks blocked in distributed collectives, presenting externally as a server hang.
Dev Engineer Review
2 * max_batch_sizesequence-slot headroom and preventsNoFreeSlotsErrorduring overlap scheduling.QA Engineer Review
tests/unittest/_torch/executor/test_seq_slot_sizing.py.test-db/orqa/entry was changed.Description
Test Coverage
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.