Skip to content

fix(batches): skip discarded tokenization on allowlist-only batch input file reads - #38330

Closed
mynkyu wants to merge 22 commits into
BerriAI:mainfrom
mynkyu:fix/batch-models-only-read
Closed

fix(batches): skip discarded tokenization on allowlist-only batch input file reads#38330
mynkyu wants to merge 22 commits into
BerriAI:mainfrom
mynkyu:fix/batch-models-only-read

Conversation

@mynkyu

@mynkyu mynkyu commented Aug 26, 2026

Copy link
Copy Markdown

TLDR

Problem this solves:

  • POST /v1/batches spends ~10s per 652-row file tokenizing rows whose token totals are then thrown away
  • It happens whenever a key has a restricted models list, even with disable_batch_input_file_rate_limiting: true
  • The opt-out is unreachable for those keys: _should_skip_batch_input_file_processing returns on the allowlist check first

How it solves it:

  • Adds a models-only read: collect each row's body.model, skip _count_entry_tokens
  • Allowlist enforcement unchanged — file still downloaded, every body.model still validated
  • Purely additive; no existing signature or return shape changes

The problem

_should_skip_batch_input_file_processing consults the operator opt-outs after the model-allowlist check:

if self._key_requires_batch_model_access_check(user_api_key_dict):
    return False, None                      # restricted `models` -> full path, always

if general_settings.get("disable_batch_input_file_rate_limiting") is True:
    return True, None                       # never reached for such keys

The download itself is genuinely required — the allowlist can only be enforced by inspecting every row. But count_input_file_usage also runs _count_entry_tokens (a real tokenizer) per row, and async_pre_call_hook then discards the totals because nothing charges them.

That tokenization is the expensive half, and it is synchronous CPU work inside the async pre-call hook.

Production measurements

Observed on a proxy running v1.96.2, during a nightly backfill submitting 652-row batches to vertex_ai/gemini-2.5-flash:

latency
POST /v1/batches (652 rows) 10~12s
batch status polling, same key 0.55~0.64s
interactive calls, same deployment 0.5~0.7s

This held a proxy-wide average-latency monitor above its threshold for as long as a batch was running. Reproduced with zero 429s, so it is not retry-driven — it is the cost of the read itself.

The fix

  • _batch_input_file_models_only() reports when the JSONL is needed solely for the allowlist
  • count_input_file_usage(models_only=...) then collects body.model without tokenizing
  • async_pre_call_hook returns before counter enforcement in that mode, leaving _batch_*_count unset — matching the existing full-skip path rather than recording zeroes

Deliberately conservative — it returns False whenever anything still needs the totals:

  • unrestricted keys (already covered by the existing full-skip paths)
  • enqueued-token scopes, whose reservation is priced from the totals
  • no opt-out configured

Tests

4 added to tests/test_litellm/proxy/hooks/test_batch_file_validation.py; suite goes 79 → 83 passing.

test asserts
..._skips_tokenization_for_restricted_key file is downloaded and allowlist is enforced, but _count_entry_tokens is never called and no counters are charged
..._still_rejects_unauthorized_model a disallowed model in the JSONL still raises 403 — the opt-out must not become an authz bypass
test_restricted_key_without_opt_out_still_counts_tokens the pre-existing path is untouched
..._disabled_when_enqueued_scopes_apply enqueued reservations still get real totals

async_pre_call_hook swallows unexpected exceptions and returns data, so the returned value alone proves nothing — each test also asserts on the calls that separate the intended path from error recovery.

Control: reverting only the source change (tests kept) fails the first test with

AssertionError: Expected '_count_entry_tokens' to not have been called. Called 2 times.

confirming the test is load-bearing rather than vacuous.

Relevant issues

I could not find an existing issue for this. Note that #35408 and #35813 touch the same function (bounding the input-file read with a deadline) — this change is orthogonal (it reduces work within the read) but will likely need a trivial rebase against whichever lands first.

Linear ticket

n/a — external contribution.

Pre-Submission checklist

  • I have added meaningful tests
  • My PR passes all CI/CD checks (not yet run)
  • My PR's scope is as isolated as possible; it only solves 1 specific problem
  • I have received a Greptile Confidence Score of at least 4/5

Screenshots / Proof of Fix

Run against main in a container, with the import origin printed so it is unambiguous which tree is under test:

IMPORTED FROM: /oss/litellm
helper present: True
83 passed, 1 warning in 9.13s

Control, same command against an otherwise identical tree with only the source change reverted:

IMPORTED FROM: /ctrl/litellm
helper present: False
FAILED tests/.../test_batch_file_validation.py::test_models_only_read_skips_tokenization_for_restricted_key
1 failed, 3 passed

yuneng-berri and others added 22 commits August 25, 2026 21:24
components/shared/Alert.tsx was base-vega's own alert.tsx copied in by
hand, carrying the same four exports and the same class strings, so
npx shadcn add could never reach it and it would drift from every
upstream fix silently. It also still wrapped each part in forwardRef,
which React 19 no longer needs.

Install the primitive into components/ui/ where the CLI can update it,
and reduce the shared file to a wrapper that adds the four status
variants (info, success, warning, error) the dashboard actually uses on
top of upstream's default and destructive.

Rendered output is unchanged: every variant produces byte-identical
classes, role and data-variant, so all 45 call sites look the same.
…arch-1e4443

refactor(ui): install the shadcn alert primitive
…e registry

These four primitives still wrapped their body in React.forwardRef, which
the dashboard has not needed since it moved to React 19: a function
component receives ref as an ordinary prop and the existing {...props}
spread already hands it to the DOM node.

Re-pulling each from base-vega drops the wrapper and its displayName.
These four were picked because the ref plumbing is their only divergence
from current upstream, so the class strings, data-slot values and exports
are untouched and nothing renders differently. The other seven primitives
that still carry forwardRef have also drifted on their class strings, so
re-pulling them would ship a visual change alongside the cleanup and they
are left alone here.

Textarea is the one with real ref call sites, roughly seventeen of them
through react-hook-form's field.ref, and ref-forwarding.test.tsx did not
cover it. Add that case next to the Label, Separator and Skeleton ones
already there.
…f_from_cli_primitives

refactor(ui): re-pull label, textarea, separator and skeleton from the registry
* feat(prometheus): configure deployment caller identity

* test(prometheus): satisfy strict caller identity lint

* fix(prometheus): align caller identity on latency metrics

* fix(prometheus): validate caller identity mode before collectors register

Fail config load on an invalid prometheus_deployment_and_latency_caller_identity
value (including null) and on include_labels entries the selected mode removes
from a target metric, instead of booting green with an empty /metrics.
Validate the mode at the top of PrometheusLogger.__init__ so an invalid value
raises before any collector lands in the process-global registry, keeping
retries free of duplicated-timeseries errors. Label-validation errors now name
the mode setting alongside the rejected label.

---------

Co-authored-by: Mark Philipp <mphilipp622@gmail.com>
Co-authored-by: Yucheng Zhu <yucheng@berri.ai>
…type

The fallback mapped every unbranched provider error by status code on every route, which changed the exception class and HTTP status for those providers and failed four provider test suites in CI. The /v1/messages handler change alone covers the ticket, since the anthropic branch already maps its errors
…ception_type unchanged

Bugbot Autofix pushed e2e16d7 to split 403 out of the shared 401/403 branch in _map_openai_like_exception. That premise was the BaseLLMException fallback, which d2e4e74 already removed, and remapping 403 for every openai-like provider is a separate contract change, so this merge resolves both files back to the base branch versions
Now that /v1/messages routes provider failures through exception_type, an
Anthropic permission_error fell through the anthropic branch to the generic
APIConnectionError and reached the client as a 500 where the raw exception
used to answer 403. Map 403 to PermissionDeniedError so the status survives
on every route.
…play_hardening

test(e2e): let the Together replayed-reasoning case survive a single provider miss
…essages_error_mapping

fix(otel): map /v1/messages provider errors before failure logging
…itellm_fix_branchless_provider_status_mapping

# Conflicts:
#	tests/test_litellm/litellm_core_utils/test_exception_mapping_utils.py
…provider_status_mapping

fix(exceptions): map upstream status codes for providers with no exception_type branch
…ect_ownership

fix(passthrough): record ownership of streamed responses under managed ids
…e reads

`_should_skip_batch_input_file_processing` returns on the model-allowlist check
before it consults `disable_batch_input_file_rate_limiting` or the provider skip
list, so a key with a restricted `models` list always takes the full path even
when the operator opted out of batch input-file rate limiting. The download is
genuinely required there -- the allowlist can only be enforced by inspecting
every row -- but `count_input_file_usage` also runs `_count_entry_tokens` on
each row, and the caller then discards the totals because nothing charges them.

That tokenization is the expensive half. On a 652-row file it added ~10s to
every `POST /v1/batches`, which was enough to hold a proxy-wide average-latency
monitor above its threshold for as long as a batch was running (interactive
calls on the same deployment were 0.5~0.7s, and the same key's batch polling
was 0.55s). It is synchronous CPU work inside the async pre-call hook.

This adds a models-only read: `_batch_input_file_models_only()` reports when the
JSONL is needed solely for the allowlist, and `count_input_file_usage()` then
collects each row's `body.model` without tokenizing it.

Deliberately conservative:
  - purely additive; no existing signature or return shape changes
  - allowlist enforcement is untouched -- the file is still downloaded and every
    `body.model` still validated, so the opt-out cannot become an authz bypass
  - returns False whenever anything still needs the totals, including
    enqueued-token scopes, whose reservation is priced from them
  - the caller returns before the counters, leaving `_batch_*_count` unset,
    matching the existing full-skip path rather than recording zeroes

Tests: 4 added to tests/test_litellm/proxy/hooks/test_batch_file_validation.py
covering the skip, the preserved 403 on a disallowed model, the unchanged
no-opt-out path, and the enqueued-scope guard. Suite goes 79 -> 83 passing;
reverting only the source change fails the tokenization test
("Expected '_count_entry_tokens' to not have been called. Called 2 times.").
@CLAassistant

CLAassistant commented Aug 26, 2026

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
4 out of 5 committers have signed the CLA.

✅ mphilippnv
✅ mynkyu
✅ mateo-berri
✅ yuneng-berri
❌ cursoragent
You have signed the CLA already but the status is still pending? Let us recheck it.

@greptile-apps

greptile-apps Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR combines the batch models-only optimization with broader changes to managed pass-through IDs, Prometheus caller identity, exception mapping, streaming utilities, and dashboard primitives

  • Skips discarded tokenization while retaining batch model allowlist enforcement
  • Adds streamed Responses API managed-ID ownership and shared SSE frame splitting
  • Adds configurable Prometheus caller identity labels and broader HTTP status normalization
  • Updates dashboard primitives for ref-as-prop behavior

Confidence Score: 4/5

The PR appears safe to merge after addressing the non-blocking repository-convention issues in the Prometheus changes

The batch optimization retains model authorization, and no concrete runtime regression remains; accepted feedback is limited to mutable label transformation and unnecessary source comments

Files Needing Attention: litellm/types/integrations/prometheus.py, litellm/integrations/prometheus.py

Important Files Changed

Filename Overview
litellm/proxy/hooks/batch_rate_limiter.py Adds a models-only JSONL read that preserves allowlist checks while skipping unused tokenization
litellm/proxy/pass_through_endpoints/managed_id_rewriter.py Adds buffered SSE response-ID ownership and rewriting for managed Responses API objects
litellm/types/integrations/prometheus.py Adds caller-identity label modes, with an in-place transformation that violates the repository's immutability convention
litellm/integrations/prometheus.py Validates caller-identity configuration and propagates user email through deployment failure metrics, but adds prohibited explanatory comments
litellm/litellm_core_utils/exception_mapping_utils.py Maps additional provider HTTP statuses to normalized LiteLLM exceptions
ui/litellm-dashboard/src/components/shared/Alert.tsx Wraps the new alert primitive while retaining application-specific status variants

Reviews (1): Last reviewed commit: "fix(batches): skip discarded tokenizatio..." | Re-trigger Greptile

Comment on lines +372 to +377
if caller_identity == "user_email":
resolved_labels[alias_index] = UserAPIKeyLabelNames.USER_EMAIL.value
elif caller_identity == "both":
resolved_labels.insert(alias_index + 1, UserAPIKeyLabelNames.USER_EMAIL.value)

return resolved_labels

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 In-place label mutation

This resolver mutates resolved_labels in place, violating the repository’s immutable-data convention and complicating the label transformation

Suggested change
if caller_identity == "user_email":
resolved_labels[alias_index] = UserAPIKeyLabelNames.USER_EMAIL.value
elif caller_identity == "both":
resolved_labels.insert(alias_index + 1, UserAPIKeyLabelNames.USER_EMAIL.value)
return resolved_labels
if caller_identity == "user_email":
return [
*resolved_labels[:alias_index],
UserAPIKeyLabelNames.USER_EMAIL.value,
*resolved_labels[alias_index + 1 :],
]
if caller_identity == "both":
return [
*resolved_labels[: alias_index + 1],
UserAPIKeyLabelNames.USER_EMAIL.value,
*resolved_labels[alias_index + 1 :],
]
return resolved_labels

Context Used: CLAUDE.md (source)

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!

Comment on lines +179 to +181
# Validate the caller-identity mode before any collector registers so an
# invalid value cannot leave partially-registered metrics behind in the
# process-global registry.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Redundant implementation comments

These comments narrate the calls immediately below and duplicate ordinary behavior, adding prose that must remain synchronized with future changes

Context Used: CLAUDE.md (source)

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!

@codecov

codecov Bot commented Aug 26, 2026

Copy link
Copy Markdown

from litellm.types.integrations.prometheus import (
_sanitize_prometheus_label_name,
_sanitize_prometheus_label_value,
validate_prometheus_deployment_and_latency_caller_identity,
from litellm.litellm_core_utils.logging_worker import GLOBAL_LOGGING_WORKER
from litellm.proxy._types import PassThroughEndpointLoggingResultValues
from litellm.proxy.common_request_processing import ProxyBaseLLMRequestProcessing
from litellm.proxy.common_utils.sse_keepalive import split_complete_sse_frames
Comment thread litellm/__init__.py
prometheus_exclude_metrics: Optional[List[str]] = None
prometheus_exclude_labels: Optional[List[str]] = None
prometheus_emit_stream_label: bool = False
prometheus_deployment_and_latency_caller_identity: Literal[
whenever anything still needs the totals, including enqueued-token
scopes, whose reservation is priced from them.
"""
from litellm.proxy.proxy_server import general_settings
@mynkyu mynkyu closed this Aug 26, 2026
@codspeed-hq

codspeed-hq Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Merging this PR will not alter performance

✅ 31 untouched benchmarks


Comparing mynkyu:fix/batch-models-only-read (ba3e3e8) with main (6e569ee)

Open in CodSpeed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants