fix(router): include Bedrock batch/S3 fields and model in deployment credentials - #24548
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Lint failure is a false positive. Both files its complaining about are untouched in this PR |
Greptile SummaryThis PR fixes Bedrock batch job creation through the LiteLLM proxy by ensuring that
Confidence Score: 5/5
|
| Filename | Overview |
|---|---|
| litellm/types/router.py | Moves s3_region_name, s3_encryption_key_id, and aws_batch_role_arn from GenericLiteLLMParams into the parent CredentialLiteLLMParams whitelist, and removes redundant declarations from the subclass. The inheritance chain is preserved correctly; gcs_bucket_name and s3_bucket_name were already in the parent, so only the three new fields are net additions. |
| litellm/router.py | Adds credentials["model"] = deployment.litellm_params.model after the credential whitelist filter, so batch/file callers receive the deployment model needed to route to the Bedrock batch config. The placement is correct: after credential resolution but before returning, ensuring the model is always present for deployment-scoped calls. |
| litellm/proxy/openai_files_endpoints/common_utils.py | Strips the newly added model key from credentials returned by get_team_provider_credentials so provider-scoped callers are not silently pinned to a specific deployment model. The dict comprehension is the minimal correct change. |
| tests/test_litellm/test_router.py | Adds a pure in-memory regression test covering all three new batch/S3 fields and the returned model value. No real network calls are made, consistent with the tests/test_litellm/ convention. |
| ui/litellm-dashboard/src/lib/http/schema.d.ts | Auto-generated TypeScript schema updated to reflect the three new fields (aws_batch_role_arn, s3_region_name, s3_encryption_key_id) in the relevant param interfaces. Change appears correct and consistent with the Python model additions. |
Reviews (7): Last reviewed commit: "fix(router): return model and Bedrock ba..." | Re-trigger Greptile
| @@ -7373,6 +7373,9 @@ def get_deployment_credentials_with_provider( | |||
| # Remove the credential name since we've resolved it | |||
| credentials.pop("litellm_credential_name", None) | |||
|
|
|||
| # Add model (needed for provider-config routing in batches/files) | |||
| credentials["model"] = deployment.litellm_params.model | |||
There was a problem hiding this comment.
model now present in credentials for all callers
After this change every call to prepare_data_with_credentials — including afile_retrieve, afile_delete, afile_content, and the vector-store file endpoints — will receive model merged into their data dict via data.update(credentials).
For acreate_batch/acreate_file this is intentional and correct. For file-retrieval / file-delete operations the model key flows into **data as an unknown kwarg, which is silently absorbed by those functions' GenericLiteLLMParams(**kwargs) with extra="allow". There is no regression today, but it is worth noting that the semantics of get_deployment_credentials_with_provider have expanded: it now returns routing metadata (model) alongside credentials. If future callers are added that pass these credentials to an API that does not accept an arbitrary model kwarg, they would need to strip it explicitly.
There was a problem hiding this comment.
I think this is ok. Even if it's None. Thoughts?
There was a problem hiding this comment.
Yes, this is reasonable. A few considerations:
1. model=None would still break batch routing
If deployment.litellm_params.model is None, the batch endpoint will still hit the original error ("LiteLLM doesn't support custom_llm_provider=bedrock for 'create_batch'") because ProviderConfigManager.get_provider_batches_config() needs a model to route. So functionally, None here is no worse than the bug you're fixing—it just means the deployment is misconfigured.
2. Valid deployments should always have a model
For any functioning batch/file deployment, model should be set in litellm_params. A None here would indicate a malformed config, not a valid use case.
3. The semantic expansion is acceptable
Mixing routing metadata (model) into credentials is pragmatic. The alternative—restructuring get_deployment_credentials_with_provider to return (credentials, metadata)—is far more invasive and breaks all existing callers. The current approach:
- Works for existing callers (they absorb it via
extra="allow") - Solves the immediate bug
- Has low risk (future callers will likely also use Pydantic models with
extra="allow")
If you're concerned about None validation, you could add:
if deployment.litellm_params.model is None:
raise ValueError("model is required in litellm_params for batch/file operations")But given the scope of this fix and the fact that None would fail downstream anyway, shipping as-is seems fine.
61b8c69 to
47f02dd
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
47f02dd to
4df4ce9
Compare
4df4ce9 to
8209278
Compare
8209278 to
df70e25
Compare
570775d to
6b32cb3
Compare
|
Rebased onto One thing to call out for review: the I raised I kept the explicit-field approach (declaring the fields on |
…entials get_deployment_credentials_with_provider dropped s3_region_name, s3_encryption_key_id, and aws_batch_role_arn because CredentialLiteLLMParams never declared them, and it never returned the deployment's model, so proxy batch creation against Bedrock failed with "LiteLLM doesn't support custom_llm_provider=bedrock for 'create_batch'" or "AWS IAM role ARN is required" (BerriAI#25104) Provider-only file and batch calls keep their no-model contract: get_team_provider_credentials strips the model key so a provider-scoped request is not pinned to an arbitrary matching deployment
6b32cb3 to
3d275d9
Compare
|
Rebuilt this on current litellm_internal_staging: same credential fields plus returning the deployment model, verified live end to end on a real Bedrock batch job |
0acca3e
into
BerriAI:litellm_internal_staging
TLDR
Problem this solves:
How it solves it:
User Flow
Before: the flow dies at batch creation, with a different error depending on whether the model is repeated in the create body
After: the same upload plus the minimal create body produce a real Bedrock batch job whose results can be downloaded once it finishes
Relevant issues
Fixes #25104
Linear ticket
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
@greptileaito re-request a review after pushing changes)Delays in PR merge?
If you're seeing a delay in your PR being merged, ping the LiteLLM Team on Slack (#pr-review).
Screenshots / Proof of Fix
Live proxy, real Bedrock spend in us-west-2. Config pins a Claude Haiku 4.5 deployment with aws_region_name, s3_bucket_name, and aws_batch_role_arn in litellm_params. Before runs at commit 0659738, after runs at 3d275d9
Before, upload succeeds:
Before, create fails without a body model (the file id already encodes it):
Before, create still fails with the body model because the role ARN was dropped from credentials:
After, the same upload plus the minimal create body submit a real job:
AWS confirms the job exists with the configured role and bucket:
After, retrieve and results download once the job completes:
All 100 records completed with the exact requested reply
The issue's third symptom (inference-profile ARNs stripped to a bare model name) no longer reproduces at head: a deployment whose model is bedrock/arn:aws:bedrock:...:inference-profile/us.anthropic.claude-haiku-4-5-20251001-v1:0 created job dteqa9v1zg5p and AWS shows modelId as the full ARN, since job names no longer embed the model and the modelId passes through verbatim
Type
🐛 Bug Fix
Changes
CredentialLiteLLMParamsis the credential whitelist thatget_deployment_credentials_with_provider()filterslitellm_paramsthrough for the model-scoped files, batches, and vector-store endpoints; any field it doesn't declare is silently dropped.s3_bucket_nameandgcs_bucket_namewere promoted to it earlier, buts3_region_name,s3_encryption_key_id, andaws_batch_role_arnwere still missing, so Bedrock batch creation failed asking for a role ARN the config already set. This PR declares the three fields there and removes the now redundant redeclarations onGenericLiteLLMParamsget_deployment_credentials_with_provider()also never returned the deployment'smodel, so the proxy's model-encoded-file-id path calledlitellm.acreate_batch()without one, skipped the provider-config dispatch that loadsBedrockBatchesConfig, and 400'd with "LiteLLM doesn't support custom_llm_provider=bedrock for 'create_batch'". The credentials dict now carriesmodel. Provider-only calls keep their existing no-model contract:get_team_provider_credentials()strips the key so a provider-scoped request is not silently pinned to whichever deployment happened to match, which the existing scenario tests continue to assertRegression test:
test_get_deployment_credentials_with_provider_bedrock_batch_fieldsasserts the resolved credentials for a Bedrock deployment include the model and all three batch/S3 fieldsFinal Attestation