fix(router): keep bedrock batch litellm_params through credential resolution - #34553
Conversation
…olution Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
Greptile SummaryThis PR preserves Bedrock batch and S3 parameters during router credential resolution.
Confidence Score: 5/5The PR appears safe to merge, with the changed allowlist fields matching downstream contracts and the original credential-dropping regression covered by a focused test. The router now retains the Bedrock batch and S3 configuration consumed by existing batch handling, while the test directly verifies the credential-filtering boundary that previously discarded those values.
|
| Filename | Overview |
|---|---|
| litellm/types/router.py | Extends the credential-resolution allowlist with types matching the downstream Bedrock batch parameter contracts. |
| tests/test_litellm/test_router.py | Adds focused regression coverage for all newly preserved parameters without network access or global-state mutation. |
Reviews (1): Last reviewed commit: "fix(router): keep bedrock batch litellm_..." | Re-trigger Greptile
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
TLDR
Problem this solves:
/v1/batchesdroppedaws_batch_role_arnHow it solves it:
CredentialLiteLLMParamsRelevant issues
Linear ticket
Resolves LIT-4781
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
Config used for both runs (
lit4781_config.yaml):Both runs start the proxy with
and then issue the same request
Before, at
8177230a29(staging HEAD); the role ARN set inlitellm_paramsnever reaches the Bedrock transformation:{ "error": { "message": "AWS IAM role ARN is required for Bedrock batch jobs. Set 'aws_batch_role_arn' in litellm_params or AWS_BATCH_ROLE_ARN env var", "type": "internal_server_error", "param": "None", "code": "500" } }After, at
8c5f96e940; the request is now signed and sent to Bedrock'sCreateModelInvocationJob, and the response is AWS rejecting the model id, which is expected since the repro account only has plain Bedrock invoke access and the buckets/role are placeholders:{ "error": { "message": "{\"message\":\"The provided model identifier is invalid.\"}", "type": "internal_server_error", "param": "None", "code": "400" } }The error moving from LiteLLM's own pre-flight check to an AWS API response is the proof that the params now survive credential resolution.
Type
🐛 Bug Fix
Changes
Router.get_deployment_credentials_with_provider()round-trips a deployment'slitellm_paramsthroughCredentialLiteLLMParams, which acts as an allowlist: anything not declared as a field is dropped bymodel_dump(exclude_none=True).s3_bucket_namewas added in #30745 to fix the/v1/filesupload, but the batch-side keys were never declared, so model-routed/v1/batcheslost them.class CredentialLiteLLMParams(BaseModel): ... s3_bucket_name: Optional[str] = None + s3_output_bucket_name: Optional[str] = None + s3_region_name: Optional[str] = None + s3_encryption_key_id: Optional[str] = None + aws_batch_role_arn: Optional[str] = None + bedrock_tags: Optional[List[Dict[str, str]]] = Noneaws_batch_role_arnands3_output_bucket_namewere fatal at batch create;s3_region_namedegraded silently to the model-derived region, ands3_encryption_key_id/bedrock_tagswere ignored.bedrock_tagsis typed as a list of{"key": ..., "value": ...}dicts to match what_validate_bedrock_tagsin the Bedrock batch transformation accepts.The new test in
tests/test_litellm/test_router.pyasserts all five keys surviveget_deployment_credentials_with_provider(); it fails on staging HEAD and passes here.ui/litellm-dashboard/src/lib/http/schema.d.tsis regenerated vianpm run gen:apisince the new fields show up in the proxy OpenAPI spec.Final Attestation
Link to Devin session: https://app.devin.ai/sessions/77e1f69362a64e55be1917ccf0a33c35
Requested by: @shivamrawat1