mirror of
https://github.com/BerriAI/litellm.git
synced 2026-09-16 23:41:43 +00:00
fix(router): make default_litellm_params updates a full replace, not a merge
update_settings(default_litellm_params=...) merged the incoming dict into the
existing one ({**old, **new}), which can only add or overwrite keys, never
remove one. Since the Admin UI reads and re-submits the entire
default_litellm_params object as a single JSON blob, a merge meant clearing a
field (e.g. removing cache_control_injection_points) by editing it out of the
UI's textarea and saving had no effect: the old key survived the merge and
stayed visible from /get/config/callbacks and active on the live router.
Replace wholesale instead, matching how every other dict-shaped router
setting (e.g. model_group_alias) is already handled via the generic setattr
path - callers are expected to submit the complete desired object, which the
UI already does by round-tripping the full current value.
Also drop explanatory comments/docstrings added earlier in this branch that
weren't requested, per this repo's no-unrequested-comments convention.
This commit is contained in:
parent
0bf9d6e8db
commit
ac09e37ca8
6 changed files with 28 additions and 47 deletions
|
|
@ -14621,13 +14621,6 @@ def _apply_webhook_role_gate(webhook_map, is_full_admin: bool):
|
|||
|
||||
|
||||
def _apply_router_settings_role_gate(router_settings: dict, is_full_admin: bool) -> dict:
|
||||
"""
|
||||
``default_litellm_params`` is an open-ended kwargs dict (Router merges it into
|
||||
every completion call), so an admin can put a shared ``api_key`` or an
|
||||
``extra_headers`` Authorization token there. Unlike the other fields on
|
||||
this page, nothing else masks it before it reaches non-full-admin callers
|
||||
of /get/config/callbacks.
|
||||
"""
|
||||
default_litellm_params = router_settings.get("default_litellm_params")
|
||||
if is_full_admin or not isinstance(default_litellm_params, dict):
|
||||
return router_settings
|
||||
|
|
|
|||
|
|
@ -9706,23 +9706,11 @@ class Router:
|
|||
_settings_to_return["routing_groups"] = [group.model_dump() for group in self._routing_groups.values()]
|
||||
return _settings_to_return
|
||||
|
||||
def _merge_default_litellm_params_setting(self, value: dict | None) -> None:
|
||||
def _replace_default_litellm_params_setting(self, value: dict | None) -> None:
|
||||
if value is not None:
|
||||
self.default_litellm_params = {**self.default_litellm_params, **value}
|
||||
self.default_litellm_params = value
|
||||
|
||||
def _remove_optional_pre_call_checks(self, removed_checks: OptionalPreCallChecks) -> list[str]:
|
||||
"""
|
||||
Reverse of `add_optional_pre_call_checks` for the subset of checks that can be
|
||||
safely turned off at runtime: clears the corresponding flag(s) on shared
|
||||
affinity callbacks, or unregisters the dedicated callback instance entirely.
|
||||
|
||||
Returns the subset of `removed_checks` that were kept active anyway because
|
||||
they're still required by config (currently only `router_budget_limiting`,
|
||||
which `Router.__init__` auto-enables whenever budgets are configured on the
|
||||
deployments/provider, independent of `optional_pre_call_checks`) - callers
|
||||
should fold these back into the tracked `optional_pre_call_checks` list so it
|
||||
doesn't claim a check is off when it's actually still enforced.
|
||||
"""
|
||||
from litellm.router_utils.pre_call_checks.encrypted_content_affinity_check import (
|
||||
EncryptedContentAffinityCheck,
|
||||
)
|
||||
|
|
@ -9762,9 +9750,6 @@ class Router:
|
|||
if check == "router_budget_limiting" and RouterBudgetLimiting.should_init_router_budget_limiter(
|
||||
model_list=self.model_list, provider_budget_config=self.provider_budget_config
|
||||
):
|
||||
# Budgets are still configured on the deployments/provider - Router.__init__
|
||||
# would auto-enable this regardless of optional_pre_call_checks, so a save
|
||||
# that omits it must not silently disable budget enforcement.
|
||||
retained_checks.append(check)
|
||||
continue
|
||||
litellm.logging_callback_manager.remove_callbacks_by_type(optional_callbacks, callback_type)
|
||||
|
|
@ -9783,12 +9768,8 @@ class Router:
|
|||
retained_checks = self._remove_optional_pre_call_checks(removed_checks) if removed_checks else []
|
||||
self.optional_pre_call_checks = list(dict.fromkeys([*value, *retained_checks]))
|
||||
|
||||
# Settings whose update logic doesn't fit `setattr(self, var, value)` (e.g.
|
||||
# merge-not-replace, or side effects beyond storing the value). Dispatched via
|
||||
# a single `var in ...` branch in update_settings so adding an entry here
|
||||
# doesn't grow that function's branch count per field.
|
||||
_CUSTOM_UPDATE_SETTINGS_HANDLERS: dict = {
|
||||
"default_litellm_params": _merge_default_litellm_params_setting,
|
||||
"default_litellm_params": _replace_default_litellm_params_setting,
|
||||
"optional_pre_call_checks": _apply_optional_pre_call_checks_setting,
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -243,9 +243,6 @@ ROUTER_SETTINGS_FIELDS: List[RouterSettingsField] = [
|
|||
"that cached the prompt."
|
||||
),
|
||||
field_default=[],
|
||||
# "forward_client_headers_by_model_group" is excluded: it's a literal in the
|
||||
# OptionalPreCallChecks type union, but add_optional_pre_call_checks has no
|
||||
# handler for it, so selecting it from the UI would silently do nothing.
|
||||
options=[
|
||||
"prompt_caching",
|
||||
"router_budget_limiting",
|
||||
|
|
|
|||
|
|
@ -80,7 +80,7 @@ ignored_function_names = [
|
|||
"_override_vector_store_methods_for_router", # No-op placeholder, called during Router init
|
||||
"_merge_tools_from_deployment", # Tested indirectly via _update_kwargs_with_deployment (test files lack "router" in name)
|
||||
"_invalidate_access_groups_cache", # Tested indirectly via set_model_list, upsert_model etc. (test files lack "router" in name)
|
||||
"_merge_default_litellm_params_setting", # Tested indirectly via update_settings(default_litellm_params=...)
|
||||
"_replace_default_litellm_params_setting", # Tested indirectly via update_settings(default_litellm_params=...)
|
||||
"_apply_optional_pre_call_checks_setting", # Tested indirectly via update_settings(optional_pre_call_checks=...)
|
||||
"_remove_optional_pre_call_checks", # Tested indirectly via update_settings(optional_pre_call_checks=...)
|
||||
]
|
||||
|
|
|
|||
|
|
@ -5065,19 +5065,15 @@ def _make_router_for_settings_tests(**kwargs):
|
|||
)
|
||||
|
||||
|
||||
def test_update_settings_merges_default_litellm_params_without_dropping_existing_keys():
|
||||
"""
|
||||
Regression test: `update_settings(default_litellm_params=...)` must merge into
|
||||
the existing dict, not replace it wholesale. A naive `setattr` replace would
|
||||
silently drop keys the Router set at init (e.g. `timeout`, `max_retries`,
|
||||
`metadata`) whenever an admin edits `default_litellm_params` from the UI to
|
||||
add something like `cache_control_injection_points`.
|
||||
"""
|
||||
def test_update_settings_replaces_default_litellm_params_wholesale():
|
||||
router = _make_router_for_settings_tests(timeout=42)
|
||||
assert router.default_litellm_params.get("timeout") == 42
|
||||
|
||||
router.update_settings(
|
||||
default_litellm_params={"cache_control_injection_points": [{"location": "message", "role": "system"}]}
|
||||
default_litellm_params={
|
||||
"timeout": 42,
|
||||
"cache_control_injection_points": [{"location": "message", "role": "system"}],
|
||||
}
|
||||
)
|
||||
|
||||
assert router.default_litellm_params["timeout"] == 42
|
||||
|
|
@ -5086,6 +5082,25 @@ def test_update_settings_merges_default_litellm_params_without_dropping_existing
|
|||
]
|
||||
|
||||
|
||||
def test_update_settings_can_clear_default_litellm_params_keys():
|
||||
"""
|
||||
Regression test: a prior merge-not-replace implementation could never remove a
|
||||
key - `{**old, **new}` keeps any key present in `old` but absent from `new`.
|
||||
Saving `default_litellm_params` after removing `cache_control_injection_points`
|
||||
from the Admin UI's textarea must actually clear it, not silently keep the old
|
||||
callback-affecting value active.
|
||||
"""
|
||||
router = _make_router_for_settings_tests()
|
||||
router.update_settings(
|
||||
default_litellm_params={"cache_control_injection_points": [{"location": "message", "role": "system"}]}
|
||||
)
|
||||
assert "cache_control_injection_points" in router.default_litellm_params
|
||||
|
||||
router.update_settings(default_litellm_params={})
|
||||
|
||||
assert "cache_control_injection_points" not in router.default_litellm_params
|
||||
|
||||
|
||||
def test_update_settings_optional_pre_call_checks_is_idempotent():
|
||||
"""
|
||||
Regression test: `add_optional_pre_call_checks` has no built-in guard against
|
||||
|
|
|
|||
|
|
@ -207,11 +207,6 @@ describe("RouterSettings", () => {
|
|||
});
|
||||
|
||||
it("should round-trip default_litellm_params and optional_pre_call_checks unmodified on save", async () => {
|
||||
// Regression test: default_litellm_params holds a dict (e.g. cache_control_injection_points),
|
||||
// not a plain string - without listing it in the save handler's jsonKeys set, it'd be persisted
|
||||
// as raw stringified text instead of parsed JSON. optional_pre_call_checks is a list owned by
|
||||
// its own multi-select (no DOM input); without the fallback-to-state path in parseInputValue,
|
||||
// an untouched value would be silently dropped instead of round-tripping through Save.
|
||||
vi.mocked(getCallbacksCall).mockResolvedValue({
|
||||
router_settings: {
|
||||
...mockCallbacksResponse.router_settings,
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue