feat(bedrock): S3 URL pass-through for multimodal content blocks#23672
feat(bedrock): S3 URL pass-through for multimodal content blocks#23672ryanh-ai wants to merge 1 commit intoBerriAI:mainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
| image=BedrockImageBlock(source=s3_source, format=bedrock_format) | ||
| ) | ||
| elif bedrock_format in supported_doc_formats: | ||
| doc_name = f"s3doc_{hashlib.sha256(s3_url.encode()).hexdigest()[:16]}_{bedrock_format}" |
Check failure
Code scanning / CodeQL
Use of a broken or weak cryptographic hashing algorithm on sensitive data High
e5e319e to
4070c74
Compare
Greptile SummaryThis PR adds S3 URI pass-through support for Bedrock's Converse API, mapping Key changes:
Confidence Score: 3/5
|
| Filename | Overview |
|---|---|
| litellm/litellm_core_utils/prompt_templates/factory.py | Core S3 pass-through logic added to BedrockImageProcessor. New helpers (_extract_video_url, _get_bedrock_format_from_extension, _create_s3_bedrock_block) and model guard are wired into user-message paths, but tool-result and assistant-content call sites at lines ~4047, ~4684, ~5080 still call process_image_sync/async without model info, silently bypassing the guard for unsupported models. |
| litellm/types/llms/bedrock.py | Adds S3Location TypedDict and makes SourceBlock total=False to allow mutual exclusivity between bytes and s3Location fields. Clean type additions with appropriate Required annotations. |
| model_prices_and_context_window.json | Adds supports_s3_input: true to 25 Bedrock-provider Nova model entries (bedrock_converse provider only). Nova Micro models and non-Bedrock providers (amazon_nova, vercel_ai_gateway) are correctly excluded. Also removes a duplicate vertex_ai/gemini-embedding-2-preview key. |
| tests/local_testing/test_bedrock_s3_content.py | Integration tests correctly placed in tests/local_testing/ and guarded by @pytest.mark.skipif requiring AWS credentials. However, the autouse=True _ensure_s3_input_flag fixture patches supports_s3_input globally for all tests, which can mask regressions in the real JSON-backed implementation since the JSON flag is already in this PR. |
Sequence Diagram
sequenceDiagram
participant User as Caller
participant BCMP as _bedrock_converse_messages_pt
participant BIP as BedrockImageProcessor
participant Guard as supports_s3_input
participant Builder as _create_s3_bedrock_block
participant AWS as Bedrock Converse API
User->>BCMP: messages with s3:// image_url
BCMP->>BIP: process_image_sync(url, model, provider)
alt url starts with s3://
BIP->>Guard: supports_s3_input(model, provider)
Guard-->>BIP: true or false from JSON config
alt unsupported model
BIP-->>BCMP: ValueError model does not support s3
else supported Nova model
BIP->>Builder: _create_s3_bedrock_block(s3_url, format)
Builder-->>BIP: BedrockContentBlock with s3Location
BIP-->>BCMP: BedrockContentBlock
BCMP-->>AWS: Converse request with s3Location block
end
else base64 or https url
BIP->>BIP: existing download and encode path unchanged
BIP-->>BCMP: BedrockContentBlock with inline bytes
BCMP-->>AWS: Converse request with inline bytes
end
Note over BIP: tool_result path at line 4047 and assistant<br/>content paths at lines 4684 and 5080 call<br/>process_image_sync with model=None bypassing the guard
Comments Outside Diff (3)
-
litellm/litellm_core_utils/prompt_templates/factory.py, line 4047-4050 (link)Model guard silently bypassed in tool-result and assistant message paths
process_image_syncandprocess_image_asyncare called here (and at the assistant-content paths at lines ~4684 and ~5080) without passingmodelorcustom_llm_provider. Because the guard inside those methods is gated onif model and not supports_s3_input(...), aNonemodel skips the check entirely.This creates an inconsistency:
- An
s3://URL in a user message → raises a clearValueErrorfor an unsupported model (e.g. Claude 3 on Bedrock). - The same
s3://URL in a tool result (here) or assistant content (lines ~4684, ~5080) → silently builds ans3Locationblock that Bedrock will reject with a cryptic API-level error.
All three call sites need the model information threaded through:
# line ~4047 — tool result path _block: BedrockContentBlock = BedrockImageProcessor.process_image_sync( image_url=image_url, format=format, model=model, # add custom_llm_provider=llm_provider, # add )
# line ~4684 — async assistant content assistants_part = await BedrockImageProcessor.process_image_async( image_url=image_url, model=model, # add custom_llm_provider=llm_provider, # add )
# line ~5080 — sync assistant content assistants_part = BedrockImageProcessor.process_image_sync( # type: ignore image_url=image_url, model=model, # add custom_llm_provider=llm_provider, # add )
This requires checking that
modelandllm_providerare in scope at each call site (they are in the functions that contain these blocks), and updating the function signatures if needed to accept and forward them. - An
-
litellm/litellm_core_utils/prompt_templates/factory.py, line 3731-3740 (link)S3 URL format dispatch relies on incomplete MIME-type subtype mapping
When users pass an explicit
formatoverride that is a full MIME type (e.g."video/quicktime"for.movfiles), the code strips the subtype:raw_format = format.split("/")[-1] if "/" in format else format bedrock_format = cls._get_bedrock_format_from_extension(raw_format.lower())
This produces
"quicktime"forvideo/quicktime, which is not inget_supported_video_types()(["mp4", "mov", "mkv", "webm", ...]). As a result:s3://bucket/clip.movwithformat="video/quicktime"raisesValueError("Unsupported file format 'quicktime'"), even though.movfiles work fine via extension detection.- Similarly,
video/x-msvideo→"x-msvideo"(not"avi"), etc.
_get_bedrock_format_from_extensiononly maps"jpg"→"jpeg"and"3gpp"→"3gp". A small MIME-subtype-to-Bedrock-format lookup table would close these gaps for common video MIME types:_MIME_SUBTYPE_TO_BEDROCK = { "quicktime": "mov", "x-msvideo": "avi", "x-matroska": "mkv", "mpeg": "mpeg", }
Rule Used: What: Avoid writing provider-specific code outside... (source)
-
tests/local_testing/test_bedrock_s3_content.py, line 900-920 (link)Autouse fixture with hardcoded "nova" string-match may mask future regressions
The
_ensure_s3_input_flagfixture isautouse=Trueand patchessupports_s3_inputwith a hardcoded lambda that matches "nova" by substring. Because the fixture applies to all tests in the file, includingtest_should_reject_s3_url_for_unsupported_model(which verifies Claude 3 is rejected), any future refactor that breaks the realsupports_s3_input()lookup would still pass locally because the mock completely replaces the function.Additionally, now that this PR adds
supports_s3_input: trueto all the relevant Nova model entries in the JSON, the real function will resolve correctly from the bundled config at test time. The fixture is therefore unnecessary for tests run against the shipped codebase and should be removed, or at minimum scoped away from tests that deliberately test model rejection:@pytest.fixture # remove autouse=True def _ensure_s3_input_flag(monkeypatch): ...
Rule Used: What: prevent any tests from being added here that... (source)
Last reviewed commit: "feat(bedrock): S3 UR..."
|
Additional research shows that Nova models are only models of |
4070c74 to
b3bb914
Compare
b3bb914 to
6612abd
Compare
|
@greptileai review |
f033dc2 to
c9612e7
Compare
|
Addressed both issues from the Greptile review:
Additional: 43 unit tests now (was 41), all passing. Full bedrock suite: 463 passed. @greptileai review |
c9612e7 to
215138d
Compare
215138d to
3e258ad
Compare
|
@greptileai review |
3e258ad to
9b8d9e8
Compare
9b8d9e8 to
733708b
Compare
733708b to
5d18d18
Compare
|
@greptile can you re-review this please |
ea6736e to
699ae62
Compare
|
@greptile can you re-review this please |
699ae62 to
b8aa8de
Compare
|
@greptile can you please review this and make sure to give the score you usually do? I need to see if the score has improved as I've addressed your feedback. Thanks! |
Summary
When users provide
s3://URLs in OpenAI-format messages, LiteLLM now maps them directly to Bedrock's natives3Locationcontent blocks — no downloading or base64-encoding required.This enables efficient multimodal workflows where large images, documents, and videos are already in S3.
Closes #21746
Changes
Core (
factory.py)BedrockImageProcessor.process_image_sync/async— new first branch:s3://→_create_s3_bedrock_block()_create_s3_bedrock_block()— routes toImageBlock/DocumentBlock/VideoBlockbased on file extension_extract_video_url()— helper forvideo_urlcontent typesupports_s3_inputflag; raises clearValueErrorfor unsupported modelsvideo_urlcontent type handling in both sync and async message transformation pathsTypes (
bedrock.py,utils.py)S3LocationTypedDict with requireduriand optionalbucketOwnerSourceBlockchanged tototal=Falsefor mutual exclusivity (bytesvss3Location)supports_s3_inputfield onModelInfoModel config (
model_prices_and_context_window.json)supports_s3_input: trueadded to 25 vision-capable Amazon Nova modelsUtility (
utils.py)supports_s3_input()helper following existing_supports_factorypatternTests
tests/test_litellm/llms/bedrock/chat/test_s3_content_blocks.py— all block types, edge cases, async, model guard, HTTPS regressiontests/llm_translation/test_bedrock_s3_content.py— real Bedrock requests with Nova Lite/Pro/2-Lite (properly skipped without AWS creds)Usage
Test results