fix(router): don't log 'Could not identify azure model' when the deployment name resolves from the cost map - #37869
Conversation
…oyment name resolves from the cost map get_router_model_info already falls back to resolving the azure deployment's model name against the model cost map when base_model is unset — and for deployments named after real azure models (e.g. azure/gpt-4o) that resolution returns correct max tokens and costs. The unconditional ERROR was therefore spurious for exactly the deployments that need no operator action, and on busy proxies it logs thousands of times per day per multi-deployment group. Log at debug when the fallback entry carries usable limits/costs (membership alone is not enough: Router init auto-registers every deployment name as a zeroed stub), keep the ERROR otherwise. Fixes #33172 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ack gate Review follow-up: token-limit fields used 'is not None' while the cost field used '> 0' — a cost-map entry explicitly storing 0 limits could suppress the error log without carrying usable resolution data. All three checks now require a positive value. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@greptileai can you reply here with a score and why that score? |
|
bugbot run |
|
Score: 4/5 What the PR does well:
One minor concern keeping it from 5/5: The inline comments in That's a style nit, not a correctness issue. The fix itself is correct and the tests verify all paths. |
Greptile SummaryThe PR suppresses an Azure model-identification error when the deployment name resolves to a cost-map entry with usable limits or pricing, while preserving the error for zeroed registration stubs
Confidence Score: 4/5The PR appears safe to merge after addressing two non-blocking maintainability issues in the implementation comments and test fixture The Azure fallback remains aligned with downstream cost-map resolution, and the accepted concerns are limited to redundant comments and avoidable global-state mutation in tests Files Needing Attention: litellm/router.py, tests/test_litellm/test_router.py
|
| Filename | Overview |
|---|---|
| litellm/router.py | The fallback logging behavior is narrowly corrected, but the added explanatory block violates the repository's concise-comment policy |
| tests/test_litellm/test_router.py | The tests cover the intended logging cases, but their local fixture directly mutates the global cost map instead of reusing isolated fixture infrastructure |
Reviews (1): Last reviewed commit: "fix(router): use consistent positive che..." | Re-trigger Greptile
| # the `if model is None` fallback below resolves the deployment's | ||
| # model name against the model cost map — when the name is a known | ||
| # azure key (e.g. deployment model "azure/gpt-4o"), that resolution | ||
| # gives correct max tokens / costs and there is nothing for the | ||
| # operator to fix, so don't spam an ERROR on every request. | ||
| # membership alone isn't enough: Router init auto-registers every | ||
| # deployment name into litellm.model_cost as a zeroed stub, so | ||
| # require the entry to carry usable limits/costs. |
There was a problem hiding this comment.
Redundant fallback explanation
This comment restates the fallback lookup and positive-field check immediately below, adding maintenance overhead if the implementation changes
| # the `if model is None` fallback below resolves the deployment's | |
| # model name against the model cost map — when the name is a known | |
| # azure key (e.g. deployment model "azure/gpt-4o"), that resolution | |
| # gives correct max tokens / costs and there is nothing for the | |
| # operator to fix, so don't spam an ERROR on every request. | |
| # membership alone isn't enough: Router init auto-registers every | |
| # deployment name into litellm.model_cost as a zeroed stub, so | |
| # require the entry to carry usable limits/costs. |
Context Used: CLAUDE.md (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!
There was a problem hiding this comment.
Trimmed to just the zeroed-stub note in 419d306
| def _use_local_model_cost_map(self, monkeypatch): | ||
| monkeypatch.setenv("LITELLM_LOCAL_MODEL_COST_MAP", "True") | ||
| original_model_cost = litellm.model_cost |
There was a problem hiding this comment.
Direct global cost-map mutation
This fixture directly reassigns litellm.model_cost, adding global-state coupling and manual restoration instead of reusing the repository's isolated cost-map fixture and cache handling
Context Used: CLAUDE.md (source)
Knowledge Base Used: Roll back live Router membership between tests
There was a problem hiding this comment.
Switched to the shared local_model_cost_map fixture in 419d306, which also clears the get_model_info cache
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 43f6a5f. Configure here.
…al_model_cost_map fixture
|
Addressed both notes in 419d306. @greptileai can you reply here with a score and why that score? |
|
The implementation looks correct and addresses both previous notes. Here's the breakdown: Score: 5/5 Why:
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 419d306. Configure here.
…itellm_pr33292_azure_base_model_log
|
Merged latest litellm_internal_staging to pick up the test tree lint fix. @greptileai can you reply here with a score and why that score? |
|
Score: 4.5/5 What the change does: When an Azure deployment has no Why this is correct:
Minor flag (reason it's not 5/5): The Everything else is tight: no behavior change, no new DB queries in the critical path, no provider-specific code outside |
|
Keeping the stub guard note: the first review asked to retain exactly this comment, and the guard is genuinely non-obvious |
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 80b7d53. Configure here.
Note
Copy of #33292 by @mihidumh, pushed to a
litellm_branch so the full CircleCI suite runs. Commit authorship is preserved; full credit to the original author. The only difference from the original branch is that 476 regeneratedlitellm/proxy/_experimental/outbuild files that were accidentally swept into the second commit have been dropped, and the base has been updated to latestlitellm_internal_staging(one trivial append conflict intests/test_litellm/test_router.pyresolved by keeping both new test classes)Relevant issues
Fixes #33172
Original PR: #33292
Linear ticket
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
@greptileaito re-request a review after pushing changes)Delays in PR merge?
Screenshots / Proof of Fix
Production observation by the original author (litellm proxy 1.92.0 on Azure, real traffic): every request to a multi-deployment azure group without
base_modellogsthousands of ERROR lines a day, even though
azure/gpt-5.4is an exact key in the shipped cost map andget_router_model_info's existingif model is None: model = _modelfallback resolves it correctly. The original author verified on a live proxy that hand-settingmodel_info.base_model = litellm_params.modelon every such row (56 rows on one env) changes nothing about resolution and only silences the log; the ERROR was spurious for exactly the deployments that need no operator actionMembership in
litellm.model_costcannot be the discriminator on its own:Router.__init__auto-registers every deployment name into the map as a zeroed stub (register_model: model=... not in built-in cost map ... will default to 0). The check therefore requires the fallback entry to carry usable limits/costsUnit tests (new
TestAzureBaseModelFallbackLoggingintests/test_litellm/test_router.py):Before (base):
test_map_known_deployment_name_resolves_without_error_logFAILS (spurious ERROR fires); the other 2 passAfter (this branch):
covering: (1) map-known deployment name resolves with no ERROR and model_info carries the map's max tokens/costs, (2) genuinely unmappable name still logs the ERROR and returns the zeroed stub unchanged, (3) explicit
base_modelstill winsType
🐛 Bug Fix
Changes
get_router_model_info's azure branch: whenbase_modelis unset, check whether theazure/<deployment model name>entry in the cost map carries usable limits/costs (guarding against Router-init's zeroed auto-registration stubs). If so, log at debug; the existing fallback resolution handles it. Otherwise keep the existing errorNote
Low Risk
Logging-only change in Azure model-info lookup; cost/token resolution behavior is unchanged.
Overview
Stops
get_router_model_infofrom ERROR-logging “Could not identify azure model” on every request when an Azure deployment has nobase_modelbut the name already maps to a real cost-map entry (usable max tokens or input cost). Those cases now log at debug; genuinely unmappable names still ERROR.Resolution and returned
model_infoare unchanged. Router-init zeroed stubs are ignored so membership inlitellm.model_costalone is not treated as a hit. Tests cover known names, unmappable names, and explicitbase_model.Reviewed by Cursor Bugbot for commit 80b7d53. Bugbot is set up for automated code reviews on this repo. Configure here.