[None][fix] cold-start warmup for KV-aware ADP router - #14307
Conversation
1c80f10 to
0fc5a32
Compare
With match_rate_threshold=0.1 the cache-affinity gate stays ON for any
system prompt longer than ~10% of a request. Once a single rank caches
the prompt, the (req_tokens - match_len) term dominates the load term
and subsequent single-request batches keep landing on that warm rank
until the fair-share cap finally evicts it. Other ranks never warm up
and pay the full prefill cost when traffic eventually spills over.
Track _pending_warmup_ranks (Set[int]) on the router, initialised in
__init__ to {0, ..., tp_size-1}. In route_requests, any target_dp_rank
-- explicit strict or synthesised warmup -- discards its rank from the
set; relaxed requests with no explicit target synthesise
min(_pending_warmup_ranks) so warmup spreads the shared prompt across
every rank within roughly the first tp_size requests. Strict
pre-assignments count toward the warmup naturally and cap saturation
also discards so the synthesiser never loops on a busy rank. Once the
set empties the warmup path is dormant forever and routing reverts to
pure scoring.
Tests that exercise pure scoring behaviour opt out via
router._pending_warmup_ranks = set() -- 3 tests in
test_kvcache_aware_router.py and the shared _make_router helper in
test_adp_router.py.
Scope: single shared system prompt. Multi-prompt warmup is a
follow-up.
Signed-off-by: Lanyu Liao <lancelly@users.noreply.github.com>
0fc5a32 to
b3c5632
Compare
📝 WalkthroughWalkthroughThe PR adds cold-start warmup behavior to the KVCacheAwareADPRouter. It tracks pending tensor-parallel ranks that need initial requests, assigns new unrouted requests to the smallest unwarmed rank before cache-aware scoring, and removes ranks from the warmup set once targeted. Existing tests disable warmup to validate pure scoring. New tests comprehensively validate warmup initialization, dispatch, and fallback behavior. ChangesWarmup-driven routing for ADP
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tensorrt_llm/_torch/pyexecutor/scheduler/adp_router.py (1)
26-26: 💤 Low valueConsider using built-in
setinstead oftyping.Set.Per coding guidelines, prefer built-in types (
set) over legacytyping.Setsince Python 3.10+.🔧 Suggested change
-from typing import TYPE_CHECKING, Dict, List, Set, Tuple +from typing import TYPE_CHECKING, Dict, List, TupleAnd at line 455:
- self._pending_warmup_ranks: Set[int] = set(range(self.dist.tp_size)) + self._pending_warmup_ranks: set[int] = set(range(self.dist.tp_size))🤖 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 `@tensorrt_llm/_torch/pyexecutor/scheduler/adp_router.py` at line 26, Replace usage of typing.Set with the built-in set: remove Set from the typing import list in adp_router.py and update all annotations that use Set[...] to use the modern built-in syntax set[...] (e.g., change Set[int] to set[int]). Search for occurrences in this module (e.g., any functions or variables annotated with Set like in router logic) and update their annotations accordingly; keep other typing imports (Dict, List, Tuple, TYPE_CHECKING) as-is.
🤖 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 `@tensorrt_llm/_torch/pyexecutor/scheduler/adp_router.py`:
- Line 26: Replace usage of typing.Set with the built-in set: remove Set from
the typing import list in adp_router.py and update all annotations that use
Set[...] to use the modern built-in syntax set[...] (e.g., change Set[int] to
set[int]). Search for occurrences in this module (e.g., any functions or
variables annotated with Set like in router logic) and update their annotations
accordingly; keep other typing imports (Dict, List, Tuple, TYPE_CHECKING) as-is.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9a744c6b-eb4e-43fe-82b5-478a7a0992ba
📒 Files selected for processing (3)
tensorrt_llm/_torch/pyexecutor/scheduler/adp_router.pytests/unittest/_torch/executor/test_adp_router.pytests/unittest/_torch/executor/test_kvcache_aware_router.py
|
/bot run --disable-fail-fast |
|
PR_Github #49216 [ run ] triggered by Bot. Commit: |
|
PR_Github #49216 [ run ] completed with state |
Cold-start round-robin warmup is now controlled by ``AttentionDpConfig.kv_cache_routing_cold_start_warmup`` (default False). The previous behaviour always seeded ``_pending_warmup_ranks`` from ``dist.tp_size``, which forces the first tp_size relaxed requests across ranks regardless of cache affinity. That helps the assumed "shared system prompt" case but can scatter requests with partially overlapping prompts onto cold ranks, wasting prefill. Default False preserves pre-warmup routing for diverse-prompt workloads; users hitting cold-start pinning opt in explicitly. Tests in TestKVCacheAwareADPRouterWarmup now pass ``cold_start_warmup= True`` and a new ``test_disabled_by_default`` covers the default. The three opt-out hacks in scoring tests (manual ``_pending_warmup_ranks = set()``) are no longer needed and are removed. Signed-off-by: Lance Liao <108499334+lancelly@users.noreply.github.com>
Pure cosmetic sync with feat/deepseek_v4 NVIDIA#14388: move the inline comment in ``test_empty_pending_disables_warmup`` to a standalone line above the assignment, matching the form there. No behaviour change. Signed-off-by: Lance Liao <108499334+lancelly@users.noreply.github.com>
The ruff-legacy hook (D200) fired on 24 docstrings in
``tensorrt_llm/llmapi/llm_args.py`` that follow the multi-line
form for a single line of text:
\"\"\"
Configuration for X.
\"\"\"
These predate this PR but the gate only checks files touched by a
change, so this PR was the one to surface them. Collapsing to the
one-line form satisfies D200 without touching semantics or the
baseline ratchet.
Signed-off-by: Lance Liao <108499334+lancelly@users.noreply.github.com>
|
/bot run --disable-fail-fast |
|
PR_Github #49624 [ run ] triggered by Bot. Commit: |
|
PR_Github #49624 [ run ] completed with state |
A saturated warmup pick was being unconditionally removed from ``_pending_warmup_ranks``, so a rank could be marked as warmed without ever receiving a request and never having its shared prefix seeded. Move the ``discard`` under the scheduled branch so only ranks that actually received a request leave the pending set. Also use ``dist.mapping.dp_size`` instead of ``dist.tp_size`` for the initial pending set; the values match under ``enable_attention_dp=True`` but ``dp_size`` reads more clearly at the call site. Addresses reviewer feedback on PR NVIDIA#14307. Signed-off-by: Lance Liao <108499334+lancelly@users.noreply.github.com>
|
/bot run --disable-fail-fast |
|
PR_Github #49819 [ run ] triggered by Bot. Commit: |
A saturated warmup pick was being unconditionally removed from ``_pending_warmup_ranks``, so a rank could be marked as warmed without ever receiving a request and never having its shared prefix seeded. Move the ``discard`` under the scheduled branch so only ranks that actually received a request leave the pending set. Also use ``dist.mapping.dp_size`` instead of ``dist.tp_size`` for the initial pending set; the values match under ``enable_attention_dp=True`` but ``dp_size`` reads more clearly at the call site. Addresses reviewer feedback on PR NVIDIA#14307. Signed-off-by: Lance Liao <108499334+lancelly@users.noreply.github.com>
|
/bot run --disable-fail-fast |
|
/bot run --disable-fail-fast |
|
PR_Github #49821 [ run ] triggered by Bot. Commit: |
|
PR_Github #49823 [ run ] triggered by Bot. Commit: |
|
PR_Github #49821 [ run ] completed with state |
|
PR_Github #49819 [ run ] completed with state |
|
/bot run --disable-fail-fast |
|
PR_Github #49858 [ run ] triggered by Bot. Commit: |
|
PR_Github #49823 [ run ] completed with state |
|
/bot run --disable-fail-fast |
|
PR_Github #49862 [ run ] triggered by Bot. Commit: |
|
PR_Github #49858 [ run ] completed with state |
|
/bot run --disable-fail-fast |
|
PR_Github #49879 [ run ] triggered by Bot. Commit: |
|
PR_Github #49862 [ run ] completed with state |
|
PR_Github #49879 [ run ] completed with state |
Signed-off-by: Lanyu Liao <lancelly@users.noreply.github.com> Signed-off-by: Lance Liao <108499334+lancelly@users.noreply.github.com> Co-authored-by: Lanyu Liao <lancelly@users.noreply.github.com>
Signed-off-by: Lanyu Liao <lancelly@users.noreply.github.com> Signed-off-by: Lance Liao <108499334+lancelly@users.noreply.github.com> Co-authored-by: Lanyu Liao <lancelly@users.noreply.github.com>
Summary
Cold-start mitigation for the KV-aware ADP router: round-robin the first relaxed requests across all ranks so every rank caches the (assumed shared) system prompt before cache-affinity scoring would otherwise pin all traffic to the first warm rank.
Problem
With
match_rate_threshold=0.1the cache-affinity gate stays ON for any system prompt longer than ~10% of a request. Once a single rank caches that prompt, the(req_tokens - match_len)term dominates the load term:Fix
KVCacheAwareADPRoutertracks_pending_warmup_ranks: Set[int]:{0, ..., tp_size-1}only whencold_start_warmup=True; otherwise an empty set.route_requests, anytarget_dp_rank(explicit strict or synthesised warmup) discards its rank from the set. Relaxed requests with no explicit target synthesisemin(_pending_warmup_ranks)while the set is non-empty.Opt-in (default False)
The behaviour is gated by
AttentionDpConfig.kv_cache_routing_cold_start_warmup(defaultFalse). The warmup is a win for workloads that share a long system prompt, but for diverse-prompt workloads it can scatter requests onto cold ranks and bypass cache-affinity scoring entirely for the firsttp_sizerequests — wasting prefill work the scoring path would otherwise consolidate. DefaultFalsepreserves pre-warmup routing; users who hit cold-start pinning opt in explicitly.Test layout:
TestKVCacheAwareADPRouterWarmupconstructs its router withcold_start_warmup=Trueto exercise the new code path, plus a newtest_disabled_by_defaultasserting the empty pending set when the flag is absent._pending_warmup_ranks = set()opt-out (default is already off).