feat(routing): add expected_output_tokens signal to RouteContext - #3163
Conversation
Read the caller's max_tokens / max_completion_tokens (max_tokens wins if both are present) into RouteContext::Params::expected_output_tokens, a ceiling rather than an estimate, left unset when neither field is sent or the value is non-positive/non-integer. No ranking logic changes yet: this is plumbing for a later token-weighted cost_select ranking (raised in PR lemonade-sdk#3100 review), which will read this field once it exists.
|
The new loop runs for all three but only knows the first two names, so a Responses { "model": "my-router", "input": "summarize this", "max_output_tokens": 256 } Looks like the third name just didn't get added: Could we add that plus a test? |
…ut_tokens build_route_context only checked max_tokens/max_completion_tokens, so a collection.router request through /v1/responses never populated expected_output_tokens even when the caller set max_output_tokens — the name that endpoint uses for the same limit. Once token-weighted cost_select ranking lands, that endpoint would silently rank on a guessed output length instead of the real one. Addresses review feedback from meghsat on PR lemonade-sdk#3163.
|
Fixed in 17e2af7, added max_output_tokens to the key list plus a test using a Responses-shaped request (test_build_route_context_reads_max_output_tokens_for_responses). |
meghsat
left a comment
There was a problem hiding this comment.
LGTM! Thank you for addressing the feedback
ramkrishna2910
left a comment
There was a problem hiding this comment.
Approving. Reviewed the parse path, the ops-spec obligations, and the interaction with the other three in-flight routing PRs.
Why this is correct as-is, including the no-schema/no-docs choice. expected_output_tokens is provably unreachable from the policy language today: routing_match_expr_keys() is a closed set and parse_match_expr opens with reject_unknown_keys(...), so a policy naming the field is rejected at load; no condition factory, classifier, or desugaring reads it (the llm-router prompt still emits only has_tools/has_images/chars). The schemas README scopes its evolution rule to vocabulary — "a new condition op, classifier type, or decision field" — and a RouteContext::Params member no op can address is none of those. So this correctly ships no schema, no lock refresh, and no router-policy.md change, while #3169 and #3181 each correctly ship all of them. Nothing is redefined, and the README's reservation of a future min_tokens/max_tokens for input length is left intact.
Parsing is appropriately defensive. Absent, null, zero, negative, and string values all resolve to nullopt, and an invalid first key correctly falls through to the next since the validity test is inside the loop condition. Rejecting -1 is right — that's llama.cpp's "until context is full", not a ceiling.
Two things to pick up in the follow-up that adds a consumer, neither blocking here:
-
Precedence disagrees with the vLLM backend. This reads
max_tokensfirst, so it wins when both are present;vllm_server.cpp:617prefersmax_completion_tokens. For a body carryingmax_tokens: 128, max_completion_tokens: 512— exactly the casetest_build_route_context_max_tokens_wins_over_max_completion_tokenspins — routing would reason about 128 while vLLM generates up to 512. Harmless while nothing reads the field; worth reconciling before something does. Note the cited justification doesn't quite hold:JsonUtils::add_legacy_max_tokens_aliasmirrors only whenmax_tokensis absent, so it says nothing about the both-present case. -
Ceiling vs. estimate. The header comment already flags this correctly, and it matters for the token-weighted cost ranking this is presumably headed for: a caller who sets
max_tokens: 4096as a safety cap but typically gets 200-token replies would skew a cost ranking harder than a fixed default would. Whoever wires up the consumer should make the ceiling-vs-estimate call deliberately rather than treating this as a drop-in.
Minor: the header says "max_tokens / max_completion_tokens" but the code also reads max_output_tokens — worth adding to the contract surface. Also untested: a JSON float (256.0) is number_float, so is_number_integer() is false and it is silently treated as absent, which some SDKs and JSON round-trips will produce.
The large-value path is safe but accidentally so — anything above LLONG_MAX is rejected as a side effect of get<long long>() narrowing rather than by an explicit guard. vllm_server.cpp:607 tests is_number_integer() || is_number_unsigned() explicitly; worth matching if a consumer starts multiplying this by a price.
Resolve conflicts with lemonade-sdk#3181 (min_total_chars/max_total_chars), which landed first: - routing_policy.h: keep both new Params fields (total_chars and expected_output_tokens) plus lemonade-sdk#3181 comment-column realignment. - test_routing_classifier_services.cpp: keep both sets of tests and both sets of main() registrations. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…olution Both sides of the conflict in test_routing_classifier_services.cpp ended mid-function, sharing the closing brace that sat below the conflict region. Reordering the halves left lemonade-sdk#3181's last test function unclosed, so every following definition parsed as nested. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Read the caller's max_tokens / max_completion_tokens (max_tokens wins if both are present) into RouteContext::Params::expected_output_tokens, a ceiling rather than an estimate, left unset when neither field is sent or the value is non-positive/non-integer.
No ranking logic changes yet: this is plumbing for a later token-weighted cost_select ranking (raised in PR #3100 review)