Repository navigation
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes router model ID resolution so routing group names and model group aliases correctly map to member deployment IDs, improving cooldown and retry-after behavior on 429 paths
Changes:
- Add
Router._resolve_to_deployment_model_names()to map served names (alias, routing group, direct model) to underlying deploymentmodel_names - Update
Router.get_model_ids()to resolve through that helper, enabling correct per-deployment cooldown lookups for groups - Add regression tests covering group and alias ID resolution plus correct cooldown propagation for fully cooled-down groups
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| litellm/router.py | Resolve routing-group and alias names to underlying deployment model names during get_model_ids() |
| tests/test_litellm/router_strategy/test_router_routing_groups.py | Add regression tests for group/alias get_model_ids() resolution and correct cooldown time reporting |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| The deployment `model_name`s behind a requested name: a | ||
| `model_group_alias` resolves to its target and a callable routing group | ||
| to its members, so lookups keyed by model group name reach the same | ||
| deployments the router would route the request to. Names that are | ||
| neither resolve to themselves. |
Greptile SummaryResolves routing-group names and aliases to their member deployment IDs so cooldown consumers can use deployment-specific values
Confidence Score: 4/5The team-scoped cooldown calculation should be corrected before merging because it can report a retry interval from an ineligible deployment Routing applies team eligibility while selecting group members, but the changed ID expansion discards that context before calculating the minimum cooldown Files Needing Attention: litellm/router.py
|
| Filename | Overview |
|---|---|
| litellm/router.py | Adds group-aware ID resolution, but the expansion includes deployments excluded from a team-scoped request and can report another team's cooldown |
| tests/test_litellm/router_strategy/test_router_routing_groups.py | Adds focused group and alias resolution tests plus cooldown regression coverage, but does not cover team-filtered routing groups |
Reviews (1): Last reviewed commit: "fix(router): resolve routing group names..." | Re-trigger Greptile
| for deployment_model_name in self._resolve_to_deployment_model_names(model=model_name): | ||
| for idx in self.model_name_to_deployment_indices.get(deployment_model_name) or (): |
There was a problem hiding this comment.
Cooldown lookup ignores team scope
When a team-scoped group is exhausted, this includes other teams' deployment cooldowns, causing an incorrect retry-after value.
Knowledge Base Used: Router: deployment selection, retries, fallbacks, and cooldowns
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Merging this PR will not alter performance
|
|
checked the P1 before changing anything. a team deployment is indexed under its own internal model_name and its public team name lives in a separate index, so resolving a group through the model_name index cant reach another teams ids. ran the same probe on base and on this branch. the plain model name gives ['shared-1'] on both. the group name gives [] on base and ['shared-1'] here. so team scope was already not consulted at that call site and this doesnt change it, it only stops the group name resolving to nothing. pushed a test pinning that, plus the docstring reword. |
|
Pushed
The commit calls it directly and pins each of the three branches its docstring documents: an alias resolves to its target, a routing group resolves to its member After the change: The other two red checks are not from this PR, which touches only
Happy to rebase again if you would rather see those cleared first. @krrishdholakia this has been waiting on a first human look since Aug 19 if you get a chance. |
|
Following up on my last comment: the coverage fix worked, but The job runs its steps in order. Two things say this is base-branch debt rather than mine:
That also explains why my other PR #37480 shows So the one red check this PR was responsible for is fixed, and what is left needs either the docs page updated for those 28 settings or the gate adjusted, both of which look like base-branch work rather than something to smuggle into a routing change. Happy to send a separate PR documenting them if that would help, just say the word. |
|
merged current main in ( |
Signed-off-by: pjdurden <prajjwalchittori1@gmail.com>
Clarify _resolve_to_deployment_model_names' docstring and cover the case a review flagged: a group name resolving through the model_name index cannot reach another team's deployment ids, since a team deployment is indexed under its own internal model_name. Signed-off-by: pjdurden <prajjwalchittori1@gmail.com>
The code-quality job runs tests/code_coverage_tests/router_code_coverage.py, which requires every function defined in router.py to be called from a test file with "router" in its name. _resolve_to_deployment_model_names is added by this PR and was only exercised indirectly through get_model_ids, so the gate failed with 0.27% of functions untested. Calls it directly instead, pinning each of the three branches its docstring documents: an alias resolves to its target, a routing group resolves to its member model_names in declaration order, and any other name resolves to itself, including an unknown name that is passed through for the caller's lookup to miss on. A second test covers the team-scoped case, where a deployment indexed under its internal model_name is never yielded by group resolution and is reached only under that name. router_code_coverage.py now reports untested_perc 0.0 and exits 0. Signed-off-by: pjdurden <prajjwalchittori1@gmail.com> Signed-off-by: pjdurden <adobearakap@gmail.com>
Issue #36373: addressing a whole group of differently-named models by one name
1. Root cause
The capability the issue asks for, one virtual model name in front of several deployments that
carry different
model_names, already exists in the current tree asrouter_settings.routing_groups:my-groupis callable, shows up in/v1/modelsand/model_group/info, unions the members'deployments, applies the group's own routing strategy, and cools members down on 429. I verified all
of that against a live proxy before changing anything.
What is still wrong is that a routing group name is not resolvable to the deployment IDs behind it.
Router.get_model_ids(model_name=...)only consultsmodel_name_to_deployment_indices, which iskeyed by concrete deployment
model_names, so a group name (and an alias pointing at one) returns[]. The plainmodel_group_aliascase happens to work only by accident: the request path rewritesmodelto the alias target before the error path runs, whereas a group call deliberately keeps thegroup name so strategy selectors key their state by it.
The user-visible consequence is on the 429 path. When every member of a group is cooling down,
async_raise_no_deployment_exception(and the three siblingRouterRateLimitErrorsites inrouter.py) callscooldown_cache.get_min_cooldown(model_ids=[]), which falls back to the router'sdefault cooldown instead of the members' actual cooldown. The proxy turns that number into the
retry-afterheader, so a client backs off for the wrong amount of time, 5s where the deploymentsasked for 120s in my repro. The same lookup is used by the pass-through cost-tracking handlers,
which fall back to the raw name for a group.
2. The fix
Router._resolve_to_deployment_model_names(model)maps any name the router serves to the concretedeployment
model_names behind it: amodel_group_aliasresolves to its target, a callable routinggroup to its members, anything else to itself.
get_model_idsnow iterates those names instead oflooking the requested name up directly.
That mirrors what
get_model_list,get_model_group_info,is_recognized_modelandrouting_group_has_alternativesalready do, so the group name behaves like a model group name atthis lookup too. It is one resolution helper rather than a fix at each of the six call sites, and it
is a no-op for ordinary model names (
_get_model_from_aliasandget_routing_groupboth returnNonefor them, andget_routing_groupdeliberately lets a real deployment name shadow asame-named group).
I did not add a second way to declare a group (for example, letting
model_group_aliastake a listof models).
routing_groupsalready covers the issue's use case, and a parallel mechanism would betwo sources of truth for the same thing.
3. Files changed
litellm/router.py: added_resolve_to_deployment_model_names, andget_model_idsnow resolvesthrough it.
tests/test_litellm/router_strategy/test_router_routing_groups.py: two regression tests, one onthe resolution itself and one end-to-end on the cooldown time reported for a fully cooled-down
group.
4. Risk / uncertainty
all. Callers that pass a real deployment
model_nameare byte-for-byte unchanged.get_model_ids()with nomodel_nameis untouched, so the/model/info-style callers thatenumerate every deployment are unaffected.
{"alias": {"model": "x", "hidden": true}}) now resolves here too.hiddenonlygoverns listing, so resolving it for an ID lookup is consistent with
_get_model_from_alias.group of differently-named models by one name", which
routing_groupsanswers. If they meantsomething narrower, this fix still stands on its own as a correctness bug on the 429 path.
5. How I verified it
Live proxy on
localhost:4121, no mocks in the routing path (the deployments mock the upstream 429so the cooldown is deterministic). Config:
srv-a+srv-bgrouped asmy-group, plus twodeployments sharing the name
bothas the control, all four withcooldown_time: 120.Before, third call (both members cooling down):
Control, same call against the same-named group
both:After the fix, the group matches the control:
Both new tests fail on the unpatched tree (
assert 5 == 120, and[] != ['deploy-1', 'deploy-2', 'deploy-3']) and pass with it; the fulltest_router_routing_groups.pyfile is green at 64 tests.For regressions across the
get_model_idsconsumers:tests/test_litellm/router_strategyis 929passed / 4 skipped, and
tests/test_litellm/test_router.pyplustests/test_litellm/router_utilsis 566 passed.
ruff checkis clean on both changed files (ruff formatwants to reflow lines thatwere already unformatted before this change, so I left them alone).