Repository navigation
feat(kv-router): add runtime-free selection service - #10641
Conversation
Signed-off-by: PeaBrane <yanrpei@gmail.com>
WalkthroughThis PR introduces a standalone HTTP service for worker selection and reservation management in the Dynamo KV Router. The implementation includes a complete type system, error handling, in-memory worker catalog, overlap scoring utilities, an Axum-based REST API, comprehensive tests, Python CLI bindings, and integration into the E2E test framework. ChangesDynamo Selection Service
🎯 4 (Complex) | ⏱️ ~75 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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/bindings/python/rust/lib.rs (1)
254-262:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winMissing
select-servicein clap error-handling feature gate.The
#[cfg(any(...))]attribute at line 255 gates clap error handling forkv-indexerandslot-trackerbut omitsselect-service. Whenrun_select_service(line 299) callsstandalone_to_pyerr, clap errors—including--helprequests—are not converted toPySystemExit; they fall through toto_pyerrand become genericPyExceptioninstances. Users runningpython -m dynamo.select_service --helpwill see an exception instead of clean help output.🐛 Proposed fix
fn standalone_to_pyerr(err: anyhow::Error) -> PyErr { - #[cfg(any(feature = "kv-indexer", feature = "slot-tracker"))] + #[cfg(any(feature = "kv-indexer", feature = "slot-tracker", feature = "select-service"))] if let Some(clap_error) = err.downcast_ref::<clap::Error>() { let _ = clap_error.print(); return pyo3::exceptions::PySystemExit::new_err(clap_error.exit_code()); } to_pyerr(err) }Once this is fixed, the exception-handling workaround in
lib/bindings/python/src/dynamo/select_service/main.py:14-18can be removed.🤖 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/rust/lib.rs` around lines 254 - 262, The clap-specific error handling in standalone_to_pyerr is incorrectly gated and omits the select-service feature, so clap::Error (which yields PySystemExit for --help/exit) falls through to to_pyerr for run_select_service; update the cfg on the clap branch in standalone_to_pyerr to include "select-service" (i.e., use #[cfg(any(feature = "kv-indexer", feature = "slot-tracker", feature = "select-service"))]) so clap errors are converted to pyo3::exceptions::PySystemExit as intended and you can remove the workaround in run_select_service/main.py.
🧹 Nitpick comments (1)
lib/kv-router/src/services/selection/error.rs (1)
52-54: ⚡ Quick winFragile error detection via string matching.
The booking conflict detection relies on
message.contains("already exists"), which will break if the upstreamKvSchedulerError::BookingFailedmessage format changes. SinceBookingFailedcarries an opaqueString, this pattern is unavoidable without refactoring the upstream error type to expose structured conflict variants.Consider adding a comment documenting this coupling, or file a follow-up issue to refactor
KvSchedulerError::BookingFailedinto separate structured variants (e.g.,BookingConflict,BookingOtherFailure) if the distinction is important.🤖 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/kv-router/src/services/selection/error.rs` around lines 52 - 54, The match arm on KvSchedulerError::BookingFailed currently detects conflicts by string matching via message.contains("already exists"), which is fragile; update the code by adding an explicit comment above the KvSchedulerError::BookingFailed match (referencing KvSchedulerError::BookingFailed and the message.contains("already exists") test) documenting the coupling to the upstream message format and the need to refactor the upstream error into structured variants (e.g., BookingConflict vs BookingOtherFailure) as a follow-up; optionally create or link a follow-up issue to refactor KvSchedulerError so future maintainers know this is a technical debt hotspot.
🤖 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/bindings/python/src/dynamo/select_service/main.py`:
- Around line 10-19: The try/except workaround in main (function main calling
run_select_service) masks errors and should be removed once clap errors are
converted to Py exceptions by Rust; update the cfg on the standalone_to_pyerr
feature gate (the #[cfg(any(...))] that guards standalone_to_pyerr) to include
feature = "select-service" so clap help/exit become PySystemExit, then delete
the try/except block in main (the code that checks for "-h"/"--help", prints
exc, and re-raises) leaving a straight call to run_select_service(args) and
returning 0; after removing the workaround ensure no stdout print of exceptions
remains and let Python receive the proper exception converted by
standalone_to_pyerr.
---
Outside diff comments:
In `@lib/bindings/python/rust/lib.rs`:
- Around line 254-262: The clap-specific error handling in standalone_to_pyerr
is incorrectly gated and omits the select-service feature, so clap::Error (which
yields PySystemExit for --help/exit) falls through to to_pyerr for
run_select_service; update the cfg on the clap branch in standalone_to_pyerr to
include "select-service" (i.e., use #[cfg(any(feature = "kv-indexer", feature =
"slot-tracker", feature = "select-service"))]) so clap errors are converted to
pyo3::exceptions::PySystemExit as intended and you can remove the workaround in
run_select_service/main.py.
---
Nitpick comments:
In `@lib/kv-router/src/services/selection/error.rs`:
- Around line 52-54: The match arm on KvSchedulerError::BookingFailed currently
detects conflicts by string matching via message.contains("already exists"),
which is fragile; update the code by adding an explicit comment above the
KvSchedulerError::BookingFailed match (referencing
KvSchedulerError::BookingFailed and the message.contains("already exists") test)
documenting the coupling to the upstream message format and the need to refactor
the upstream error into structured variants (e.g., BookingConflict vs
BookingOtherFailure) as a follow-up; optionally create or link a follow-up issue
to refactor KvSchedulerError so future maintainers know this is a technical debt
hotspot.
🪄 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: 58e1a8e6-3687-452f-8887-b3d1aba76083
📒 Files selected for processing (22)
lib/bindings/python/Cargo.tomllib/bindings/python/rust/lib.rslib/bindings/python/rust/llm/kv.rslib/bindings/python/src/dynamo/llm/__init__.pylib/bindings/python/src/dynamo/select_service/__init__.pylib/bindings/python/src/dynamo/select_service/__main__.pylib/bindings/python/src/dynamo/select_service/main.pylib/kv-router/Cargo.tomllib/kv-router/src/services/mod.rslib/kv-router/src/services/selection/README.mdlib/kv-router/src/services/selection/catalog.rslib/kv-router/src/services/selection/error.rslib/kv-router/src/services/selection/input.rslib/kv-router/src/services/selection/mod.rslib/kv-router/src/services/selection/scoring.rslib/kv-router/src/services/selection/server.rslib/kv-router/src/services/selection/tests.rslib/kv-router/src/services/selection/types.rstests/router/common.pytests/router/helper.pytests/router/mocker_process.pytests/router/test_router_e2e_with_mockers.py
Signed-off-by: PeaBrane <yanrpei@gmail.com>
Signed-off-by: PeaBrane <yanrpei@gmail.com>
Signed-off-by: PeaBrane <yanrpei@gmail.com>
Signed-off-by: PeaBrane <yanrpei@gmail.com>
Summary
dynamo.select_serviceentrypoint and extends router mocker tests to exercise standalone selector scoring.Validation
cargo test -p dynamo-kv-router --features standalone-selection services::selection --no-default-featurescargo test --features select-service --lib run_select_servicefromlib/bindings/pythonSummary by CodeRabbit
Release Notes
python -m dynamo.select_service