fix(check_batch_cost): delegate file-id reconciliation to ensure_batch_response_managed_file_ids (LIT-3386) - #29131
Conversation
|
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Greptile SummaryThis PR fixes
Confidence Score: 5/5Safe to merge; the change narrows scope by delegating to an already-tested central helper, and the defensive copy-on-read guards against cross-iteration state bleed. The diff replaces a narrow manual reconciliation block with a call to the same helper used on the HTTP path, so correctness follows from the helper's own test coverage. The two previous concerns (class-level dict mutation and silent degraded mode) are both addressed: _hidden_params is copied before mutation and a verbose_proxy_logger.warning is emitted on assignment failure. New tests drive real LiteLLMBatch objects through the full code path with stronger assertions than the previous mock-heavy approach. No files require special attention.
|
| Filename | Overview |
|---|---|
| enterprise/litellm_enterprise/proxy/common_utils/check_batch_cost.py | Replaces partial manual file-ID reconciliation with the central helper; correctly copies _hidden_params before mutation and warns on assignment failure. |
| tests/proxy_unit_tests/test_check_batch_cost.py | Existing test upgraded to use real LiteLLMBatch objects and stronger assertions; two new tests added for input_file_id resolution and managed-row dedup; no real network calls. |
Reviews (2): Last reviewed commit: "fix(check_batch_cost): copy-on-read _hid..." | Re-trigger Greptile
…ailure (Greptile)
|
Addressed both Greptile P2 suggestions in de102db:
All 9 tests still pass ( |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
What
When the user has uploaded the batch input file through the proxy (so a
LiteLLM_ManagedFileTablerow exists for the raw providerinput_file_id), theCheckBatchCostbackground poller currently persists the raw providerinput_file_idinLiteLLM_ManagedObjectTable.file_objectinstead of the managed unified ID.GET /v1/batches/{id}then returns a rawinput_file_idthat the user cannot correlate back to their managed upload, and listing endpoints can't round-trip it.update_batch_in_database(used by the retrieve/cancel HTTP path) already does this reconciliation via the central helperensure_batch_response_managed_file_ids.CheckBatchCosthad its own partial reconciliation block from #27984 that only coveredoutput_file_id/error_file_idand didn't consult existing managed_file rows — missing bothinput_file_idand dedup behavior.Closes the remaining gap called out at the bottom of #28294 ("PR does not touch check_batch_cost.py") for LiteLLM ticket LIT-3386.
Change
enterprise/litellm_enterprise/proxy/common_utils/check_batch_cost.py: replace the partial in-place output/error registration with a single call toensure_batch_response_managed_file_ids(the same helperupdate_batch_in_databaseuses on the retrieve/cancel HTTP path). The helper:input_file_idto the existing managed unified ID (the gap the legacy block missed),output_file_id/error_file_idto existing managed rows when one already exists (avoiding duplicate writes by parallel paths),Auth context (
UserAPIKeyAuth.user_id = job.created_by) is unchanged — still anchored on the batch creator so managed rows are owned by the real user, notdefault_user_id.response._hidden_params["model_id"]is populated defensively from the deployment we just resolved so the helper has all the metadata it needs to register new rows when the provider transformation didn't set it.Files
enterprise/litellm_enterprise/proxy/common_utils/check_batch_cost.py— swap manual reconciliation block forensure_batch_response_managed_file_ids.tests/proxy_unit_tests/test_check_batch_cost.py— update the existing raw-output coverage to the new code path; add 2 new tests for input-file-id resolution and reuse-dedup behavior.Evidence
Runtime evidence harness drives the real
CheckBatchCost.check_batch_cost()against a mock prisma + router that mirrors the production data path (the poller has no HTTP trigger by design, so this is the closest practical reproduction). Captures the JSONfile_objectpayload thatCheckBatchCostpersists tolitellm_managedobjecttable.update(). The setup pre-populates a managed_file row for the raw input file id (simulating a user who uploaded their batch input through the proxy first).BEFORE — clean upstream main HEAD
5699a06:AFTER — with this fix:
Key delta (BEFORE -> AFTER):
persisted file_object.input_file_id'file-bedrock-input-bbb111'(raw provider value) ->FLAG_INPUT_IS_RAW=TrueFLAG_INPUT_IS_MANAGED=Trueensure_batch_response_managed_file_ids invocations0(legacy partial block ran)1(delegated to central helper)response.output_file_id/error_file_iduser_id=shin-real-user-iduser_id=shin-real-user-id(unchanged)Unit tests
9/9 pass in
tests/proxy_unit_tests/test_check_batch_cost.py(6 pre-existing kept as-is, 1 updated to drive through the helper, 2 new tests added):Related coverage (
tests/test_litellm/enterprise/proxy/test_batch_update_db_managed_output_file_id.py, 8/8) and broader batch/managed_file tests (pytest -k "batch or managed_file"in proxy_unit_tests, 14/14) also pass.New tests:
test_input_file_id_resolved_to_existing_managed_id— mockslitellm_managedfiletable.find_firstto return a managed row for the raw input file id; asserts the persistedfile_object.input_file_idis the managed unified id (was the raw value onmain).test_existing_output_managed_row_is_reused_not_duplicated— mocks an existing managed row for the raw output_file_id; assertsstore_unified_file_id/get_unified_output_file_idare NOT called (no duplicate write) andresponse.output_file_idis the existing managed id.Updated test:
test_raw_output_file_id_converted_to_managed_id— same scope (covers the no-existing-managed-row path) but now drives through the helper instead of the legacy block; uses a real base64-encoded unified_object_id and mocksfind_first -> None.Linear
LIT-3386 — https://linear.app/litellm-ai/issue/LIT-3386/fix-remaining-batch-id-transformation-in-checkbatchcost
Verification (ship-pr)
litellm_oss_agent_shin_daily_branch✓de102db— "Safe to merge; the change narrows scope by delegating to an already-tested central helper, and the defensive copy-on-read guards against cross-iteration state bleed." Two P2 suggestions from Greptile's first pass addressed inde102db.pytest,lint,code-quality,semgrep,secret-scan,validate-model-prices-json, all proxy-* suites, all-other-providers, vertex, integrations, enterprise-routing, build-ui, docs)cleantests/proxy_unit_tests/test_check_batch_cost.py; relatedtest_batch_update_db_managed_output_file_id.py8/8; broader-k "batch or managed_file"14/145699a06) + AFTER terminal output inlined above (persisted file_object.input_file_idflips from raw provider value to managed unified id; helper invocation count flips from 0 → 1)check_batch_cost.py+ the test file)