Repository navigation
refactor(router): unify classifier inputs; deprecate absolute busy thresholds - #16620
Conversation
✅ Dynamo PR CI passed — run 40457088307 (attempt 1) on
|
dabbcb0 to
54657de
Compare
54657de to
9577af0
Compare
ishandhanani
left a comment
There was a problem hiding this comment.
Deprecation only. Warnings are structured, the flag help and every docs page that names the flag are updated, and the "zero queue limit never queues" paragraph matches the existing zero_class_queue_limit_never_queues test. One nit inline.
9577af0 to
0390cb4
Compare
033100f to
a6fd83f
Compare
dd707fa to
19d6f88
Compare
19d6f88 to
5df9abc
Compare
jthomson04
left a comment
There was a problem hiding this comment.
Context naming verified on 5df9abc.
5df9abc to
4f6018e
Compare
82f14fc to
311f02f
Compare
The absolute prefill thresholds duplicate the fractional ones and hide an unreported `max_num_batched_tokens` behind a 10,000,000-token default. Deprecate policy YAML `prefill_busy_threshold` and `--active-prefill-tokens-threshold` for removal in v1.7; both log a warning. Also document that a policy class with a zero queue limit rejects instead of waiting when its workers are full, which is how a class fails fast without a separate setting. Part of #16543. Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
The classifier's per-request object was `ClassifyRequest`, while worker selection's per-request object is `WorkerSelectionContext`. Rename it so the name says what it holds, and give shared request facts one name: - `ClassifyRequest` -> `RequestClassificationContext`, the per-request facts and overrides `classify` receives and returns. - `RequestClassifierContext`, the router-wide block size and registered workers given to a classifier factory, keeps its name; `RequestClassifierContext::workers()` now returns the `WorkerInfo` worker selection reads, which also carries `max_num_batched_tokens`. - `RequestClassifierWorker` is removed in favor of `WorkerInfo`. - The request's input sequence length is `isl_tokens()` in both contexts, matching `SchedulingRequest::isl_tokens` and the policy YAML's ISL buckets: `WorkerSelectionContext::prompt_tokens()` and `RequestClassificationContext::input_tokens()` are renamed. This breaks out-of-tree classifiers and selection plugins that use the old names; in-tree users and docs are updated. Part of #16543. Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
ba41d9e to
508ed49
Compare
kthui
left a comment
There was a problem hiding this comment.
P3 — Low priority. The new warning is placed in RouterConfigBase.validate_rejection_thresholds(), whose only production caller is frontend validation. However, the same CLI flag and environment variable are also accepted by the worker-side router advertisement parser.
For example, a worker configured with:
--router-mode kv --active-prefill-tokens-threshold 123
passes through parse_worker_router_config() and build_router_config() without invoking that validator. The absolute value is forwarded into the advertised router configuration. The concrete vLLM path calls the parser in args.py and builds the advertisement in main.py; TRT-LLM and SGLang use the same shared functions. The Rust binding constructor also accepts the value without emitting this warning.
Consequence: operators configuring the deprecated setting through worker advertisements miss the promised startup migration notice. This is a gap in the new deprecation behavior, not evidence of incorrect routing or rejection today.
Recommended correction: cover active worker advertisements in the shared configuration path, preserving the frontend's single warning and the current unset behavior. Add coverage for both worker CLI and environment use.
Non-blocking because the absolute setting remains accepted and enforced.
…tisements The --active-prefill-tokens-threshold deprecation warning lived in RouterConfigBase.validate_rejection_thresholds(), which only the frontend runs. A worker advertising a router config with the flag or with DYN_ACTIVE_PREFILL_TOKENS_THRESHOLD forwarded the value through parse_worker_router_config() (vLLM, SGLang, TRT-LLM) or WorkerRouterConfig.from_cli_args() (mocker) without the startup notice. Emit the warning from build_router_config() instead: every advertisement, the frontend's included, is built there, so each process warns exactly once and only when the deprecated value is actually forwarded. A worker that omits --router-mode advertises nothing and stays quiet, as before. Cover worker CLI and environment use in test_router_advertisement.py and pin the frontend's single warning in test_kv_router_args.py. Part of #16543. Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
Part of #16543 (PR 5 of 5)
Summary
Unify classifier worker metadata and input sequence length naming with worker selection, and deprecate the absolute prefill busy thresholds.
Classifier inputs (breaking)
The classifier retains
ClassifyRequestfor its per-request object andRequestClassifierContextfor its router-scoped installation context.RequestClassifierContext::workers()returns theWorkerInfoworker selection reads (refactor(router): judge queue capacity on the shared LOAD row #16617), which also carriesmax_num_batched_tokens.RequestClassifierWorkeris removed.isl_tokens()in both plugin APIs, matchingSchedulingRequest::isl_tokensand the policy YAML's ISL buckets.WorkerSelectionContext::prompt_tokens()andClassifyRequest::input_tokens()returned the same value under two names.Absolute prefill thresholds (deprecated)
The absolute prefill thresholds duplicate the fractional ones, and a worker that does not report
max_num_batched_tokenssilently gets a 10,000,000-token default under them. Deprecated for removal in v1.7:prefill_busy_threshold(logs a warning per class at load);--active-prefill-tokens-threshold/DYN_ACTIVE_PREFILL_TOKENS_THRESHOLD(logs a warning at startup frombuild_router_config(), so the frontend and any worker advertising the value each warn once; a worker without--router-modeadvertises nothing and stays quiet).Use the
_fracforms instead. Docs updated.The router policy docs now also state that a class with any queue limit set to
0never queues: it admits while an eligible worker is below the class's busy thresholds, and rejects with 529 when every eligible worker is busy (zero_class_queue_limit_never_queuescovers this). That is how a class fails fast when its workers are full, so the DEP'son_full: rejectsetting is not added: capacity-based rejection stays router-wide (/busy_thresholdand the frontend flags), and classes wait.Validation
After moving the deprecation warning into
build_router_config()(review follow-up):test_router_advertisement.pyandtest_kv_router_args.py: 99 passed, 3 skipped (backends not installed); the two new worker CLI/env cases fail without the fix.mocker/tests/unit/test_config.py,frontend/tests/test_batch_gateway_args.py,frontend/tests/test_batch_gateway_transport.py: 39 passed.pre-commiton the touched files: clean.After restoring
ClassifyRequest:cargo fmt --all --checkandgit diff --check: clean.cargo test --locked -p dynamo-kv-router --lib --test plugin_api_compat: 1127 unit tests and 1 plugin API compatibility test passed; 3 ignored.cargo test --locked -p dynamo-custom-policy-builtin: 86 tests passed; 2 ignored.cargo check --locked -p dynamo-llm --no-default-features --tests: passed.Earlier stack validation (before restoring the type name):
cargo fmt --all --checkandcargo clippy --workspace --all-targets -- -D warnings: clean.cargo test -p dynamo-kv-router --lib: 1120 passed;plugin_api_compat: passed.cargo test -p dynamo-custom-policy-builtin: passed.cargo test -p dynamo-mocker --lib: 272 passed.cargo test -p dynamo-llm --lib -- kv_router discovery::worker_monitor: 489 passed (includingclassifier_context_tracks_capacity_and_registered_ranks, which checksmax_num_batched_tokens).test_kv_router_args.py: the absolute CLI flag warns; the other flags do not. 2ais_perf_moecases fail identically onmain(localaisimulateimport issue).codespell, trailing whitespace, Fern asset paths) on the touched pages: clean.Stack