[None][fix] fix disagg overlap slot headroom without MTP - #17282
[None][fix] fix disagg overlap slot headroom without MTP#17282reasonsolo wants to merge 5 commits into
Conversation
|
/bot run --disable-fail-fast |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughSequence-slot overlap headroom now applies to disaggregated attention-DP deployments. Eligibility uses topology, cache-transceiver configuration, pipeline parallelism, and overlap scheduling. Unit and integration tests cover the updated behavior. ChangesDisaggregated attention-DP overlap headroom
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant ModelEngine
participant HeadroomUtility
participant SequenceSlotSizing
participant GuidedDecoder
ModelEngine->>HeadroomUtility: evaluate topology, cache-transceiver config, and overlap setting
HeadroomUtility-->>ModelEngine: return headroom eligibility
ModelEngine->>SequenceSlotSizing: calculate maximum sequence slots
SequenceSlotSizing-->>ModelEngine: return slot capacity
ModelEngine->>GuidedDecoder: use expanded slot capacity when enabled
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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: |
fredricz-20070104
left a comment
fredricz-20070104
left a comment
There was a problem hiding this comment.
Review summary - Approve (non-blocking)
Approving so this is not blocked on me. The points raised in my review comment above are non-blocking — please read them and address what you agree with before merging.
Worth doing before this is relied on: Bug fix whose unit test does not reproduce the original NoFreeSlotsError hang; a waiver was removed (test re-enabled) and the test config/backend simultaneously changed; and an arch guard was dropped from an fp8 test. A human should run the disagg DSv4 overlap path (draft_len 0, overlap on) and confirm the test only lands on supported hardware.
Automated review by NVCortex Lite, run by @fredricz-20070104.
|
PR_Github #64440 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #64539 [ run ] triggered by Bot. Commit: |
|
PR_Github #64539 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #64575 [ run ] triggered by Bot. Commit: |
|
PR_Github #64575 [ run ] completed with state
|
Signed-off-by: Lizhi Zhou <1432185+reasonsolo@users.noreply.github.com>
|
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 #64592 [ run ] triggered by Bot. Commit: |
|
PR_Github #64592 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #64745 [ run ] triggered by Bot. Commit: |
|
/bot run --disable-fail-fast |
|
PR_Github #64745 [ run ] completed with state
|
|
PR_Github #64758 [ run ] triggered by Bot. Commit: |
|
PR_Github #64758 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #64788 [ run ] triggered by Bot. Commit: |
|
PR_Github #64788 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #64823 [ run ] triggered by Bot. Commit: |
|
PR_Github #64823 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #64837 [ run ] triggered by Bot. Commit: |
11 participants
Dev Engineer Review
2 * max_batch_sizesequence-slot headroom only for eligible disaggregated attention-DP deployments._enable_disagg_adp_overlap_headroom.QA Engineer Review
tests/integration/test_lists/waives.txt.Description
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.
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.