-
-
Notifications
You must be signed in to change notification settings - Fork 11.2k
fix(register_model): preserve built-in cache pricing when registering custom overrides under unmapped keys #30044
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
f5f5765
f841cd8
18615f2
503cd39
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2887,6 +2887,61 @@ def _convert_stringified_numbers(value): | |
| return value | ||
|
|
||
|
|
||
| _BEDROCK_REGION_PREFIXES = ( | ||
| "us.", | ||
| "eu.", | ||
| "apac.", | ||
| "jp.", | ||
| "au.", | ||
| "us-gov.", | ||
| "global.", | ||
| "ap-northeast-1.", | ||
| ) | ||
|
|
||
| _CACHE_PRICING_FIELDS = ( | ||
| "cache_creation_input_token_cost", | ||
| "cache_creation_input_token_cost_above_1hr", | ||
| "cache_creation_input_token_cost_above_200k_tokens", | ||
| "cache_read_input_token_cost", | ||
| "cache_read_input_token_cost_above_200k_tokens", | ||
| ) | ||
|
|
||
|
|
||
| def _resolve_builtin_model_cost_entry( | ||
| key: str, provider: str | ||
| ) -> Optional[Dict[str, Any]]: | ||
| """Best-effort lookup of a built-in ``model_cost`` entry for a custom key | ||
| whose shape ``get_model_info`` cannot resolve (double provider prefixes | ||
| like ``bedrock/bedrock/us.anthropic.claude-sonnet-4-6`` or region aliases). | ||
|
|
||
| Returns a copy of the matching entry so the caller can inherit its defaults | ||
| (most importantly cache pricing) without mutating the shared built-in. | ||
| Returns ``None`` when no safe match exists. | ||
| """ | ||
| candidates: List[str] = [] | ||
| segments = key.split("/") | ||
| idx = 0 | ||
| while idx < len(segments) - 1 and segments[idx] in LlmProvidersSet: | ||
| idx += 1 | ||
| candidates.append("/".join(segments[idx:])) | ||
|
|
||
| base = candidates[-1] if candidates else key | ||
| for region_prefix in _BEDROCK_REGION_PREFIXES: | ||
| if base.startswith(region_prefix): | ||
| candidates.append(base[len(region_prefix) :]) | ||
|
|
||
| if provider: | ||
| stripped = _strip_model_name(model=base, custom_llm_provider=provider) | ||
| if stripped != base: | ||
| candidates.append(stripped) | ||
|
|
||
| for candidate in candidates: | ||
| entry = litellm.model_cost.get(candidate) | ||
| if entry is not None and entry.get("litellm_provider") is not None: | ||
| return dict(entry) | ||
|
Comment on lines
+2921
to
+2941
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| return None | ||
|
|
||
|
|
||
| def register_model(model_cost: Union[str, dict]): # noqa: PLR0915 | ||
| """ | ||
| Register new / Override existing models (and their pricing) to specific providers. | ||
|
|
@@ -2933,6 +2988,26 @@ def register_model(model_cost: Union[str, dict]): # noqa: PLR0915 | |
| except Exception: | ||
| existing_model = {} | ||
| model_cost_key = key | ||
| builtin_entry = _resolve_builtin_model_cost_entry( | ||
| key=_key_str, provider=provider | ||
| ) | ||
| if builtin_entry is not None: | ||
| for field in _CACHE_PRICING_FIELDS: | ||
| if ( | ||
| value.get(field) is None | ||
| and builtin_entry.get(field) is not None | ||
| ): | ||
| existing_model[field] = builtin_entry[field] | ||
| elif ( | ||
| value.get("cache_creation_input_token_cost") is None | ||
| and value.get("cache_read_input_token_cost") is None | ||
| ): | ||
| verbose_logger.warning( | ||
| f"register_model: model={key} not in built-in cost map and no " | ||
| "prefix/region variant matched; cache cost fields will default " | ||
| "to 0. To track cache cost, add cache_creation_input_token_cost " | ||
| "and cache_read_input_token_cost to model_info" | ||
| ) | ||
| # ``get_model_info`` returns ``litellm_provider: None`` when the | ||
| # provider is unknown (e.g. custom deployments registered via | ||
| # ``Router.add_deployment``). Persisting that None into | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,87 @@ | ||
| """ | ||
| Regression test for enforced-spend underreporting when Redis fails during the | ||
| budget-reservation reconcile step of ``increment_spend_counters``. | ||
|
|
||
| Production failure mode: a managed Redis returns an intermittent timeout on the | ||
| reconcile increment. Reconcile deletes (invalidates) the shared counter and | ||
| gives up, but ``increment_spend_counters`` still treats the counter as | ||
| "already reconciled" and skips the direct increment. The actual call cost never | ||
| lands in the enforced counter, so budgets stop gating until the next cold | ||
| reseed pulls a lagging value from the DB. | ||
|
|
||
| The fix makes the reconcile path fall back to the direct increment when it | ||
| fails, so the actual cost is always written to the shared counter. | ||
| """ | ||
|
|
||
| import pytest | ||
|
|
||
| from litellm.caching import DualCache | ||
| from litellm.proxy import proxy_server | ||
|
|
||
|
|
||
| class _FlakyRedisCache: | ||
| def __init__(self) -> None: | ||
| self._store: dict = {} | ||
| self._increment_calls = 0 | ||
|
|
||
| async def async_increment(self, key, value, **kwargs): | ||
| self._increment_calls += 1 | ||
| if self._increment_calls == 1: | ||
| raise Exception("Redis timeout") | ||
| self._store[key] = float(self._store.get(key, 0.0)) + float(value) | ||
| return self._store[key] | ||
|
|
||
| async def async_get_cache(self, key, *args, **kwargs): | ||
| return self._store.get(key) | ||
|
|
||
| async def async_delete_cache(self, key, *args, **kwargs): | ||
| self._store.pop(key, None) | ||
|
|
||
| async def async_set_cache(self, key, value, *args, **kwargs): | ||
| self._store[key] = float(value) | ||
| return True | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_direct_increment_runs_when_reservation_reconcile_hits_redis_failure( | ||
| monkeypatch, | ||
| ): | ||
| hashed_token = "hashed_test_token" | ||
| counter_key = f"spend:key:{hashed_token}" | ||
| reserved_cost = 0.5 | ||
| response_cost = 1.0 | ||
|
|
||
| flaky_redis = _FlakyRedisCache() | ||
| flaky_redis._store[counter_key] = reserved_cost | ||
|
|
||
| monkeypatch.setattr(proxy_server, "prisma_client", None) | ||
| monkeypatch.setattr(proxy_server, "user_api_key_cache", DualCache()) | ||
| monkeypatch.setattr(proxy_server.spend_counter_cache, "redis_cache", flaky_redis) | ||
| proxy_server.spend_counter_cache.in_memory_cache.set_cache( | ||
| key=counter_key, value=reserved_cost | ||
| ) | ||
|
|
||
| budget_reservation = { | ||
| "reserved_cost": reserved_cost, | ||
| "finalized": False, | ||
| "entries": [ | ||
| { | ||
| "counter_key": counter_key, | ||
| "entity_type": "Key", | ||
| "entity_id": hashed_token, | ||
| "reserved_cost": reserved_cost, | ||
| "applied_adjustment": 0.0, | ||
| } | ||
| ], | ||
| } | ||
|
|
||
| await proxy_server.increment_spend_counters( | ||
| token=hashed_token, | ||
| team_id=None, | ||
| user_id=None, | ||
| response_cost=response_cost, | ||
| budget_reservation=budget_reservation, | ||
| ) | ||
|
|
||
| enforced_spend = await flaky_redis.async_get_cache(key=counter_key) | ||
| assert enforced_spend == response_cost |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_inherit_builtin_cache_pricingskips the lookup only when all fivecache_fieldsare already non-None. If a user setscache_creation_input_token_costandcache_read_input_token_cost(the two the cost calculator actually reads) but leaves the three tiered variantsNone, the function still performs the backend lookup. The behaviour is actually correct because the per-fieldif model_info.get(field) is Nonecheck inside the loop protects user-set values, but the early-return guard is misleading and causes an unnecessaryget_model_infocall. Consider tightening the early exit to just the two canonical fields to make the intent clearer.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!