feat(routing): add cost_select deterministic cost-based router - #3100
feat(routing): add cost_select deterministic cost-based router#3100Bekhouche wants to merge 22 commits into
Conversation
Adds a new "cost" classifier and routing.router.type "cost_select" sugar, mirroring the existing "llm" router: ranks candidates by cost_input_per_million + cost_output_per_million and routes to the cheapest, falling back to the first-listed candidate when no candidate has cost data. Also fixes /routing/validate, which was never wiring CostServices into its engine.
There was a problem hiding this comment.
Overall, looks good, I just have a few minor improvement suggestions.
1. Avoid recomputing a request-independent decision on every request.
CostClassifier::evaluate ranks all candidates via cost_of on every request, but the result depends only on labels + the injected cost_services — not on ctx.request. On the hot path this is O(N) cost lookups per request that always return the same winner. Worse, attach_estimated_cost then calls cost_of an (N+1)th time for the winning route_to, duplicating a lookup the classifier just did. Compute the ranking once (memoize on first evaluate, or precompute at construction) and, if practical, reuse the winner's already-fetched CostInfo rather than re-invoking cost_of in attach_estimated_cost in routing_policy.cpp.
2. Fix the rationale number formatting.
result.rationale = "lowest estimated cost ($" + std::to_string(*best_score) + ... — std::to_string(double) emits fixed 6-decimal, locale-dependent output (e.g. "0.300000", or "0,300000" under some locales). Use an ostringstream with controlled precision (std::defaultfloat/setprecision). Also the "$X/M tokens" label is misleading: the value is input + output per-million summed, not a per-token cost — reword to something like "cheapest by input+output per-M sum".
3. Reconsider the equal-weight sum as the ranking metric.
Ranking by cost_input_per_million + cost_output_per_million hardcodes a 1:1 input/output token split, but that matches almost no real workload: across major providers output tokens are priced ~3–5× input (e.g. GPT-4o around $2.50/M in vs $10/M out, Claude Sonnet $3 vs $15, GPT-4o-mini $0.15 vs $0.60), while actual in:out ratios swing wildly — RAG/summarization runs input-heavy (thousands in, tens out) and chat/generation runs output-heavy — so the summed metric can rank the wrong model cheapest. The minimum correct fix is to score by expected per-request cost, weighting each price by a token estimate: use the real input size already in ctx.request.params.chars (÷4 for a byte→token proxy) and a documented default output length (e.g. 512), dropping the shared 1/1e6 factor since it doesn't affect ordering.
4. Guard against invalid price values.
compute_cost_score accepts any two doubles. A negative, NaN, or inf price (from a bad config entry or a flaky discovery response) would silently win as "cheapest" (-5.0 < everything), or poison comparisons. Add a std::isfinite(...) && value >= 0 check and treat invalid input as no-data (std::nullopt), consistent with the existing partial-data handling.
5. The no-data fallback bypasses default_model.
When no candidate is priced, the router falls back to candidates[0] rather than the configured default_model (the e2e test even asserts this). That's surprising: default_model exists precisely for the "engine can't decide" case, and here candidate ordering silently overrides it. Consider routing to default_model on total no-data, or make the fallback explicit/configurable — the current split ("no data → candidates[0]" vs. everywhere else "→ default_model") is an easy footgun.
Two smaller notes: the conformance corpus still has no stub-injection harness for cost (acknowledged in the diff, fine to defer), and log_cost_of_failure_once keeps an unbounded process-global std::set — harmless given the bounded candidate set, but worth a mental note.
Memoize the cost classifier's ranking (request-independent, so compute it once per engine instance instead of on every routed request), format the rationale's cost figure with locale-independent fixed precision instead of std::to_string(double), reject negative/NaN/infinite prices instead of letting them win as spuriously cheapest, and fail open to default_model (instead of candidates[0]) when no candidate has cost data, mirroring the llm classifier's fail-open contract.
Thanks for the review. Fixed most of it in the latest commit:
On the ranking metric: agree in principle, but we don't have an expected-output-token signal in RouteContext yet, so weighting now would just be a guess. I'll open two follow-up PRs for this: one to add that signal, one to add token-weighted ranking on top of it. |
|
Thank you for adding this!
2 options:
|
CostClassifier cached its ranking on the classifier object itself, which is shared via RoutePolicy::classifiers' shared_ptr across every request's engine for the policy's entire lifetime — so a price change (or a candidate initially missing price data) never took effect. Re-rank on every evaluate() instead; it's cheap because make_router_cost_services' price cache now invalidates on a real ModelManager registry-change generation bump rather than living until restart. Also fixes refresh_cloud_models/evict_cloud_models never calling notify_models_changed(), which made a price discovered via POST /v1/cloud/auth invisible to any generation-based consumer. Addresses review feedback from meghsat and sdevinenamd on PR lemonade-sdk#3100.
…he/lemonade into feat/routing-cost-select
|
Fixed at the root in 9da16f2 rather than documenting it as intended: |
ramkrishna2910
left a comment
There was a problem hiding this comment.
Picking this up alongside the other three routing PRs. The engineering here is solid — the desugaring is clean, test_routing_policy_cost_router.cpp is thorough (every fix that landed has a matching regression test, and the price-change test is a direct lock for the cache bug), the lock is refreshed correctly in the same diff, and the schema widening is genuinely back-compatible: I ran main's schema and this one against every combination of presence/absence/wrong-type for type/model/prompt in a router block and found zero divergences, because the if/then restores model+prompt for type: "llm". Replacing the "Phase A is reporting only" sentence with a Phase B section in the same diff is also the right handling — that contradiction is properly resolved.
Four things blocking.
1. cost_select can never select a free local model — "route to the cheapest" routes to a paid cloud model. compute_cost_score returns nullopt unless both per-million fields resolve. server_models.json contains zero occurrences of cost_input_per_million, cost_output_per_million, or cost_tier, so cloud auto-discovery is the only populator. Concretely, with candidates ["Qwen3-8B-GGUF", "openrouter.gpt-4o-mini", "openrouter.gpt-4o"] and default_model: "Qwen3-8B-GGUF": the GGUF costs the user nothing, is listed first, and is the default — and is excluded from ranking entirely for want of price metadata. gpt-4o-mini wins every request, and every request bills the user's cloud account. That is the inversion of the feature's intent in a product whose premise is local inference. Compounding it, cost_tier: "free" already exists in the documented vocabulary and compute_cost_score ignores it, so an author who explicitly marks a local model free still gets it excluded. Minimum fix: treat a resolved cost_tier: "free" as score 0.0. Otherwise router-policy.md needs to say plainly that unpriced candidates are never selectable, rather than the current softer "excluded from ranking rather than treated as free".
2. This freezes a ranking metric you have already committed to changing. The new frozen row pins the metric, and the README text says the metric, tie-break, and no-data fallback "are frozen v1 behavior ... changing any of them later needs a new major". But the plan of record is a follow-up adding token-weighted ranking on top of #3163's expected_output_tokens — which is now approved and queued. Under this PR's own evolution rule, that follow-up would need schema v2. Please either add the row without the ranking metric (freeze only the tie-break and the no-data fallback, and mark the metric provisional pending the weighting work), or land the weighted metric here. As written the two commitments contradict each other.
3. /v1/routing/validate + cost_select is a new amplification vector. The handler uses an identity component resolver — "accept any candidate/component name as-is" — and this PR wires CostServices into it. routing.candidates has no upper bound at parse time, CostClassifier::evaluate calls cost_of once per candidate per evaluation, and the price memo is a process-global static std::map<std::string, CostInfo> keyed by the caller-supplied name, cleared only on a registry-generation bump the caller cannot trigger. So one POST with a large list of unique random candidate names drives that many registry misses — each falling through to a full read and JSON parse of user_models.json — and permanently inserts that many cache entries. Neither half was reachable on main, where validate had no CostServices and candidates resolved against the live registry. The endpoint is unauthenticated unless LEMONADE_API_KEY is set. Please cap routing.candidates at parse time and bound the cache; fixing the memory alone leaves the disk amplification.
4. Cloud pricing becomes a routing-control input while the docs still disclaim it. parse_cloud_cost reads pricing.prompt/pricing.completion verbatim from the provider response, accepting a number or a string via std::stod, filtered only by > 0. A provider returning 1e-30 yields a finite, positive value that is the argmin against every honestly-priced candidate and captures all traffic from any cost_select policy listing it; with allow_insecure_http enabled, a network MITM controls routing. Credit where it is due — the isfinite guard added for review item 4 catches the overflow direction (std::stod("inf") returns HUGE_VAL and passes > 0), so it is load-bearing well beyond the config-typo case it was requested for. It is the underflow direction that is open. A sanity floor plus an explicit statement of the new trust level would cover it. Related: this PR edits cloud.md to say a policy "can also route automatically to the cheapest candidate using these same numbers" in a sentence that still calls them "illustrative, not a billing figure" — a number cannot be both non-authoritative and the authoritative routing input.
On the earlier review round, so nothing gets lost: items 2, 4, and 5 are fixed and tested. Item 1 is not satisfied at head — 85383f7 added exactly the memoization requested, then e2b719a removed it after the classifier-level memo was shown to freeze the winner for the policy's lifetime. That is the right call technically, but it means the original request is now deliberately un-satisfied without the requesting reviewer being looped back in, so it should be re-litigated openly rather than left as a resolved thread. Item 1b was never addressed: attach_estimated_cost still re-invokes cost_of for the winner, so it remains N+1 lookups per request. Separately, @sdevinenamd's inline note that it "only ranks once, at first request" is now obsolete — e2b719a made it rank every time, so the line objected to is correct as written; worth closing that thread explicitly since it currently reads as ignored.
Smaller things, none blocking:
compute_cost_scorevalidates both operands but not the sum — two finite near-DBL_MAXprices sum to+inf, which passes and renders as"$inf"in the rationale.- The frozen row says the classifier "fails open" on total no-data, but it returns
ok = truewith no labels, which is not theon_errorpath. Since the text is normative on merge, "returns no winning label" would be clearer. notify_models_changedbumps the generation after itsif (!cb) return;, so the whole cache-invalidation premise rests on a callback being registered. One is, unconditionally, so it is not a live bug — but moving the increment above the early return would make it robust rather than coincidental.- The two new
notify_models_changed()calls mean every/v1/cloud/auth, install, refresh, and eviction now also triggersreconcile_routing_helpers— a wider blast radius than "invalidate the price cache", and worth a line in the PR description. - The tie-break is bit-exact IEEE equality, so
0.1+0.2and0.15+0.15are not equal; two candidates an author considers equally priced resolve by floating-point sum, not by listing order. Worth a note since it is frozen. (The tie-break itself is sound — forward scan with strict<, no sort whose stability could be mis-relied on, and NaN cannot reach the comparator.) - On the corpus: the stated justification (no stub-injection harness for model-backed groups) is honest and matches the
l0a/l2/l3precedent, but thecostclassifier is deterministic — it needs only aCostServiceshook, not a model stub. That is why I would rather not call these semantics "frozen" while nothing replays them, which ties back to point 2.
I checked the rest and found nothing: no leak of losing candidates' prices into the trace (the rationale carries only the winner's aggregate and is gated on the winning label, and Phase A already publishes exact per-million figures in outputs.estimated_cost ungated, so this is strictly less information), no integer overflow, no injection, and sound exception safety.
…onade-sdk#3163) * feat(routing): add expected_output_tokens signal to RouteContext 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. * fix(routing): read max_output_tokens for /v1/responses' expected_output_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. * fix: close test function left open by the lemonade-sdk#3181 merge resolution 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> --------- Co-authored-by: ramkrishna2910 <ramkrishna2910@gmail.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
… items) - cost_tier: "free" now scores as the cheapest (0.0), short-circuiting per-million fields entirely — previously a free local model could never beat a priced cloud one, since compute_cost_score required both per-million fields and ignored cost_tier outright. - Split the frozen-semantics table: tie-break and no-data fallback stay frozen v1; the ranking metric itself is now explicitly provisional, since the plan of record (token-weighted ranking) would otherwise contradict this PR's own "no change without a major" rule. - Capped routing.candidates at 64 in the parser itself (not just schema maxItems), since /v1/routing/validate calls the parser directly with an identity resolver and has no registered-model count to bound it otherwise. Also bounded the price cache at 4096 entries so a stream of unique candidate names can't grow it without limit. - Added a $0.000001/M sanity floor on cloud-reported prices, so a spoofed or underflowed near-zero value can't always win the ranking; reworded cloud.md's contradictory "illustrative / not billing" + "routes automatically" framing to state plainly that cost_select uses the number as its actual routing input. Also: sum-overflow guard, "fails open" wording fix (not the on_error path), notify_models_changed's generation bump moved above its early-return so it's robust rather than coincidentally safe, and a float-tie-break note in the frozen table. Merged main; the only real conflict was schema-lock.json, resolved via test_schema_lock.py --update rather than a hand merge.
kMaxCachedCandidates was a lambda-local constexpr in make_router_cost_services; GCC/Clang allow using a compile-time constant inside a lambda without capturing it, but MSVC doesn't (error C3493). Moved it to file scope in an anonymous namespace instead, where no capture is needed on any compiler. Caught by the three failing PR checks (Build Lemonade Server Installer, Build Embeddable Lemonade (Windows), and the Inference backend tests gate that cascaded from them) — all three were the same MSVC compile error, since prior verification here was WSL/GCC only. Confirmed fixed with a native MSVC (VS 2026) build, plus a full WSL/GCC cpp-ci rerun (61/61) to make sure nothing else regressed.
Conflict in docs/dev/router-policy.md: main added a paragraph on the `llm` router receiving has_tools/has_images, and this branch added the cost_select section, both immediately after the LLM-as-router text. Kept both — main's paragraph stays with the LLM router it describes, and the cost_select section follows it.
|
@Bekhouche I was wondering we don't even need to call the notify_models_changed for refreshed costs as it is heavy and because of it cloud-provider auth/refresh now triggers a router reconcile pass, even on servers with no router collections configured at all. Instead we could re-use the next_notify_generation to make the price cache observe the change. This way we don't have to make any changes to the Model Manager. |
…oncile Refreshing or evicting a provider's cloud models called notify_models_changed(), whose callback reconciles routing helpers across the whole registry via get_supported_models(). Every cloud auth or refresh paid that pass, including on servers with no router collection at all, where the helper set is always empty. Bump the registry generation instead. The price cache already reads current_notify_generation() on each lookup, so it still drops stale prices the moment a catalog refresh lands, while the reconcile stays for the paths that actually change a router collection -- the same rule register_user_model and delete_model already follow. Addresses sdevinenamd's review comment.
|
@sdevinenamd done as you suggested, next_notify_generation() in refresh_cloud_models and evict_cloud_models instead of notify_models_changed(), pushed in e315aad. |
|
Thank you for addressing all the feedback @Bekhouche :) So was wondering if we can include the price cache rebuild inside routing_classifier_services_router.cpp/make_router_cost_services using a hardcoded periodic TTL check whenever you call the make_router_cost_services? |
@sdevinenamd I'd rather keep it exact. A 30s TTL means that a price discovered via
Your point about coupling is fair, though. The Router already has a |
Adds a new "cost" classifier and routing.router.type "cost_select" sugar, mirroring the existing "llm" router: ranks candidates by cost_input_per_million + cost_output_per_million and routes to the cheapest, falling back to the first-listed candidate when no candidate has cost data. Also fixes /routing/validate, which was never wiring CostServices into its engine.
Related to #3078 — that issue's rate-limit/budget-enforcement proposal explicitly builds on top of
cost_selectfor soft-enforcement (rerouting to cheaper candidates once a budget is exceeded). This PR is the selection mechanism; usage accounting and enforcement are not part of this change.