Revert "feat: add model_cost aliases expansion support" - #23313
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Greptile SummaryThis PR reverts #21601 ("feat: add model_cost aliases expansion support"), but the revert is not a clean rollback — it includes an additional batch of unrelated changes that introduce several critical regressions on top of removing the alias feature. Key issues introduced by this revert:
Confidence Score: 1/5
|
| Filename | Overview |
|---|---|
| litellm/litellm_core_utils/duration_parser.py | Reverts correct modular month arithmetic — target_month = current_time.month + value raises ValueError for any value > 1 between months January–November when the sum exceeds 12. |
| litellm/router_strategy/lowest_latency.py | TTFT latency averaging now divides by len(item_latency) instead of len(item_ttft_latency), producing incorrect averages and a potential ZeroDivisionError for streaming-only deployments. |
| litellm/proxy/auth/model_checks.py | Removes defensive list copies — direct references to cached user_api_key_dict.models and proxy_model_list are mutated by _get_models_from_access_groups, silently expanding authorization on subsequent requests. Deduplication also removed. |
| litellm/caching/dual_cache.py | Removes automatic application of default_in_memory_ttl in async_set_cache and async_set_cache_pipeline — entries will now never expire in memory when no explicit TTL is passed. |
| litellm/router.py | Removes fast-fail for non-retryable errors (e.g. 400 ContextWindowExceeded) and latest-error tracking — non-retryable errors now waste all retry attempts, and the original (first) error is raised instead of the most recent one. |
| litellm/llms/vertex_ai/gemini/transformation.py | Removes filter for LiteLLM-internal extra_body keys (cache, tags) — these are now forwarded directly to Vertex AI, causing 400 errors on any request using LiteLLM caching or tagging with Vertex AI. |
| litellm/llms/fireworks_ai/chat/transformation.py | Reverts fix for double /v1 in the Fireworks model listing URL — when api_base already ends with /v1, the constructed URL will contain /v1/v1/, producing 404s. |
| litellm/llms/sagemaker/completion/handler.py | Reverts to basic boto3.client construction, losing support for AWS role assumption (aws_role_name/aws_session_name) in SageMaker embeddings. |
| litellm/llms/openai/chat/gpt_5_transformation.py | Removes _get_effort_level helper and is_model_gpt_5_4_plus_model — dict-format reasoning_effort inputs (e.g. {"effort": "none", "summary": "detailed"}) are no longer normalised to a string before equality comparisons, which may break edge-case tool/sampling guards. |
| litellm/llms/bedrock/chat/converse_transformation.py | Removes completion_tokens_details (including reasoning_tokens) from Bedrock usage and drops the output_config key cleanup — minor regressions in usage reporting and parameter handling. |
| litellm/proxy/management_endpoints/team_endpoints.py | Introduces _unfurl_all_proxy_models helper but its call site is immediately commented out, leaving dead code. |
| litellm/litellm_core_utils/get_model_cost_map.py | Removes _expand_model_aliases and its call sites — model cost alias expansion is no longer performed at load time; the aliases key in model_prices_and_context_window.json will have no effect. |
| litellm/responses/litellm_completion_transformation/transformation.py | Removes consecutive-assistant-message merging logic and convert_apply_patch_tool_call_to_chat_completion_tool_call — Anthropic tool-use multi-turn sessions may regress; whitespace-only cleanup for the rest. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Incoming Request] --> B{Router retry loop}
B -->|attempt| C[make_call]
C -->|success| D[Return response]
C -->|error| E{Error type?}
subgraph BEFORE["Before revert (feature PR)"]
E -->|non-retryable 400/404| F[Raise immediately]
E -->|retryable 429/500| G[Update original_exception to latest error]
G --> H[Sleep + retry]
H --> B
B -->|retries exhausted| I[Raise latest error]
end
subgraph AFTER["After revert (this PR)"]
E2{Error type?} -->|ANY error| G2[Continue retry loop]
G2 --> H2[Sleep + retry]
H2 --> B2[Router retry loop]
B2 -->|retries exhausted| I2[Raise FIRST error seen]
end
style F fill:#90EE90
style I fill:#90EE90
style G2 fill:#FF6B6B
style I2 fill:#FF6B6B
Comments Outside Diff (1)
-
litellm/router.py, line 5484-5510 (link)Non-retryable errors are no longer raised immediately; latest error is no longer tracked
The revert removes two improvements from the retry loop:
-
Non-retryable errors continue to retry unnecessarily. The removed
should_retry_this_errorcheck meant that errors likeContextWindowExceededError(400) are now retried the fullnum_retriescount rather than failing fast. This wastes time, consumes quota on other healthy deployments, and contradicts the documented behaviour. -
The first error is raised instead of the most recent one. The removed
original_exception = eupdate meant that when all retries are exhausted, the exception that ultimately propagates is the very first error encountered, not the last. For example, if attempt 1 raises a 429 and all subsequent retries raise a 400, the caller receives the 429 — potentially leading to incorrect retry logic on the client side.
These were explicit regression-fix behaviours with dedicated tests (
test_router_retry_non_retryable_errors.py) that this PR also deletes. -
Last reviewed commit: 3d2df7e
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if current_time.month == 12: | ||
| target_year = current_time.year + 1 | ||
| target_month = 1 | ||
| else: | ||
| target_year = current_time.year | ||
| target_month = current_time.month + value |
There was a problem hiding this comment.
Month overflow not handled for value > 1
The reverted code only handles overflow when current_time.month == 12, but does nothing when current_time.month + value > 12 for other months. For example, if today is October (month=10) and value=3, then target_month = 13, which is an invalid month and will raise a ValueError when the datetime constructor is called on line 81.
The code removed by this revert handled this correctly using modular arithmetic:
total_months = current_time.month - 1 + value # 0-indexed months
target_year = current_time.year + total_months // 12
target_month = total_months % 12 + 1 # back to 1-indexedWith the current reverted code, any duration string like "3mo" or "2mo" used between October–November will crash at runtime.
| for _call_latency in item_ttft_latency: | ||
| if isinstance(_call_latency, float): | ||
| total += _call_latency | ||
| item_latency = total / len(item_ttft_latency) | ||
| else: | ||
| for _call_latency in item_latency: | ||
| if isinstance(_call_latency, float): | ||
| total += _call_latency | ||
| item_latency = total / len(item_latency) | ||
| item_latency = total / len(item_latency) |
There was a problem hiding this comment.
Wrong denominator when averaging TTFT latency — potential ZeroDivisionError
When the streaming/TTFT branch is taken (lines 499–501), total is accumulated from item_ttft_latency, but the final division on line 506 always uses len(item_latency) as the divisor regardless of which branch was taken.
This has two problems:
- Wrong result: the average is computed over
item_ttft_latencyvalues but divided by a count from a different list, producing an incorrect latency score. - ZeroDivisionError: if
item_latencyis empty butitem_ttft_latencyis non-empty (a perfectly valid state for a streaming-only endpoint),len(item_latency)is0and the division crashes.
The feature PR correctly split this into two branches, each using its own len(...):
if use_ttft:
...
item_latency = total / len(item_ttft_latency)
else:
...
item_latency = total / len(item_latency)| if len(user_api_key_dict.models) > 0: | ||
| all_models = list(user_api_key_dict.models) # copy to avoid mutating cached objects | ||
| all_models = user_api_key_dict.models | ||
| if SpecialModelNames.all_team_models.value in all_models: | ||
| all_models = list(user_api_key_dict.team_models) # copy to avoid mutating cached objects | ||
| all_models = user_api_key_dict.team_models | ||
| if SpecialModelNames.all_proxy_models.value in all_models: | ||
| all_models = list(proxy_model_list) # copy to avoid mutating caller's list | ||
| if include_model_access_groups: | ||
| all_models.extend(model_access_groups.keys()) | ||
| all_models = proxy_model_list | ||
|
|
||
| all_models = _get_models_from_access_groups( | ||
| model_access_groups=model_access_groups, | ||
| all_models=all_models, | ||
| include_model_access_groups=include_model_access_groups, | ||
| model_access_groups=model_access_groups, all_models=all_models | ||
| ) | ||
|
|
||
| # deduplicate while preserving order | ||
| all_models = list(dict.fromkeys(all_models)) | ||
|
|
||
| verbose_proxy_logger.debug("ALL KEY MODELS - {}".format(len(all_models))) |
There was a problem hiding this comment.
Direct list references can silently corrupt cached UserAPIKeyAuth objects
The reverted code assigns all_models directly to user_api_key_dict.models, user_api_key_dict.team_models, and proxy_model_list without copying:
all_models = user_api_key_dict.models # direct reference
all_models = user_api_key_dict.team_models
all_models = proxy_model_listThese objects are typically shared cached values. When _get_models_from_access_groups appends to all_models (line 117), it mutates the original cached list. The same object is used on the next request, now containing extra model entries from the previous call's access group expansion. This can cause authorization to silently expand over time — users gaining access to models they shouldn't have.
The feature PR added defensive copies (list(...)) to prevent this mutation, which this revert removes.
| ) | ||
| try: | ||
| if self.in_memory_cache is not None: | ||
| if "ttl" not in kwargs and self.default_in_memory_ttl is not None: | ||
| kwargs["ttl"] = self.default_in_memory_ttl | ||
| await self.in_memory_cache.async_set_cache(key, value, **kwargs) | ||
|
|
||
| if self.redis_cache is not None and local_only is False: |
There was a problem hiding this comment.
default_in_memory_ttl no longer applied in async_set_cache and async_set_cache_pipeline
The revert removes these lines from both async_set_cache (line 346) and async_set_cache_pipeline (line 367):
if "ttl" not in kwargs and self.default_in_memory_ttl is not None:
kwargs["ttl"] = self.default_in_memory_ttlAny caller that relies on DualCache.default_in_memory_ttl being automatically applied when no explicit TTL is passed will now store entries in the in-memory cache without a TTL, causing them to never expire. This is a silent behavioral regression — entries that should have been evicted after the default TTL will now persist indefinitely in memory.
| raise e | ||
|
|
||
|
|
||
| # Keys that LiteLLM consumes internally and must never be forwarded to the | ||
| _LITELLM_INTERNAL_EXTRA_BODY_KEYS: frozenset = frozenset({"cache", "tags"}) | ||
|
|
||
|
|
||
| def _pop_and_merge_extra_body(data: RequestBody, optional_params: dict) -> None: | ||
| """Pop extra_body from optional_params and shallow-merge into data, deep-merging dict values.""" | ||
| extra_body: Optional[dict] = optional_params.pop("extra_body", None) | ||
| if extra_body is not None: | ||
| data_dict: dict = data # type: ignore[assignment] | ||
| for k, v in extra_body.items(): |
There was a problem hiding this comment.
Internal LiteLLM keys (cache, tags) forwarded to Vertex AI
The revert removes the filter that prevented LiteLLM-internal extra_body keys from being sent to the Vertex AI API:
_LITELLM_INTERNAL_EXTRA_BODY_KEYS: frozenset = frozenset({"cache", "tags"})
...
if k in _LITELLM_INTERNAL_EXTRA_BODY_KEYS:
continueWithout this filter, LiteLLM-internal keys like cache and tags passed via extra_body will now be merged directly into the Vertex AI request body. Vertex AI does not recognise these keys and will return a 400 Bad Request / InvalidArgument error, breaking all requests that use LiteLLM's caching or tagging features with Vertex AI.
| ) | ||
|
|
||
| base = api_base.rstrip("/") | ||
| if base.endswith("/v1"): | ||
| base = base[: -len("/v1")] | ||
| response = litellm.module_level_client.get( | ||
| url=f"{base}/v1/accounts/{account_id}/models", | ||
| url=f"{api_base}/v1/accounts/{account_id}/models", | ||
| headers={"Authorization": f"Bearer {api_key}"}, | ||
| ) |
There was a problem hiding this comment.
Double /v1 in Fireworks model listing URL when api_base ends with /v1
The reverted code constructs the URL as:
url=f"{api_base}/v1/accounts/{account_id}/models"If api_base is already "https://api.fireworks.ai/inference/v1" (the default), this produces:
https://api.fireworks.ai/inference/v1/v1/accounts/{account_id}/models
which is a 404. The feature PR fixed this by stripping the trailing /v1 from api_base before appending /v1/accounts/...:
base = api_base.rstrip("/")
if base.endswith("/v1"):
base = base[: -len("/v1")]
url = f"{base}/v1/accounts/{account_id}/models"The revert re-introduces this URL construction bug.
| def _unfurl_all_proxy_models( | ||
| team_info: LiteLLM_TeamTable, llm_router: Router | ||
| ) -> LiteLLM_TeamTable: | ||
| if ( | ||
| SpecialModelNames.all_proxy_models.value in team_info.models | ||
| and llm_router is not None | ||
| ): | ||
| team_models: set[str] = set() # make set to avoid duplicates | ||
| for model in team_info.models: | ||
| if model != SpecialModelNames.all_proxy_models.value: | ||
| team_models.add(model) | ||
| for model in llm_router.get_model_names(): | ||
| team_models.add(model) | ||
| team_info.models = list(team_models) | ||
| return team_info | ||
|
|
||
|
|
There was a problem hiding this comment.
Dead code: _unfurl_all_proxy_models is defined but never called
The function _unfurl_all_proxy_models is introduced by this PR but its only call site at line 2973 is commented out:
# ## UNFURL 'all-proxy-models' into the team_info.models list ##
# if llm_router is not None:
# _team_info = _unfurl_all_proxy_models(_team_info, llm_router)The unused function adds noise and may cause confusion about what is and isn't active. Consider either removing the function or uncommenting the call if the behaviour is intentional.
| ### BOTO3 INIT | ||
| import boto3 | ||
|
|
||
| # Use _load_credentials to support role assumption (aws_role_name, aws_session_name) | ||
| credentials, aws_region_name = self._load_credentials(optional_params) | ||
| # pop aws_secret_access_key, aws_access_key_id, aws_region_name from kwargs, since completion calls fail with them | ||
| aws_secret_access_key = optional_params.pop("aws_secret_access_key", None) | ||
| aws_access_key_id = optional_params.pop("aws_access_key_id", None) | ||
| aws_region_name = optional_params.pop("aws_region_name", None) | ||
|
|
||
| # Create boto3 session with the loaded credentials | ||
| session = boto3.Session( | ||
| aws_access_key_id=credentials.access_key, | ||
| aws_secret_access_key=credentials.secret_key, | ||
| aws_session_token=credentials.token, | ||
| region_name=aws_region_name, | ||
| ) | ||
| client = session.client(service_name="sagemaker-runtime") | ||
| if aws_access_key_id is not None: | ||
| # uses auth params passed to completion | ||
| # aws_access_key_id is not None, assume user is trying to auth using litellm.completion | ||
| client = boto3.client( | ||
| service_name="sagemaker-runtime", | ||
| aws_access_key_id=aws_access_key_id, | ||
| aws_secret_access_key=aws_secret_access_key, | ||
| region_name=aws_region_name, | ||
| ) | ||
| else: | ||
| # aws_access_key_id is None, assume user is trying to auth using env variables | ||
| # boto3 automaticaly reads env variables | ||
|
|
||
| # we need to read region name from env | ||
| # I assume majority of users use .env for auth | ||
| region_name = ( | ||
| get_secret("AWS_REGION_NAME") | ||
| or aws_region_name # get region from config file if specified | ||
| or "us-west-2" # default to us-west-2 if region not specified | ||
| ) | ||
| client = boto3.client( | ||
| service_name="sagemaker-runtime", | ||
| region_name=region_name, | ||
| ) | ||
|
|
||
| # pop streaming if it's in the optional params as 'stream' raises an error with sagemaker | ||
| inference_params = deepcopy(optional_params) |
There was a problem hiding this comment.
SageMaker embedding role assumption no longer supported
The feature PR replaced the manual credential construction pattern with _load_credentials, which is the shared helper that handles aws_role_name / aws_session_name cross-account role assumption. The reverted code builds a plain boto3.client directly from static credentials or environment variables, bypassing the role assumption path entirely.
Users who previously relied on role assumption for SageMaker embeddings will silently fall back to their environment credentials instead of assuming the configured role, potentially causing AccessDenied errors or billing on the wrong account. This is a functional regression for that use case.
…-cost-aliases Revert "feat: add model_cost aliases expansion support"
1 participant
Reverts #21601