feat(router): expose conditional disagg configuration - #11723
Conversation
This comment has been minimized.
This comment has been minimized.
6c8ca79 to
b7e75d2
Compare
MatejKosec
left a comment
There was a problem hiding this comment.
Two small logging consistency issues (details in inline comments).
MatejKosec
left a comment
There was a problem hiding this comment.
reviewed with comments
WalkthroughThe PR adds conditional disaggregation configuration parsing, validation, defaulting, CLI integration, frontend constraints, standalone-router rejection, Python/Rust propagation, tests, and documentation. ChangesConditional disaggregation
Estimated code review effort: 3 (Moderate) | ~30 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
Comment |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
lib/bindings/python/src/dynamo/_core.pyi (1)
1811-1816: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the activation prerequisites.
The docstring describes the policies and thresholds, but it does not state the router-mode and KV-event requirements enforced by the surrounding validation paths. Add the exact prerequisites, or state that incompatible combinations are rejected during validation.
🤖 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/bindings/python/src/dynamo/_core.pyi` around lines 1811 - 1816, Update the parameter documentation near conditional_disagg_enabled and the related policy/threshold options to state the required router mode and KV-event configuration for activation, matching the surrounding validation behavior. Explicitly document that incompatible combinations are rejected during validation, and preserve the existing descriptions and defaults.
🤖 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 `@components/src/dynamo/common/configuration/groups/kv_router_args.py`:
- Around line 135-173: Update _parse_conditional_disagg_config in
components/src/dynamo/common/configuration/groups/kv_router_args.py (lines
135-173) so eff_isl_ratio_threshold rejects explicit null while preserving null
acceptance for the genuinely nullable thresholds. Add the corresponding invalid
JSON case to test_conditional_disagg_config_rejects_invalid_json in
components/src/dynamo/common/tests/configuration/test_kv_router_args.py (lines
462-489), asserting the existing “eff_isl_ratio_threshold must be a number”
validation message.
- Around line 135-173: Update _parse_conditional_disagg_config so
eff_isl_ratio_threshold is validated as a required numeric field, rejecting
explicit None just like eff_isl_threshold. Keep prefill_busy_threshold and
decode_busy_threshold in the nullable validation loop, since they remain
Optional.
In `@components/src/dynamo/frontend/frontend_args.py`:
- Around line 163-181: Update validate() to add non-negative checks for
conditional_disagg_prefill_busy_threshold and
conditional_disagg_decode_busy_threshold, matching the existing checks for
conditional_disagg_eff_isl_threshold. Raise ValueError with clear
option-specific messages when either threshold is below zero, while preserving
the current validation flow.
- Around line 163-181: Add a startup-time compatibility check for conditional
disaggregation in the SGLang serving initialization, rejecting configurations
that would send the bypass annotation to the SGLang decode handler before
accepting requests. Do not rely solely on FrontendConfig.validate(); use the
SGLang backend startup path or pass its capabilities into validation, while
preserving existing validation for other configurations.
In `@components/src/dynamo/router/tests/test_standalone_router.py`:
- Around line 94-96: Update the match pattern in
test_standalone_router_rejects_conditional_disagg so the period in “standalone
dynamo.router” is treated literally, using re.escape() or an explicitly escaped
dot while preserving the expected ValueError assertion.
In
`@docs/fern/pages/developer-guide/knowledge-base/modular-components/router/configuration-and-tuning.md`:
- Line 29: Update the Conditional Disaggregation link in the router
configuration documentation to target the existing disaggregated-serving.md
page, preserving the current section anchor and link text.
---
Nitpick comments:
In `@lib/bindings/python/src/dynamo/_core.pyi`:
- Around line 1811-1816: Update the parameter documentation near
conditional_disagg_enabled and the related policy/threshold options to state the
required router mode and KV-event configuration for activation, matching the
surrounding validation behavior. Explicitly document that incompatible
combinations are rejected during validation, and preserve the existing
descriptions and defaults.
🪄 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: baaacd8e-6cd7-4674-b78d-2452701a8918
📒 Files selected for processing (11)
components/src/dynamo/common/configuration/groups/kv_router_args.pycomponents/src/dynamo/common/tests/configuration/test_kv_router_args.pycomponents/src/dynamo/frontend/frontend_args.pycomponents/src/dynamo/router/args.pycomponents/src/dynamo/router/tests/test_standalone_router.pydocs/fern/pages/developer-guide/knowledge-base/modular-components/backends/tensorrt-llm/overview.mddocs/fern/pages/developer-guide/knowledge-base/modular-components/router/configuration-and-tuning.mddocs/fern/pages/developer-guide/knowledge-base/modular-components/router/disaggregated-serving.mddocs/fern/pages/reference/general/glossary.mdlib/bindings/python/rust/llm/entrypoint.rslib/bindings/python/src/dynamo/_core.pyi
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>
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>
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>
…ctivation Signed-off-by: Karen Chung <karenc@nvidia.com>
Signed-off-by: Karen Chung <karenc@nvidia.com>
Important
This PR is [5/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
Documentation