feat(router): orchestrate conditional disagg bypass - #11725
Conversation
PeaBrane
left a comment
There was a problem hiding this comment.
AI-assisted review of the conditional-disagg orchestration and stacked contracts. Posting these as neutral questions and edge cases for consideration.
PeaBrane
left a comment
There was a problem hiding this comment.
Approving on the condition that the AI-assisted review comments above are resolved.
e7dfe16 to
dea4358
Compare
7543bc5 to
b2edaef
Compare
e126b5e to
e71349b
Compare
49168ef to
eabd007
Compare
Signed-off-by: Karen Chung <karenc@nvidia.com>
Signed-off-by: Karen Chung <karenc@nvidia.com>
Signed-off-by: Karen Chung <karenc@nvidia.com>
Signed-off-by: Karen Chung <karenc@nvidia.com>
eabd007 to
3eb4288
Compare
WalkthroughChangesThe PR adds conditional-disaggregation support to the prefill router, including decode-worker selection without admission, session-affinity lookup, worker-busy gating, bypass stream generation, and updated Conditional disaggregation routing
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
lib/llm/src/kv_router/prefill_router/conditional_bypass.rs (1)
231-243: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the dead
decode_chosen_worker_busywrite
ConditionalDisaggDecisionInputisCopy, so this still compiles, but the updated value is never used:should_bypass_remote_prefillhas already run, and the decision/log path readsdecode_busydirectly. Remove the assignment unless a policy will consume the field.🤖 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 `@lib/llm/src/kv_router/prefill_router/conditional_bypass.rs` around lines 231 - 243, Remove the unused input reassignment via with_decode_chosen_worker_busy in the conditional bypass flow. Keep the decode_busy computation intact, since the existing decision and logging paths already use it directly, and do not add a replacement field update unless a policy consumes that field.
🤖 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 `@lib/kv-router/src/scheduling/queue.rs`:
- Around line 693-716: The capacity checks in worker_is_prefill_busy and
projected_decode_load_exceeds currently return None when configuration capacity
is unavailable, unlike the queue path’s DEFAULT_MAX_BATCHED_TOKENS fallback.
Align both methods with that queue behavior by using the established fallback
when capacity is missing, while preserving their existing Option-based
worker/config lookup handling.
In `@lib/llm/src/kv_router/prefill_router/conditional_bypass.rs`:
- Around line 174-175: Update both conditional-disaggregation probes in
conditional_bypass.rs: at lines 174-175 and 352-353, pass the request’s
policy_class and session_id to find_best_match_details_without_admission instead
of None, reusing the values derived by the normal KvPushRouter::select_request
path so both advisory checks inspect the same class queue and session view as
real routing.
- Around line 125-156: Reorder the pin resolution in the conditional bypass flow
so resolve_request_decode_pin is evaluated before decode_affinity_target. If the
request pin resolves, use it without aborting when affinity lacks a DP rank;
only evaluate and fall back to affinity when no request pin exists, while
preserving unresolved request-pin behavior and the existing pinned_worker
precedence.
---
Nitpick comments:
In `@lib/llm/src/kv_router/prefill_router/conditional_bypass.rs`:
- Around line 231-243: Remove the unused input reassignment via
with_decode_chosen_worker_busy in the conditional bypass flow. Keep the
decode_busy computation intact, since the existing decision and logging paths
already use it directly, and do not add a replacement field update unless a
policy consumes that field.
🪄 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: 4c2d1a8f-45f6-46b1-b4db-a3b8e29d0669
📒 Files selected for processing (13)
deploy/inference-gateway/ext-proc/src/epp.rslib/bindings/c/src/lib.rslib/kv-router/src/scheduling/local.rslib/kv-router/src/scheduling/queue.rslib/llm/src/discovery/watcher.rslib/llm/src/entrypoint/input/common.rslib/llm/src/kv_router.rslib/llm/src/kv_router/prefill_router/activation.rslib/llm/src/kv_router/prefill_router/conditional_bypass.rslib/llm/src/kv_router/prefill_router/mod.rslib/llm/src/kv_router/push_router.rslib/llm/src/kv_router/push_router/selection.rslib/llm/src/kv_router/scheduler.rs
Signed-off-by: Karen Chung <karenc@nvidia.com>
…rchestration Signed-off-by: Karen Chung <karenc@nvidia.com>
|
One design question after tracing this path: The scheduler already has the information needed here: which worker it selected, the projected load for that worker, and the worker’s capacity. At the moment, the advisory selection returns only part of that information. Would it be cleaner for the advisory selection to return the relevant load measurements together with the selected worker? For example, a small result could contain the worker, cached tokens, projected decode blocks, and total KV capacity. The conditional-disaggregation code could then apply its configured threshold directly. I think that would make the responsibilities easier to follow. The scheduler would report the measurements it already owns, while conditional disaggregation would decide what those measurements mean for the bypass policy. It would also let the worker selection and the associated load/capacity measurements come from the same observation, rather than selecting first and querying some of the state afterward. I’m not suggesting a broad snapshot of everything in the scheduler—just a small, purpose-built advisory result. This would not remove the acknowledged race between advisory selection and real admission, but it would make this path more cohesive and avoid adding more one-off load queries through every layer as conditional disaggregation evolves. I do not think this needs to block the PR, but I wanted to raise it as a possible cleanup direction. |
Signed-off-by: Karen Chung <karenc@nvidia.com>
|
Signed-off-by: Karen Chung <karenc@nvidia.com>
Signed-off-by: Karen Chung <karenc@nvidia.com>
Signed-off-by: Karen Chung <karenc@nvidia.com>
MatejKosec
left a comment
There was a problem hiding this comment.
Approved, once comments resolved.
Important
This PR is [4/5] in the conditional-disaggregation stack.
Merge order is bottom-up: #11718 merges to
mainfirst, and #11723 merges last.Do not merge this PR until every earlier PR in the checklist has merged and this branch has been rebased/retargeted as needed.
Link to the DEP: #11514
Stack merge order:
Reference to original (closed) combined PR, with addressed review comments: ai-dynamo/dynamo#11357
Overview:
Details:
Where should the reviewer start?
Related Issues
🔗 This PR is linked to an issue:
🚫 This PR is NOT linked to an issue:
Summary by CodeRabbit
New Features
Bug Fixes