[None][fix] ADP router crashes on serve when scheduling_params.attent… - #14267
Conversation
|
/bot run --disable-fail-fast |
📝 WalkthroughWalkthroughThis PR fixes a crash in ADP request scheduling when ChangesADP Router Relax None Normalization
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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.
🧹 Nitpick comments (2)
tests/unittest/_torch/executor/test_adp_router.py (2)
448-493: QA list updates are unnecessary for this PR.This is a unittest-only regression addition; no
tests/integration/test_lists/qa/*change is needed.As per coding guidelines "If the PR only touches unittest/ or narrow unit scope, say explicitly whether QA list updates are unnecessary or optional."
🤖 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_adp_router.py` around lines 448 - 493, The PR should explicitly state that QA list updates are unnecessary because this change only touches unit tests; update the PR description or commit message to include a line like "QA list updates are unnecessary for this PR" referencing that the change is confined to tests/unittest/_torch/executor/test_adp_router.py (specifically the test_schedule_attention_dp_requests_serve_default_relax_None_does_not_crash addition) so reviewers and release managers know no tests/integration/test_lists/qa/* edits are required.
448-493: ⚡ Quick winAdd a matching
KVCacheAwareADPRouterregression forattention_dp_relax=None.This new test validates only the default router path (
_assign(...)), while the PR also changesKVCacheAwareADPRouter.get_relax_value. Add one parallel test to lock coverage on that second code path.Suggested test shape
+def test_kv_cache_aware_schedule_attention_dp_requests_serve_default_relax_none_does_not_crash(): + from tensorrt_llm.scheduling_params import SchedulingParams + + dist = _mock_dist(tp_size=2) + kv_cache_manager = MagicMock() + kv_cache_manager.enable_block_reuse = True + router = KVCacheAwareADPRouter(dist=dist, kv_cache_manager=kv_cache_manager) + + states = [ + RankState(rank=0, num_active_requests=0, num_active_tokens=0), + RankState(rank=1, num_active_requests=0, num_active_tokens=0), + ] + + def _make_req(req_id): + r = Mock() + r.py_scheduling_params = SchedulingParams() # relax/rank default to None + r.input_token_ids = [1, 2, 3] + return RequestQueueItem(req_id, r) + + reqs = [_make_req(i) for i in range(4)] + result, _ = router.route_requests(states, reqs, max_num_active_requests=8) + assert sum(len(v) for v in result.values()) == 4As per coding guidelines "Coverage expectations: Assess whether new/changed tests cover happy path, important edge cases, and failure modes relevant to the feature or fix."
🤖 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_adp_router.py` around lines 448 - 493, Add a parallel regression test that exercises the KVCacheAwareADPRouter code path when attention_dp_relax is None: create a new test (e.g., test_schedule_attention_dp_requests_kvcacheaware_relax_None_does_not_crash) that mirrors the existing test but invokes the KVCacheAwareADPRouter path (call the router method that uses KVCacheAwareADPRouter.get_relax_value or otherwise ensure get_relax_value from KVCacheAwareADPRouter is executed), build RequestQueueItem instances with mock_request.py_scheduling_params = SchedulingParams() and input_token_ids like the existing test, then run the same assignment logic (or call the public entry that triggers _assign behavior) and assert no exception and that all requests are assigned; reference SchedulingParams, RequestQueueItem, and KVCacheAwareADPRouter.get_relax_value to locate and mirror the existing test flow.
🤖 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.
Nitpick comments:
In `@tests/unittest/_torch/executor/test_adp_router.py`:
- Around line 448-493: The PR should explicitly state that QA list updates are
unnecessary because this change only touches unit tests; update the PR
description or commit message to include a line like "QA list updates are
unnecessary for this PR" referencing that the change is confined to
tests/unittest/_torch/executor/test_adp_router.py (specifically the
test_schedule_attention_dp_requests_serve_default_relax_None_does_not_crash
addition) so reviewers and release managers know no
tests/integration/test_lists/qa/* edits are required.
- Around line 448-493: Add a parallel regression test that exercises the
KVCacheAwareADPRouter code path when attention_dp_relax is None: create a new
test (e.g.,
test_schedule_attention_dp_requests_kvcacheaware_relax_None_does_not_crash) that
mirrors the existing test but invokes the KVCacheAwareADPRouter path (call the
router method that uses KVCacheAwareADPRouter.get_relax_value or otherwise
ensure get_relax_value from KVCacheAwareADPRouter is executed), build
RequestQueueItem instances with mock_request.py_scheduling_params =
SchedulingParams() and input_token_ids like the existing test, then run the same
assignment logic (or call the public entry that triggers _assign behavior) and
assert no exception and that all requests are assigned; reference
SchedulingParams, RequestQueueItem, and KVCacheAwareADPRouter.get_relax_value to
locate and mirror the existing test flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 6573e9ed-2d0d-4e9a-98fb-c029733b7d90
📒 Files selected for processing (2)
tensorrt_llm/_torch/pyexecutor/scheduler/adp_router.pytests/unittest/_torch/executor/test_adp_router.py
|
PR_Github #48922 [ run ] triggered by Bot. Commit: |
|
PR_Github #48922 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #49029 [ run ] triggered by Bot. Commit: |
|
PR_Github #49029 [ run ] completed with state |
b2e0040 to
7577b60
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #49436 [ run ] triggered by Bot. Commit: |
|
PR_Github #49436 [ run ] completed with state
|
…ion_dp_relax is None Signed-off-by: nv-guomingz <137257613+nv-guomingz@users.noreply.github.com>
7577b60 to
4f1ae83
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #49529 [ run ] triggered by Bot. Commit: |
|
PR_Github #49529 [ run ] completed with state |
NVIDIA#14267) Signed-off-by: nv-guomingz <137257613+nv-guomingz@users.noreply.github.com>
NVIDIA#14267) Signed-off-by: nv-guomingz <137257613+nv-guomingz@users.noreply.github.com>
3 participants
…ion_dp_relax is None
Summary by CodeRabbit
Bug Fixes
Tests
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.