Repository navigation
refactor(router): centralize policy configuration and flag deprecations - #15183
ishandhanani wants to merge 3 commits into
Conversation
Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
|
/ok to test f149784 |
| config.router_prefill_load_model = value.parse()?; | ||
| } | ||
|
|
||
| config.apply_policy_config()?; |
There was a problem hiding this comment.
Applying YAML after the existing DYN_SHARED_CACHE_TYPE handling loses the established implicit 0.5 multiplier for native frontend/select-service startup. With DYN_ROUTER_POLICY_CONFIG containing only router: { shared_cache_type: hicache }, the config starts from Rust's 0.0 default, enables HiCache, and gives all shared-cache hits zero routing credit, unlike the equivalent env/CLI configuration. Preserve the paired default unless the YAML or environment explicitly supplies a multiplier.
🤖 AI Fix
When applying router YAML settings, assign the 0.5 shared-cache multiplier when YAML enables shared_cache_type: hicache and neither YAML nor environment explicitly sets shared_cache_multiplier.
Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
|
/ok to test 2ded7d0 |
jthomson04
left a comment
There was a problem hiding this comment.
Source-only review of 2ded7d088dae1419891cb51bbc826814127031d7 against 1084384acf6baf398e314127dfc17d272e480492. No tests or CI checks run.
| cancellation_token: CancellationToken, | ||
| ) -> anyhow::Result<Arc<KvRouter>> { | ||
| if let Some(config) = &mut kv_router_config { | ||
| config.apply_policy_config().map_err(anyhow::Error::msg)?; |
There was a problem hiding this comment.
P2: Apply configuration overrides in a consistent order.
YAML with router_track_active_blocks: true overwrites the false set during prefill setup. The documented load-aware migration therefore counts prefill work again as decode-block load and changes worker scores. Apply YAML before role-specific settings, and preserve those settings through router construction.
There is a related ordering issue in KvRouterConfig.from_json(): JSON deserialization calls KvRouterConfig::try_from(), which validates before apply_policy_config() runs. For example, JSON with router_track_active_blocks: false, router_track_output_blocks: true, and router_policy_config: "policy.yaml" is rejected even if the YAML sets router.router_track_active_blocks: true. The normal constructor accepts the same inputs because it applies YAML first. Replay and profiler code use this JSON path.
Please use the same order across entry points: parse inputs, apply YAML, apply role-specific settings, then validate the final configuration. The JSON case is based on source inspection; it has not been run.
| assert migrated.router_mode == legacy.router_mode | ||
| # These two values move to YAML; the remaining settings stay host-owned. | ||
| migrated_kwargs = migrated.kv_router_kwargs() | ||
| migrated_kwargs.update(overlap_score_credit=0.0, shared_cache_multiplier=0.0) |
There was a problem hiding this comment.
P3: Exercise YAML in the migration test. This test uses flags and manually patches the resulting dictionary. Load the documented YAML so the test checks the migration path.
| _ => None, | ||
| }; | ||
| if let Some(replacement) = replacement { | ||
| tracing::warn!( |
There was a problem hiding this comment.
P3: Use structured warning fields. Record key and replacement as tracing fields instead of interpolating them into the message.
| | `--shared-cache-multiplier` | `shared_cache_multiplier` | | ||
| | `--router-temperature` | `router_temperature` | | ||
|
|
||
| The host and disk cache-weight settings also affect cached-token estimates used for load accounting. Configure these in the shared `router` section or with their existing flags. |
There was a problem hiding this comment.
P3: Clarify the separate accounting and scoring weights. host_cache_hit_weight and disk_cache_hit_weight can also be set under worker_selection.instances[].parameters. The router values affect host cache estimates and prefill-load accounting; an explicit policy value changes that policy's scoring while leaving the host accounting weights unchanged. Omitted policy values inherit the host values.
For example, with router.host_cache_hit_weight: 0.75 and a policy weight of 0.25, a 100-block prompt with 40 CPU-cache hits receives 30 blocks of accounting credit but only 10 blocks of scoring credit. For an idle prefill worker with no GPU/disk hits and prefill_load_scale: 1, that gives 70 blocks of estimated prefill work and a 90-block prefill contribution to the score. Each weight is applied separately to the original hit count.
Please document these independent effects and the inheritance rule here, or use names that distinguish accounting weights from scoring weights. The identical names make it easy to tune one and expect both calculations to change.
Summary
Depends on #14785. This PR targets its branch while that parent is open, so the diff contains only the configuration and deprecation changes. Retarget to
mainafter the parent merges.Move routing configuration into
--router-policy-config:worker_selection.instances[].parameters. The builtin owns the shared-cache multiplier default:0.5when shared cache is enabled. Policy parameters override explicit deprecated CLI/env values, including zero. This also fixes native startup giving YAML-enabled HiCache zero credit while env-enabled HiCache got0.5.routersection. Python validation and Rust startup apply these settings before constructing policies and trackers. Explicit YAML values override legacy flags; omitted values keep their existing settings.--load-aware, and the redundant prefill/decode selection and queue threshold/policy flags, including their environment variables. Their existing behavior and precedence remain available during migration.All Python flag deprecations use one reusable
Deprecated(replacement, remove_in)declaration through the argument helpers. It supplies the same warning format for CLI/env use and help text, including aliases and boolean negative forms. Defaults do not warn. Native Rust env reads use one matching deprecation mapping. Removal comments target v1.7, after the v1.6 YAML migration release; wire compatibility fields keep their separate lifetime.The overlap-query response reports raw cache hits and removes
router_credit_blocks, which was calculated from global weights and could disagree with the active policy.Shared infrastructure flags remain supported, including host/disk cache weights because they also affect load accounting. Conditional-disaggregation settings remain separate. The migration guide replaces the load-aware preset with policy parameters and a
routersection in one YAML file.Related: #14778.
Validation
dynamo-kv-routerand the builtin policy crate. Covers shared settings, cached startup loading, null resets, validation, policy parameters, existing selection behavior, and shared-cache defaults/explicit-zero precedence through actual worker selection.cargo check --locked) passed. The extension was not rebuilt for runtime tests.