fix: cast deployment costs to float in cost-based routing - #33979
fix: cast deployment costs to float in cost-based routing#33979devYRPauli wants to merge 2 commits into
Conversation
Greptile SummaryThis PR fixes a type-safety bug in
Confidence Score: 4/5Safe to merge — the logic change is minimal, correct, and consistent with the same pattern used elsewhere in the router strategy layer. The fix is straightforward and well-scoped: two identical try/except float() blocks that match existing patterns in sibling files. The only gap is that no automated test was committed to the repo, so the two failure modes described in the PR (mixed-type crash and silent string-concatenation) are not enforced by CI after this merges.
|
| Filename | Overview |
|---|---|
| litellm/router_strategy/lowest_cost.py | Wraps input_cost_per_token and output_cost_per_token lookups in float() with a try/except fallback to None, preventing both TypeError (mixed str/float comparisons) and silent string-concatenation bugs when PyYAML parses bare scientific-notation costs as strings. |
Comments Outside Diff (1)
-
litellm/router_strategy/lowest_cost.py, line 255-277 (link)No test committed despite checklist claiming tests were added
The PR's pre-submission checklist has "I have added meaningful tests" checked, but the diff contains no new or modified test files. The "Proof of fix" section in the description shows a manual local run, not a committed automated test.
tests/local_testing/test_lowest_cost_routing.pyhas no case covering YAML-sourced string costs, so regression coverage for both failure modes described (mixed-typeTypeErrorand silent string-concatenation) will not be enforced by CI going forward.Rule Used: What: Ensure that any PR claiming to fix an issue ... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Reviews (1): Last reviewed commit: "Cast deployment costs to float in cost-b..." | Re-trigger Greptile
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
f3b831a to
7dfc56c
Compare
7dfc56c to
81feaeb
Compare
|
Rebased onto This PR was based on I cherry-picked the commit onto current The change itself is unchanged, and I re-checked that the bug is still present on staging before pushing. |
PyYAML only resolves scientific notation to a float when a decimal point is present, so input_cost_per_token: 1e-06 in a proxy config parses as a string while 1.0e-06 parses as a float. lowest_cost.py summed those values without casting. With one string and one float cost the router raised TypeError while sorting candidate deployments, and since async_get_available_deployments does not catch it and the router re-raises, the caller's completion request failed. When every cost parsed as a string the sum silently concatenated instead, so deployments were ordered by string comparison rather than cost. Cast both values with a guarded float() and fall back to the existing model_cost defaults, matching quality_router and complexity_router. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
81feaeb to
c5c1985
Compare
Relevant issues
Related to #32787, which made the same observation ("cost values read from model_info should always be cast to float"). The fix for that issue (#33556) was scoped to
router.py:_set_model_group_info;router_strategy/lowest_cost.pywas not covered and still sums the raw values.Pre-Submission checklist
standalone reproduction against the real routing path, not a unit test)
What is the problem
PyYAML only resolves scientific notation to a float when a decimal point is present:
So
input_cost_per_token: 1e-06in a proxy config arrives as astr, while1.0e-06arrives as afloat. Both are natural things to write.lowest_cost.pythen doesitem_cost = item_input_cost + item_output_costwith no cast.Two failure modes follow:
One deployment with a bare mantissa cost and another with a normal float cost. The costs are compared while sorting candidate deployments and raise
TypeError: '<' not supported between instances of 'float' and 'str'.async_get_available_deploymentsdoes not catch this and the router re-raises, so the caller's completion request fails, not just the routing decision.Every deployment cost parsed as a string. There is no exception, but
+concatenates instead of adding, so"1e-06" + "1e-06"becomes"1e-061e-06"and deployments are ordered by string comparison rather than by cost. Routing silently picks the wrong deployment.The change
Wrap both values in a guarded
float()and fall back to the existingmodel_costdefaults on failure, so the fallback behaviour and the selection semantics are unchanged. This matches the handling already used inrouter_strategy/quality_router.py(_get_deployment_input_cost) androuter_strategy/complexity_router.py.Proof of fix
Exercised the real
LowestCostLoggingHandler.async_get_available_deploymentswith two deployments whose costs came fromyaml.safe_load, so the string/float mix is produced the same way a proxy config produces it.Before (commit 51b0b4a2ca, parent of this change):
After (commit 8dfc8a5):