mirror of
https://github.com/BerriAI/litellm.git
synced 2026-10-06 02:48:13 +00:00
3 commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
yucheng-berri
|
fc5ab31fba
|
refactor(proxy/batches): make managed-file resolution purely additive, fall back like base (#34584)
Restore the original PR's behavior on every path that did not already resolve: no database, a lookup error, a missing managed-file row, or a row without a storage_url all fall back to dispatching the original id, which the managed-files deployment hook still maps. This drops the 404 and 503 fail-closed responses I had added, which were the only behaviors that diverged from litellm_internal_staging. The change is now strictly additive: when a managed-file row with a storage_url exists, the unified batch branch substitutes it so providers like Vertex receive a real gs:// path instead of the opaque token; every other path behaves exactly as before. Verified live that non-managed, managed-owner, multi-model load-balanced, and missing-row requests are byte-identical to base |
||
|
yucheng-berri
|
5677bc237c
|
fix(proxy/batches): resolve managed unified input_file_id to storage_url with ownership check before dispatch (#34474)
* fix: resolve unified_file_id to real storage_url before dispatching batch create litellm.create_batch() against a Vertex AI-backed model crashes with an opaque error when the input file was uploaded as a LiteLLM-managed 'unified file' (multi-model file upload). The base64-encoded unified_file_id token is a LiteLLM-internal identifier, not a real provider-side file reference, but the batches_endpoints create_batch handler forwards it unchanged to llm_router.acreate_batch() / litellm.acreate_batch() for the unified_file_id branch. Provider-specific code that expects a real file location (e.g. Vertex AI's batch transformation, which parses a 'publishers/' segment out of the GCS URI) then fails on the opaque token. Resolve the unified_file_id to its real backend location (LiteLLM_ManagedFileTable.storage_url) before dispatch, mirroring the same lookup already used by the files retrieve/download endpoints for managed files. Falls back to the previous (unchanged) behavior if no managed-file record exists. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(proxy/batches): null-guard await on find_first for sync MagicMock test harnesses * fix(proxy/batches): enforce ownership and correct lookup key when resolving managed input_file_id The adopted resolution queried LiteLLM_ManagedFileTable with the decoded litellm_proxy string, but the unified_file_id column stores the raw base64 file id (see schema.prisma and the enterprise managed-files hook), so the lookup never matched in production and silently fell back to the opaque id. Query with the raw id instead and lock the key with a regression test. Move the resolution above the dispatch branches so the load-balanced router path receives the resolved storage_url too, enforce managed-file ownership with the same can_access_resource semantics the files retrieve and download endpoints use (404 on denial), and downgrade database failures to a logged fallback instead of aborting batch creation. Unresolved ids still dispatch unchanged because the managed-files deployment hook can map them via model_file_id_mapping * fix(proxy/batches): fail closed when the managed file ownership lookup errors A lookup exception previously fell back to dispatching the original unified id with the ownership gate unexecuted; the managed-files deployment hook maps unified ids from cache without re-checking ownership, so a database outage let a caller dispatch another tenant's file. Raise a clear 503 instead and lock the behavior with a regression test. No-database and no-row cases still fall back unchanged * test(proxy/batches): default harness prisma_client to None The batch routing harness left proxy_server.prisma_client at its module global, which a sibling test in the same shard can leave as a MagicMock. The unified-file rows that do not opt into managed-file resolution then entered the resolver and awaited a non-awaitable mock, surfacing as a 503. Patch prisma_client to None by default so those rows stay a no-op; resolution tests still override it explicitly * fix(proxy/batches): keep unified resolution in its own branch and fail closed on missing row Cursor flagged that hoisting the storage_url substitution above the load-balanced dispatch branch broke two things on that path: the model_file_id_mapping deployment filter keys on the original unified id, and the response returned the internal storage_url instead of the unified id. Move the resolution back inside the unified branch and exclude unified ids from the load-balanced branch so a managed file always takes the resolving path (which restores input_file_id and the unified_file_id hidden param on the response), and a load-balanced batch keeps the original id for deployment filtering. Also fail closed with a 404 when a unified id has no managed-file row while a database is present: the id cannot be ownership-verified, and dispatching it would both bypass the gate and hit the Vertex publishers-segment IndexError. Owned rows without a storage_url (legacy) still dispatch the original id * fix(proxy/batches): do not divert unified files off the load-balanced branch Excluding unified ids from the load-balanced branch (and not unified_file_id) regressed a path that works on the base revision: a multi-model managed file dispatched with an explicit router model under load balancing was routed into the unified branch, which raises a 400 for anything other than exactly one target model. Verified live against base (200, managed-files deployment hook remaps the unified id per model) versus the guarded branch (400 Expected 1 model, got 2). Restore the original three-condition load-balanced branch so that path keeps working unchanged. Unified-file storage_url resolution and the ownership 404 still apply on the non-load-balanced unified branch, which is the common managed-batch flow; the load-balanced managed path retains its existing behavior and its pre-existing enterprise-hook ownership gap, unchanged from base * refactor(proxy/batches): scope managed-file handling to resolution, drop ownership check Narrow this PR to its one problem: resolving a managed unified input_file_id to its backend storage_url so provider batch handlers (Vertex parses a publishers/ segment) receive a real location instead of the opaque token, and failing closed with a 404 when the token has no backing row so it is never dispatched into the provider crash. Remove the cross-tenant ownership check (can_access_resource) added earlier. Batch-create had no ownership enforcement before this PR, and the gap spans every managed-file call type, so it belongs in the enterprise managed-files pre-call hook (its acreate_batch branch) where files, batches and fine-tuning are covered uniformly, not partially in this one endpoint. Filed as a follow-up. This also removes the load-balanced-path ownership inconsistency the bots flagged, since there is no ownership branch to skip. Drop the inline comments flagged against the no-comments rule; behavior is documented in the helper docstring and the test docstrings * fix(proxy/batches): fail closed with 503 when the managed-file lookup errors A lookup exception previously fell back to dispatching the unresolved unified token, which defeats the fail-closed guarantee: the token still reaches the provider and can hit the same publishers-segment IndexError the resolution prevents. Treat a lookup error like the missing-row case and fail closed, but with a retryable 503 since the condition is transient. No-database and no-storage_url rows still fall back unchanged --------- Co-authored-by: htourinho-clgx <htourinho@cotality.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> |
||
|
Sameer Kankute
|
2cf565ae28
|
test(batches): add 1:1 test file scaffold for batches component paths (#30529)
* test(batches): add 1:1 test file scaffold for batches component paths Co-authored-by: Cursor <cursoragent@cursor.com> * Add harness test for create batch endpoint * Add retrieve endpoint harness tests * Add list endpoint harness tests * Add cancel endpoint harness tests * Add cancel endpoint harness tests * Add test for litellm/batches/main.py * Add test for litellm/tests/test_litellm/batches/test_batch_utils.py * Add handler and transformation tests for all providers * Fix: run batches tests in cicd * fix(tests): remove azure/__init__.py that shadowed azure namespace package Adding __init__.py to tests/test_litellm/llms/azure/ caused pytest to insert tests/test_litellm/llms/ into sys.path[0], making our empty azure/ dir shadow the real azure-identity namespace package. Any test that patched azure.identity.* would then fail with AttributeError. * style(tests): apply ruff format to test_batch_utils.py Base migrated the formatter from black to ruff format (#31317); reformat the batches scaffold test file to match. --------- Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: mateo-berri <277851410+mateo-berri@users.noreply.github.com> |