mirror of
https://github.com/BerriAI/litellm.git
synced 2026-10-08 03:08:45 +00:00
fix(proxy/batches): stop forwarding custom_llm_provider twice in list and cancel (#32813)
* fix(proxy/batches): stop forwarding custom_llm_provider twice in list and cancel
The model-routing branches of list_batches and cancel_batch passed
custom_llm_provider as an explicit kwarg while also leaving it inside the dict
they splat, so every such call raised "got multiple values for keyword argument
'custom_llm_provider'" and returned a 500.
list_batches SCENARIO 2 called data.update(credentials) but never removed
custom_llm_provider before litellm.alist_batches(custom_llm_provider=..., **data);
it now uses prepare_data_with_credentials, the same helper the create and
retrieve branches already use, which pops it out.
cancel_batch SCENARIO 3 resolved the provider with
`provider or data.pop("custom_llm_provider", None) or ...`, so when the path
param provider was set the pop short-circuited and a body custom_llm_provider
stayed in data and collided with the explicit kwarg. The body value is now
popped unconditionally before the fallback chain, so the path param wins cleanly
and data no longer carries a duplicate.
Both paths already had strict-xfail regression tests documented "remove when
fixed"; those markers are dropped so the tests now guard the fix.
Signed-off-by: Anas Khan <83116240+anxkhn@users.noreply.github.com>
* fix(proxy/files): avoid duplicate custom_llm_provider in list
Signed-off-by: Anas Khan <83116240+anxkhn@users.noreply.github.com>
---------
Signed-off-by: Anas Khan <83116240+anxkhn@users.noreply.github.com>
This commit is contained in:
parent
50ae806784
commit
7fcca523aa
4 changed files with 64 additions and 30 deletions
|
|
@ -715,7 +715,7 @@ async def list_batches(
|
|||
operation_context="batch listing",
|
||||
)
|
||||
|
||||
data.update(credentials)
|
||||
prepare_data_with_credentials(data=data, credentials=credentials)
|
||||
|
||||
response = await litellm.alist_batches(
|
||||
custom_llm_provider=credentials["custom_llm_provider"],
|
||||
|
|
@ -948,9 +948,10 @@ async def cancel_batch(
|
|||
|
||||
# SCENARIO 3: Fallback to custom_llm_provider (uses env variables)
|
||||
else:
|
||||
body_custom_llm_provider = data.pop("custom_llm_provider", None)
|
||||
custom_llm_provider: Final = (
|
||||
provider
|
||||
or data.pop("custom_llm_provider", None)
|
||||
or body_custom_llm_provider
|
||||
or get_custom_llm_provider_from_request_headers(request=request)
|
||||
or get_custom_llm_provider_from_request_query(request=request)
|
||||
or "openai"
|
||||
|
|
|
|||
|
|
@ -1352,7 +1352,7 @@ async def list_files(
|
|||
|
||||
if should_route and credentials is not None:
|
||||
# Use model-based routing with credentials from config
|
||||
data.update(credentials)
|
||||
prepare_data_with_credentials(data=data, credentials=credentials)
|
||||
response = await litellm.afile_list(
|
||||
custom_llm_provider=credentials["custom_llm_provider"],
|
||||
purpose=purpose,
|
||||
|
|
|
|||
|
|
@ -1510,26 +1510,14 @@ async def test_list__managed_files_beats_model_param(list_harness):
|
|||
|
||||
|
||||
# --------------------------------------------------------------------------- #
|
||||
# Branch 2 - model from body/query/header. CURRENTLY BROKEN: the endpoint
|
||||
# forwards custom_llm_provider both explicitly and via **data (it calls
|
||||
# data.update(credentials) but never pops custom_llm_provider the way
|
||||
# create/retrieve do through prepare_data_with_credentials), so every call
|
||||
# raises "multiple values for keyword argument 'custom_llm_provider'".
|
||||
#
|
||||
# The strict xfail below encodes the INTENDED contract (litellm seam fires,
|
||||
# creds resolved for the body model, response ids encoded). It xfails today on
|
||||
# the duplicate-kwarg TypeError; the day that branch is fixed it will XPASS and
|
||||
# strict-mode turns the green into a failure, forcing whoever fixes it to drop
|
||||
# the marker and adopt this as a live regression test.
|
||||
# Branch 2 - model from body/query/header. The endpoint resolves credentials
|
||||
# for the body model, forwards custom_llm_provider once (it pops it from data
|
||||
# via prepare_data_with_credentials the way create/retrieve do), and encodes
|
||||
# the response ids. Regression guard for the duplicate-kwarg
|
||||
# "multiple values for keyword argument 'custom_llm_provider'" bug.
|
||||
# --------------------------------------------------------------------------- #
|
||||
|
||||
|
||||
@pytest.mark.xfail(
|
||||
strict=True,
|
||||
raises=ProxyException,
|
||||
reason="list_batches model branch passes custom_llm_provider twice "
|
||||
"(explicit kwarg + **data after data.update(credentials)); remove when fixed",
|
||||
)
|
||||
@pytest.mark.asyncio
|
||||
async def test_list__model_from_body_routes_and_encodes(list_harness):
|
||||
list_harness.litellm_alist.return_value = FakeListPage([make_batch(id="batch-1"), make_batch(id="batch-2")])
|
||||
|
|
@ -1991,19 +1979,11 @@ async def test_cancel__fallback_provider_from_query(cancel_harness):
|
|||
assert cancel_harness.acancel_kwargs()["custom_llm_provider"] == "azure"
|
||||
|
||||
|
||||
@pytest.mark.xfail(
|
||||
strict=True,
|
||||
raises=ProxyException,
|
||||
reason="cancel SCENARIO 3: `provider or data.pop('custom_llm_provider')` "
|
||||
"short-circuits when provider (path param) is set, so a body "
|
||||
"custom_llm_provider is left in data and forwarded twice -> duplicate-kwarg "
|
||||
"TypeError. Intended: path param wins cleanly. Remove marker when fixed.",
|
||||
)
|
||||
@pytest.mark.asyncio
|
||||
async def test_cancel__fallback_provider_precedence_path_over_body(cancel_harness):
|
||||
"""Intended contract: provider path param beats a body custom_llm_provider.
|
||||
CURRENTLY raises because the `or` short-circuit skips the data.pop, leaving
|
||||
the body value to collide with the explicit kwarg."""
|
||||
Regression guard: the body value is popped from data before the fallback
|
||||
chain, so it never collides with the explicit kwarg."""
|
||||
await call_cancel(
|
||||
cancel_harness,
|
||||
"batch-raw-xyz",
|
||||
|
|
|
|||
|
|
@ -2346,6 +2346,59 @@ def test_list_files_resolves_wildcard_deployment_credentials(
|
|||
proxy_logging_obj.post_call_failure_hook.assert_not_called()
|
||||
|
||||
|
||||
def test_list_files_model_routing_does_not_forward_custom_llm_provider_twice(
|
||||
mocker: MockerFixture, monkeypatch, llm_router: Router
|
||||
):
|
||||
import litellm.proxy.proxy_server as ps
|
||||
from litellm.proxy._types import LitellmUserRoles
|
||||
|
||||
proxy_logging_obj = setup_proxy_logging_object(monkeypatch, llm_router)
|
||||
monkeypatch.setattr("litellm.proxy.proxy_server.master_key", None)
|
||||
monkeypatch.setattr("litellm.proxy.proxy_server.prisma_client", None)
|
||||
monkeypatch.setattr("litellm.proxy.proxy_server.llm_router", llm_router)
|
||||
proxy_logging_obj.post_call_success_hook = mocker.AsyncMock(return_value=[])
|
||||
proxy_logging_obj.post_call_failure_hook = mocker.AsyncMock()
|
||||
|
||||
captured_kwargs: dict = {}
|
||||
|
||||
async def _mock_afile_list(**kwargs):
|
||||
captured_kwargs.update(kwargs)
|
||||
return []
|
||||
|
||||
monkeypatch.setattr(litellm, "afile_list", _mock_afile_list)
|
||||
monkeypatch.setattr(
|
||||
"litellm.proxy.openai_files_endpoints.files_endpoints.handle_model_based_routing",
|
||||
lambda **kwargs: (
|
||||
True,
|
||||
"azure-gpt-4o",
|
||||
None,
|
||||
{
|
||||
"custom_llm_provider": "azure",
|
||||
"api_key": "azure-key",
|
||||
},
|
||||
),
|
||||
)
|
||||
|
||||
app.dependency_overrides[ps.user_api_key_auth] = lambda: UserAPIKeyAuth(
|
||||
api_key="test-key",
|
||||
user_role=LitellmUserRoles.PROXY_ADMIN,
|
||||
user_id="test-user",
|
||||
)
|
||||
|
||||
try:
|
||||
response = client.get(
|
||||
"/v1/files",
|
||||
headers={"Authorization": "Bearer test-key"},
|
||||
)
|
||||
finally:
|
||||
app.dependency_overrides.pop(ps.user_api_key_auth, None)
|
||||
|
||||
assert response.status_code == 200, response.text
|
||||
assert captured_kwargs["custom_llm_provider"] == "azure"
|
||||
assert captured_kwargs["api_key"] == "azure-key"
|
||||
proxy_logging_obj.post_call_failure_hook.assert_not_called()
|
||||
|
||||
|
||||
def test_list_files_without_target_model_names_uses_team_openai_deployment(
|
||||
mocker: MockerFixture, monkeypatch
|
||||
):
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue