Repository navigation
refactor(router): move default worker selection into builtin plugin - #14785
Conversation
8c237ae to
1c6a01b
Compare
|
/ok to test 1c6a01b |
1c6a01b to
93c7415
Compare
|
/ok to test 93c7415 |
|
/ok to test efe5625 |
|
/ok to test 03cd7cf |
|
/ok to test 140a13d |
|
/ok to test 9f796fe |
9f796fe to
7dc4bb2
Compare
|
/ok to test 7dc4bb2 |
Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
jthomson04
left a comment
There was a problem hiding this comment.
Two findings from the source review of feb8b34. I did not run tests or benchmarks, or inspect CI.
Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
|
Not safe to merge yet. Blocker: missing crate versionCargo.toml line 58 defines the new workspace dependency without a version: dynamo-custom-policy-builtin = { path = "lib/router-plugins/builtin" }Publishable crates use this dependency. Suggested fix: dynamo-custom-policy-builtin = {
path = "lib/router-plugins/builtin",
version = "1.6.0"
}The release process must publish this crate before its consumers. Minor: stale workflow commentpre-merge.yml line 543 describes a build without worker-selection policies. The builtin policy dependency is now unconditional. Update the comment to mention only the optional policy catalog. ValidationThe reviewer confirmed:
Recommendation: add the workspace version, then package every affected publishable crate. |
Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
dmitry-tokarev-nv
left a comment
There was a problem hiding this comment.
Round 2 review at 1084384acf
The P1 from round 1 is fixed, and I proved it by building both sides of the commit that fixed it. One P2 and two P3 items remain, all non-blocking. Approving.
What I verified this round, and where I stopped
Scope. The pull request file list from gh api --paginate repos/ai-dynamo/dynamo/pulls/14785/files gives 83 files. A local three-dot diff against the merge base 25a8159d2d gives the same 83 files and the same names, with 4106 insertions and 1168 deletions.
Push shape. My round-1 head 1dca8375be is an ancestor of 1084384acf, so the 15 new commits are an append with no rewrite. There are no merge commits in the range. All 15 git patch-id --stable values are distinct. None of them matches any of the 16 patch identifiers already in 25a8159d2d..1dca8375be. The range is new content, not a replay. The timeline records 5 head_ref_force_pushed events, all dated 2026-09-18 and 2026-09-19, which is before round 1.
The P1 is fixed. Detail is on the lib/bindings/python/rust/llm/entrypoint.rs thread. 7101ccae97 is the commit. Its parent exits 101 with five undefined CPython symbols, and the commit itself exits 0.
CI on this head. All four rust-tests jobs ran and passed. Two of them, rust-tests (.) and rust-tests (lib/bindings/kvbm), were cancelled on both heads I reviewed in round 1. rust-tests (lib/bindings/python) and rust-tests (lib/runtime/examples) also passed. All four rust-clippy jobs passed. The workspace test suite has now run on this branch.
The #15138 overlap grew. Neither head is an ancestor of the other. git merge-tree --write-tree between 1084384acf and ef6999b061 reports 7 content conflicts across 18 overlapping files, up from 5 conflicts across 16 files at round 1. The conflicts are Cargo.lock, custom-worker-selection.mdx, lib/kv-router/src/plugins/worker_selection.rs, lib/kv-router/src/scheduling/selector/mod.rs, lib/kv-router/src/scheduling/selector/policy.rs, lib/router-plugins/builtin/Cargo.toml, and lib/router-plugins/builtin/src/lib.rs. Neither branch assumes the other landed. Whichever merges second owns the resolution.
What I retracted. I opened this round suspecting that the CACHE gating in d83839ce7b was inconsistent, because scorer.rs:135 keeps the full overlap credit when a plain-decode policy tracks prefill tokens, while scorer.rs:236 denies CACHE for that policy in every case. I dropped it. lib/router-plugins/builtin/tests/default_policy.rs:239 asserts inputs.contains(WorkerInputs::CACHE) == (role != WorkerType::Decode || conditional_disagg_enabled) over 4 roles and 2 settings, so the contract is deliberate and pinned. The pull request body states it as well. What is left is a redundant condition, not a defect.
I also dropped a suspicion that the picker changes in feb8b346c7 and 3bf711fe4d alter the choice. The host rejects an empty candidate set at lib/kv-router/src/scheduling/selector/mod.rs:384 before pick runs, a single candidate takes no draw at zero temperature, and lib/kv-router/src/scheduling/selector/policy.rs:224 rejects non-finite costs before the picker sees them.
Where I stopped. I did not compare selection end to end between the merge base and this head. I therefore have no measurement that the chosen worker is identical for every input shape. I did not establish the linker-level mechanism behind the original P1. I did not establish the crate publish set, because this repository holds no publish tooling.
Summary
Move the default worker-selection policy into
dynamo-custom-policy-builtin, alongside the two-tier policy. The default now uses the same public filter, score, and pick API as custom policies. Routing hosts install it automatically; plugin authors do not need to register or depend on the builtin crate themselves.Refs #14778.
How it works
flowchart LR H[Host: snapshot and eligible workers] --> F[Filters] F --> S[Score: costs for all candidates] S --> P[Pick: choose one candidate] P --> B[Host: validate and book capacity]preparecallback.API and configuration
WorkerInputsfor the data it needs. Cache, load, and preferred-taint access is gated independently for that component. Combined CACHE requirements determine whether the host starts cache indexing. The builtin omits CACHE at zero overlap credit and for plain disaggregated decode, preserving load-only accounting.context.cache(). Exact prompt length remains available throughcontext.prompt_tokens().dynamo-default-cost-fnthrough plugin parameters. Per-request overrides no longer replace its scoring weights or temperature.Validation
ValueErrorat the Python boundary. Both plaincargo test --locked --no-runandcargo test --lockedpass without extra linker flags.Performance
Selection reuses candidate and cost buffers, computes request-wide values once per batch, and skips unused shared-cache scans. Warmed selection checks recorded zero allocations.
The short sampling comparison was inconclusive: 0.3% faster to 3.1% slower on a noisy shared host. The accepted stacked-scorer overhead remains a follow-up. End-to-end serving impact has not been measured.