feat(gateway): add multimodal support to Messages API gRPC pipeline - #776
Conversation
Wire the existing multimodal processing infrastructure (image fetch, preprocessing, token expansion, backend-specific assembly) into the Messages API pipeline. The chat completion pipeline already supports multimodal; this change adds the same capability for Messages API requests. What changed: - multimodal.rs: add has_multimodal_content_messages() detection for InputContentBlock::Image, extract_content_parts_messages() to convert ImageSource::Base64/Url to ChatContentPart::ImageUrl (data URLs for base64, passthrough for URLs), and process_multimodal_messages() entry point. Refactor process_multimodal() to delegate to a shared process_multimodal_parts() core so both pipelines share the same fetch → preprocess → expand → intermediate logic. - messages/preparation.rs: add Step 3.5 multimodal processing block mirroring chat/preparation.rs — detect multimodal content, resolve tokenizer source, call process_multimodal_messages(), update token_ids with expanded tokens, store multimodal_intermediate on ProcessedMessages. Remove clippy::unused_async expect since .await is now used. - messages/request_building.rs: assemble backend-specific multimodal data from the intermediate (via assemble_multimodal_data) and pass it to build_messages_request() instead of None. Why: Messages API requests containing images (base64 or URL) were silently ignored — the pipeline passed None for multimodal data. This blocked vision model usage through the Messages API. How: Reuse the entire existing multimodal stack. Only the top-of-funnel detection and extraction differ (InputMessage/InputContentBlock types vs ChatMessage/ContentPart types). Everything from MultimodalIntermediate downward — image preprocessing, token expansion, backend-specific assembly (sglang/vllm/trtllm), proto conversion — is shared. Signed-off-by: Simon Lin <simonslin@gmail.com> Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
github-actions
Bot
added
grpc
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Messages API by introducing comprehensive multimodal support, allowing it to process image content for vision models. By leveraging and refactoring the existing multimodal infrastructure, the changes ensure that image-based requests are no longer silently ignored, thereby unlocking new capabilities for users interacting with the API. The core processing logic is now unified, promoting code reuse and maintainability across different API pipelines. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Caution Review failedPull request was closed or merged during review Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Messages-API multimodal support and a unified media-content pipeline: detects multimodal content in Changes
Sequence DiagramsequenceDiagram
participant Client
participant Prep as MessagePreparationStage
participant Detector as MultimodalDetector
participant Extractor as ContentExtractor
participant Tokenizer as Tokenizer
participant Processor as MultimodalProcessor
participant Builder as RequestBuilder
Client->>Prep: send InputMessage[] (may include Image/Text blocks)
Prep->>Detector: has_multimodal_content_messages(messages)
Detector-->>Prep: true/false
alt multimodal present
Prep->>Extractor: extract_content_parts_messages(messages)
Extractor-->>Prep: Vec<MediaContentPart>
Prep->>Tokenizer: resolve tokenizer & token_ids
Tokenizer-->>Prep: tokenizer, token_ids
Prep->>Processor: process_multimodal_messages(parts, model_id, tokenizer, token_ids, components, src)
rect rgba(100,150,200,0.5)
Processor->>Processor: fetch & preprocess media
Processor->>Processor: expand token_ids / embed media
Processor->>Processor: assemble multimodal_data & intermediate
end
Processor-->>Prep: MultimodalOutput (expanded_token_ids, intermediate)
Prep->>Builder: build_messages_request(..., multimodal_data)
else no multimodal
Prep->>Builder: build_messages_request(..., None)
end
Builder-->>Client: gRPC request/response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively integrates multimodal support into the Messages API pipeline by reusing existing infrastructure from the chat pipeline. The refactoring to create a shared process_multimodal_parts function is a solid choice for maintainability, and the changes in the preparation and request-building stages are logical and consistent with the established patterns. I've provided a few suggestions for refactoring to enhance code clarity and align with idiomatic Rust practices, referencing relevant repository rules.
| messages.iter().any(|msg| { | ||
| if msg.role != Role::User { | ||
| return false; | ||
| } | ||
| match &msg.content { | ||
| InputContent::Blocks(blocks) => blocks | ||
| .iter() | ||
| .any(|block| matches!(block, InputContentBlock::Image(_))), | ||
| InputContent::String(_) => false, | ||
| } | ||
| }) |
There was a problem hiding this comment.
The closure passed to any() can be simplified by combining the role check with the content check using a logical AND (&&). This makes the intent clearer and more concise.
messages.iter().any(|msg| {
msg.role == Role::User
&& match &msg.content {
InputContent::Blocks(blocks) => blocks
.iter()
.any(|block| matches!(block, InputContentBlock::Image(_))),
InputContent::String(_) => false,
}
})References
- Prioritize code simplicity and clarity over micro-optimizations, especially when the performance gain is negligible for typical use cases (e.g., iterating over a small number of items).
| fn extract_content_parts_messages(messages: &[InputMessage]) -> Vec<ChatContentPart> { | ||
| let mut parts = Vec::new(); | ||
|
|
||
| for msg in messages { | ||
| if msg.role != Role::User { | ||
| continue; | ||
| } | ||
| let blocks = match &msg.content { | ||
| InputContent::Blocks(blocks) => blocks, | ||
| InputContent::String(_) => continue, | ||
| }; | ||
|
|
||
| for block in blocks { | ||
| match block { | ||
| InputContentBlock::Image(image_block) => match &image_block.source { | ||
| ImageSource::Base64 { media_type, data } => { | ||
| // Convert base64 to data URL for the media connector | ||
| let data_url = format!("data:{media_type};base64,{data}"); | ||
| parts.push(ChatContentPart::ImageUrl { | ||
| url: data_url, | ||
| detail: None, | ||
| uuid: None, | ||
| }); | ||
| } | ||
| ImageSource::Url { url } => { | ||
| parts.push(ChatContentPart::ImageUrl { | ||
| url: url.clone(), | ||
| detail: None, | ||
| uuid: None, | ||
| }); | ||
| } | ||
| }, | ||
| InputContentBlock::Text(text_block) => { | ||
| parts.push(ChatContentPart::Text { | ||
| text: text_block.text.clone(), | ||
| }); | ||
| } | ||
| _ => {} | ||
| } | ||
| } | ||
| } | ||
|
|
||
| parts | ||
| } |
There was a problem hiding this comment.
This function can be refactored to use a more idiomatic functional style with iterators. This approach chains iterator methods like filter_map and flatten to process the messages, which can improve readability by reducing nesting and eliminating the need for a mutable parts vector.
fn extract_content_parts_messages(messages: &[InputMessage]) -> Vec<ChatContentPart> {
messages
.iter()
.filter(|msg| msg.role == Role::User)
.filter_map(|msg| match &msg.content {
InputContent::Blocks(blocks) => Some(blocks.iter()),
_ => None,
})
.flatten()
.filter_map(|block| match block {
InputContentBlock::Image(image_block) => match &image_block.source {
ImageSource::Base64 { media_type, data } => {
let data_url = format!("data:{media_type};base64,{data}");
Some(ChatContentPart::ImageUrl {
url: data_url,
detail: None,
uuid: None,
})
}
ImageSource::Url { url } => Some(ChatContentPart::ImageUrl {
url: url.clone(),
detail: None,
uuid: None,
}),
},
InputContentBlock::Text(text_block) => Some(ChatContentPart::Text {
text: text_block.text.clone(),
}),
_ => None,
})
.collect()
}References
- Prioritize code simplicity and clarity over micro-optimizations, especially when the performance gain is negligible for typical use cases (e.g., iterating over a small number of items).
- When using
flat_mapon an iterator from amatchexpression, prefer returning iterators over existing data (e.g., using.as_slice()or&[]) over creating and iterating on a temporary value (e.g.,[].iter()) to prevent lifetime issues.
| match multimodal::process_multimodal_messages( | ||
| &request.messages, | ||
| model_id, | ||
| &*tokenizer, | ||
| token_ids, | ||
| mm_components, | ||
| &tokenizer_source, | ||
| ) | ||
| .await | ||
| { | ||
| Ok(output) => { | ||
| debug!( | ||
| function = "MessagePreparationStage::execute", | ||
| expanded_tokens = output.expanded_token_ids.len(), | ||
| "Multimodal processing complete" | ||
| ); | ||
| token_ids = output.expanded_token_ids; | ||
| multimodal_intermediate = Some(output.intermediate); | ||
| } | ||
| Err(e) => { | ||
| error!( | ||
| function = "MessagePreparationStage::execute", | ||
| error = %e, | ||
| "Multimodal processing failed" | ||
| ); | ||
| return Err(error::bad_request( | ||
| "multimodal_processing_failed", | ||
| format!("Multimodal processing failed: {e}"), | ||
| )); | ||
| } | ||
| } |
There was a problem hiding this comment.
This match block for error handling can be made more concise by using map_err and the ? operator. This is a common and idiomatic pattern in Rust for chaining operations that can fail.
let output = multimodal::process_multimodal_messages(
&request.messages,
model_id,
&*tokenizer,
token_ids,
mm_components,
&tokenizer_source,
)
.await
.map_err(|e| {
error!(
function = "MessagePreparationStage::execute",
error = %e,
"Multimodal processing failed"
);
error::bad_request(
"multimodal_processing_failed",
format!("Multimodal processing failed: {e}"),
)
})?;
debug!(
function = "MessagePreparationStage::execute",
expanded_tokens = output.expanded_token_ids.len(),
"Multimodal processing complete"
);
token_ids = output.expanded_token_ids;
multimodal_intermediate = Some(output.intermediate);References
- Prioritize code simplicity and clarity over micro-optimizations, especially when the performance gain is negligible for typical use cases (e.g., iterating over a small number of items).
- Refactor
matchstatements to avoid duplication. When arms have common logic, use thematchto return the differing value and perform the common logic once. - Do not introduce panics in code that interacts with external systems if the upstream server does not handle the error. Instead, handle the error gracefully or propagate it appropriately.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@model_gateway/src/routers/grpc/regular/stages/messages/preparation.rs`:
- Around line 126-154: The multimodal error is being logged and returned with
the full error string (which may include raw data URLs); update the handling
around multimodal::process_multimodal_messages in
MessagePreparationStage::execute so you never include the raw error text: log a
sanitized message (e.g., "Multimodal processing failed" plus an error kind or
truncated/safe indicator rather than %e) and return
error::bad_request("multimodal_processing_failed", "Multimodal processing
failed") or otherwise a redacted message instead of format!("Multimodal
processing failed: {e}"); apply the identical change to the analogous handler in
chat/preparation.rs to ensure no raw connector payloads are written to logs or
responses.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7b7183ae-c776-4289-8c8a-a38705269c68
📒 Files selected for processing (3)
model_gateway/src/routers/grpc/multimodal.rsmodel_gateway/src/routers/grpc/regular/stages/messages/preparation.rsmodel_gateway/src/routers/grpc/regular/stages/messages/request_building.rs
| match multimodal::process_multimodal_messages( | ||
| &request.messages, | ||
| model_id, | ||
| &*tokenizer, | ||
| token_ids, | ||
| mm_components, | ||
| &tokenizer_source, | ||
| ) | ||
| .await | ||
| { | ||
| Ok(output) => { | ||
| debug!( | ||
| function = "MessagePreparationStage::execute", | ||
| expanded_tokens = output.expanded_token_ids.len(), | ||
| "Multimodal processing complete" | ||
| ); | ||
| token_ids = output.expanded_token_ids; | ||
| multimodal_intermediate = Some(output.intermediate); | ||
| } | ||
| Err(e) => { | ||
| error!( | ||
| function = "MessagePreparationStage::execute", | ||
| error = %e, | ||
| "Multimodal processing failed" | ||
| ); | ||
| return Err(error::bad_request( | ||
| "multimodal_processing_failed", | ||
| format!("Multimodal processing failed: {e}"), | ||
| )); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the actual code in the file specified
cat -n model_gateway/src/routers/grpc/regular/stages/messages/preparation.rs | sed -n '100,160p'Repository: lightseekorg/smg
Length of output: 3253
🏁 Script executed:
# Check the multimodal.rs file to understand how data: URLs are constructed
cat -n model_gateway/src/routers/grpc/multimodal.rs | sed -n '1,50p'Repository: lightseekorg/smg
Length of output: 2524
🏁 Script executed:
# Search for process_multimodal_messages to understand the error handling
rg -n "process_multimodal_messages" --type rust -A 5Repository: lightseekorg/smg
Length of output: 1231
🏁 Script executed:
# Check if there's a multimodal crate and understand error types
find . -name "multimodal.rs" -o -name "lib.rs" | grep -i multimodal | head -20Repository: lightseekorg/smg
Length of output: 137
🏁 Script executed:
# Look for data: URL construction patterns
rg -n "data:" --type rust -B 2 -A 2 | head -40Repository: lightseekorg/smg
Length of output: 2886
🏁 Script executed:
# Look at the multimodal message processing function
cat -n model_gateway/src/routers/grpc/multimodal.rs | sed -n '289,400p'Repository: lightseekorg/smg
Length of output: 4361
🏁 Script executed:
# Search for inline base64 or data URL handling in multimodal
rg -n "inline.*base64|data.*base64" --type rust -B 3 -A 3Repository: lightseekorg/smg
Length of output: 1993
🏁 Script executed:
# Check the multimodal crate to understand error propagation
cat -n crates/multimodal/src/lib.rs | head -100Repository: lightseekorg/smg
Length of output: 1042
🏁 Script executed:
# Look for how InputContent with inline images is processed
rg -n "InputContent|ContentPart" model_gateway/src/routers/grpc/multimodal.rs -A 2 -B 2 | head -80Repository: lightseekorg/smg
Length of output: 3450
🏁 Script executed:
# Search for error message construction patterns in the entire codebase for multimodal
rg -n "Multimodal processing failed" --type rustRepository: lightseekorg/smg
Length of output: 613
🏁 Script executed:
# Check the chat/preparation.rs error handling for comparison
cat -n model_gateway/src/routers/grpc/regular/stages/chat/preparation.rs | sed -n '115,135p'Repository: lightseekorg/smg
Length of output: 1154
🏁 Script executed:
# Look for other error handling patterns in multimodal to see if there's already suppression elsewhere
rg -n "Err\(e\)|Err\(_e\)" model_gateway/src/routers/grpc/multimodal.rs -B 2 -A 4 | head -60Repository: lightseekorg/smg
Length of output: 42
🏁 Script executed:
# Check if there are any tests that demonstrate the data URL leak concern
rg -n "data:image" --type rust -B 2 -A 2Repository: lightseekorg/smg
Length of output: 480
🏁 Script executed:
# Check if there are error propagation tests showing actual error messages from multimodal
rg -n "process_multimodal" crates/multimodal -B 2 -A 10 | head -80Repository: lightseekorg/smg
Length of output: 42
🏁 Script executed:
# Verify if MediaConnector errors could include the data: URL
cat -n crates/multimodal/src/media.rs | sed -n '130,160p'Repository: lightseekorg/smg
Length of output: 1228
🏁 Script executed:
# Check if there are any existing error suppression patterns in the gateway for sensitive data
rg -n "Err\(_[a-z]\)" model_gateway/src/routers --type rust | head -20Repository: lightseekorg/smg
Length of output: 42
🏁 Script executed:
# Check if there are any other places where Err(_) pattern is used to suppress sensitive data
rg -n "Err\(.*_.*\)" model_gateway/src/routers/grpc --type rust -B 1 -A 2 | head -40Repository: lightseekorg/smg
Length of output: 3394
🏁 Script executed:
# Look at the multimodal tracker to see what errors it can produce
cat -n crates/multimodal/src/tracker.rs | head -100Repository: lightseekorg/smg
Length of output: 3970
🏁 Script executed:
# Search for the full error chain in multimodal tracker finalize
rg -n "finalize" crates/multimodal/src/tracker.rs -A 15Repository: lightseekorg/smg
Length of output: 1113
Redact multimodal errors before logging or returning them.
Inline base64 images are converted to data:{media_type};base64,{data} URLs (in multimodal.rs line 257) before multimodal processing. If the connector's error includes the failing data URL, both the server logs (error = %e) and the HTTP response (format!("Multimodal processing failed: {e}")) will leak the uploaded image payload. Keep the 400 Bad Request status, but redact the raw error message.
Proposed fix
Err(_e) => {
error!(
function = "MessagePreparationStage::execute",
+ model = %model_id,
- error = %e,
"Multimodal processing failed"
);
return Err(error::bad_request(
"multimodal_processing_failed",
- format!("Multimodal processing failed: {e}"),
+ "Multimodal processing failed",
));
}The same pattern exists in chat/preparation.rs lines 118–127 and should be fixed identically.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@model_gateway/src/routers/grpc/regular/stages/messages/preparation.rs` around
lines 126 - 154, The multimodal error is being logged and returned with the full
error string (which may include raw data URLs); update the handling around
multimodal::process_multimodal_messages in MessagePreparationStage::execute so
you never include the raw error text: log a sanitized message (e.g., "Multimodal
processing failed" plus an error kind or truncated/safe indicator rather than
%e) and return error::bad_request("multimodal_processing_failed", "Multimodal
processing failed") or otherwise a redacted message instead of
format!("Multimodal processing failed: {e}"); apply the identical change to the
analogous handler in chat/preparation.rs to ensure no raw connector payloads are
written to logs or responses.
CatherineSue
left a comment
CatherineSue
left a comment
There was a problem hiding this comment.
overall LGTM. not a fan of the overall design tho.
|
|
||
| /// Extract multimodal content parts from Messages API input messages, | ||
| /// converting `InputContentBlock::Image` to multimodal crate `ChatContentPart`. | ||
| fn extract_content_parts_messages(messages: &[InputMessage]) -> Vec<ChatContentPart> { |
There was a problem hiding this comment.
😂 Seems we don't want to do extraction directly on messages input so we transform it to ChatContentPart first?
| components: &MultimodalComponents, | ||
| tokenizer_source: &str, | ||
| ) -> Result<MultimodalOutput> { | ||
| let mut tracker = AsyncMultiModalTracker::new(components.media_connector.clone()); |
There was a problem hiding this comment.
Let's add comment originally at https://github.com/lightseekorg/smg/pull/776/changes#diff-a50caf12eac40e563302f834a904db895c131b4dbbade4eeb5dc61c9be6db9dbL225 here?
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@model_gateway/src/routers/grpc/multimodal.rs`:
- Around line 238-283: In extract_content_parts_messages, add a brief comment
above the catch-all arm (the `_ => {}` in the match over InputContentBlock
inside extract_content_parts_messages) explaining that non-image/text block
types—e.g., InputContentBlock::Document, ToolUse, ToolResult, VideoUrl, etc.—are
intentionally ignored here because this pipeline only extracts multimodal Image
and Text parts for the media connector; reference the catch-all and the specific
handled arms (InputContentBlock::Image, InputContentBlock::Text) so future
maintainers understand the omission.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b2200f36-cfcf-4e19-b2f7-e7f488ba323a
📒 Files selected for processing (5)
crates/multimodal/src/lib.rscrates/multimodal/src/tracker.rscrates/multimodal/src/types.rscrates/multimodal/tests/multimodal_tracker_test.rsmodel_gateway/src/routers/grpc/multimodal.rs
| /// Extract multimodal content parts from Messages API input messages, | ||
| /// converting `InputContentBlock::Image` to multimodal crate `MediaContentPart`. | ||
| fn extract_content_parts_messages(messages: &[InputMessage]) -> Vec<MediaContentPart> { | ||
| let mut parts = Vec::new(); | ||
|
|
||
| for msg in messages { | ||
| if msg.role != Role::User { | ||
| continue; | ||
| } | ||
| let blocks = match &msg.content { | ||
| InputContent::Blocks(blocks) => blocks, | ||
| InputContent::String(_) => continue, | ||
| }; | ||
|
|
||
| for block in blocks { | ||
| match block { | ||
| InputContentBlock::Image(image_block) => match &image_block.source { | ||
| ImageSource::Base64 { media_type, data } => { | ||
| // Convert base64 to data URL for the media connector | ||
| let data_url = format!("data:{media_type};base64,{data}"); | ||
| parts.push(MediaContentPart::ImageUrl { | ||
| url: data_url, | ||
| detail: None, | ||
| uuid: None, | ||
| }); | ||
| } | ||
| ImageSource::Url { url } => { | ||
| parts.push(MediaContentPart::ImageUrl { | ||
| url: url.clone(), | ||
| detail: None, | ||
| uuid: None, | ||
| }); | ||
| } | ||
| }, | ||
| InputContentBlock::Text(text_block) => { | ||
| parts.push(MediaContentPart::Text { | ||
| text: text_block.text.clone(), | ||
| }); | ||
| } | ||
| _ => {} | ||
| } | ||
| } | ||
| } | ||
|
|
||
| parts | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add a brief comment explaining which block types are intentionally skipped.
The _ => {} catch-all at line 277 silently skips InputContentBlock::Document, ToolUse, ToolResult, etc. This is consistent with the existing pattern for VideoUrl in the chat pipeline, but a brief comment would improve clarity for future maintainers.
📝 Suggested comment
InputContentBlock::Text(text_block) => {
parts.push(MediaContentPart::Text {
text: text_block.text.clone(),
});
}
- _ => {}
+ // Skip Document, ToolUse, ToolResult, etc. — only images and text are
+ // processed for multimodal; other block types pass through unchanged.
+ _ => {}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@model_gateway/src/routers/grpc/multimodal.rs` around lines 238 - 283, In
extract_content_parts_messages, add a brief comment above the catch-all arm (the
`_ => {}` in the match over InputContentBlock inside
extract_content_parts_messages) explaining that non-image/text block types—e.g.,
InputContentBlock::Document, ToolUse, ToolResult, VideoUrl, etc.—are
intentionally ignored here because this pipeline only extracts multimodal Image
and Text parts for the media connector; reference the catch-all and the specific
handled arms (InputContentBlock::Image, InputContentBlock::Text) so future
maintainers understand the omission.
e21f4da to
97c9261
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/multimodal/src/lib.rs`:
- Around line 13-15: The crate root removed the legacy export ChatContentPart
causing downstream breakage; restore a compatibility alias by re-exporting the
new type (MediaContentPart) under the old name (ChatContentPart) in lib.rs as a
deprecated shim so consumers keep working for one release—add a pub use or type
alias for ChatContentPart that points to MediaContentPart and mark it deprecated
in the documentation/comments.
In `@model_gateway/src/routers/grpc/multimodal.rs`:
- Around line 223-236: Add unit tests that cover the Messages-specific
multimodal helpers: write tests for has_multimodal_content_messages to assert
that images on non-User roles are ignored, that InputContentBlock variants that
are not images are skipped, and that image sources using ImageSource::Base64 are
normalized to a data: URI (verify the normalization helper that handles
ImageSource::Base64). Create focused cases constructing InputMessage instances
with Role::System/Assistant and Role::User, with InputContent::Blocks containing
InputContentBlock::Image with ImageSource::Base64 and non-image blocks, and
assert expected boolean results and normalized source strings via the multimodal
helper functions (e.g., has_multimodal_content_messages and the image-source
normalization function referenced in the same module).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 17ddca3c-49a2-4988-9e08-2d1f996e691e
📒 Files selected for processing (5)
crates/multimodal/src/lib.rscrates/multimodal/src/tracker.rscrates/multimodal/src/types.rscrates/multimodal/tests/multimodal_tracker_test.rsmodel_gateway/src/routers/grpc/multimodal.rs
| pub use types::{ | ||
| ChatContentPart, FieldLayout, ImageDetail, ImageFrame, ImageSize, ImageSource, Modality, | ||
| MediaContentPart, FieldLayout, ImageDetail, ImageFrame, ImageSize, ImageSource, Modality, | ||
| MultiModalData, MultiModalUUIDs, PlaceholderRange, PromptReplacement, TokenId, TrackedMedia, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Keep a compatibility export for the renamed public type.
Dropping ChatContentPart from the crate root will break downstream imports immediately. If llm-multimodal is consumed outside this workspace, please either keep a deprecated alias for one release or ship this behind an explicit breaking-version bump.
Possible compatibility shim
pub use types::{
MediaContentPart, FieldLayout, ImageDetail, ImageFrame, ImageSize, ImageSource, Modality,
MultiModalData, MultiModalUUIDs, PlaceholderRange, PromptReplacement, TokenId, TrackedMedia,
};
+#[deprecated(note = "renamed to MediaContentPart")]
+pub use types::MediaContentPart as ChatContentPart;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub use types::{ | |
| ChatContentPart, FieldLayout, ImageDetail, ImageFrame, ImageSize, ImageSource, Modality, | |
| MediaContentPart, FieldLayout, ImageDetail, ImageFrame, ImageSize, ImageSource, Modality, | |
| MultiModalData, MultiModalUUIDs, PlaceholderRange, PromptReplacement, TokenId, TrackedMedia, | |
| pub use types::{ | |
| MediaContentPart, FieldLayout, ImageDetail, ImageFrame, ImageSize, ImageSource, Modality, | |
| MultiModalData, MultiModalUUIDs, PlaceholderRange, PromptReplacement, TokenId, TrackedMedia, | |
| }; | |
| #[deprecated(note = "renamed to MediaContentPart")] | |
| pub use types::MediaContentPart as ChatContentPart; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/multimodal/src/lib.rs` around lines 13 - 15, The crate root removed
the legacy export ChatContentPart causing downstream breakage; restore a
compatibility alias by re-exporting the new type (MediaContentPart) under the
old name (ChatContentPart) in lib.rs as a deprecated shim so consumers keep
working for one release—add a pub use or type alias for ChatContentPart that
points to MediaContentPart and mark it deprecated in the documentation/comments.
97c9261 to
d7959a5
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
crates/multimodal/src/lib.rs (1)
13-16:⚠️ Potential issue | 🟠 MajorKeep a deprecated
ChatContentPartshim for one release.This re-export rename makes
llm_multimodal::ChatContentPartimports fail immediately. Ifllm-multimodalis still a published 1.x crate, keep a deprecated alias here (and ideally intypes.rsforllm_multimodal::types::ChatContentPart) or ship this behind a major-version bump.Possible compatibility shim
pub use types::{ MediaContentPart, FieldLayout, ImageDetail, ImageFrame, ImageSize, ImageSource, Modality, MultiModalData, MultiModalUUIDs, PlaceholderRange, PromptReplacement, TokenId, TrackedMedia, }; +#[deprecated(note = "renamed to MediaContentPart")] +pub use types::MediaContentPart as ChatContentPart;Run this to verify the crate’s publish/version status before deciding whether the shim can be skipped:
#!/bin/bash set -euo pipefail for file in $(fd Cargo.toml); do if rg -q '^\s*name\s*=\s*"llm-multimodal"' "$file"; then echo "== $file ==" rg -n --no-heading '^\s*(name|version|publish)\s*=' "$file" fi done rg -n --no-heading '\bChatContentPart\b' --type rustExpected results: if
llm-multimodalis still published and remains on a non-breaking version line, this rename needs a compatibility shim or a major-version bump.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/multimodal/src/lib.rs` around lines 13 - 16, Add a one-release compatibility shim that preserves the old llm_multimodal::ChatContentPart name by introducing a deprecated type alias to the new MediaContentPart; specifically, add `pub type ChatContentPart = MediaContentPart;` with a #[deprecated] annotation in the same module where MediaContentPart is defined (types.rs) and re-export the alias from lib.rs alongside the existing pub use list so imports resolving ChatContentPart continue to work for one release before removing the alias in a future major bump.model_gateway/src/routers/grpc/regular/stages/messages/preparation.rs (1)
145-154:⚠️ Potential issue | 🟠 MajorRedact multimodal errors before logging or returning them.
Line 148 logs
%eand Line 153 echoes{e}back to the client. Inline images are normalized todata:URLs inmodel_gateway/src/routers/grpc/multimodal.rs, so connector failures here can leak the full base64 payload into both logs and the 400 body.Safer error handling
- Err(e) => { + Err(_e) => { error!( function = "MessagePreparationStage::execute", - error = %e, + model = %model_id, "Multimodal processing failed" ); return Err(error::bad_request( "multimodal_processing_failed", - format!("Multimodal processing failed: {e}"), + "Multimodal processing failed", )); }Based on learnings, all multimodal processing failures in this pipeline currently stay as 400 Bad Request for simplicity, so the fix here should redact the message without changing the status mapping.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_gateway/src/routers/grpc/regular/stages/messages/preparation.rs` around lines 145 - 154, In MessagePreparationStage::execute, avoid logging or returning the raw multimodal error (currently using %e in error! and {e} in error::bad_request) because it may contain base64 data: URLs; instead sanitize/redact the error before use—e.g., derive a safe message like "multimodal_processing_failed" with non-sensitive details (error kind or a short code) and log only that safe message plus a non-sensitive error type, and pass the same redacted string into error::bad_request; reference MessagePreparationStage::execute, the multimodal normalization in multimodal.rs, and the error::bad_request call when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@crates/multimodal/src/lib.rs`:
- Around line 13-16: Add a one-release compatibility shim that preserves the old
llm_multimodal::ChatContentPart name by introducing a deprecated type alias to
the new MediaContentPart; specifically, add `pub type ChatContentPart =
MediaContentPart;` with a #[deprecated] annotation in the same module where
MediaContentPart is defined (types.rs) and re-export the alias from lib.rs
alongside the existing pub use list so imports resolving ChatContentPart
continue to work for one release before removing the alias in a future major
bump.
In `@model_gateway/src/routers/grpc/regular/stages/messages/preparation.rs`:
- Around line 145-154: In MessagePreparationStage::execute, avoid logging or
returning the raw multimodal error (currently using %e in error! and {e} in
error::bad_request) because it may contain base64 data: URLs; instead
sanitize/redact the error before use—e.g., derive a safe message like
"multimodal_processing_failed" with non-sensitive details (error kind or a short
code) and log only that safe message plus a non-sensitive error type, and pass
the same redacted string into error::bad_request; reference
MessagePreparationStage::execute, the multimodal normalization in multimodal.rs,
and the error::bad_request call when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ffac8cfe-8d6f-4e32-b8fa-afc222bca52f
📒 Files selected for processing (7)
crates/multimodal/src/lib.rscrates/multimodal/src/tracker.rscrates/multimodal/src/types.rscrates/multimodal/tests/multimodal_tracker_test.rsmodel_gateway/src/routers/grpc/multimodal.rsmodel_gateway/src/routers/grpc/regular/stages/messages/preparation.rsmodel_gateway/src/routers/grpc/regular/stages/messages/request_building.rs
Rename ChatContentPart to MediaContentPart in the llm-multimodal crate. This type is API-agnostic — it represents normalized media content for the multimodal tracker, not chat-specific content. The old name made it look like the Messages API pipeline was converting to Chat types, violating the first-class design principle. The name is now consistent everywhere — MediaContentPart in the crate and MediaContentPart in the gateway, no aliases needed. What changed: - crates/multimodal/src/types.rs: rename enum ChatContentPart → MediaContentPart - crates/multimodal/src/tracker.rs: update push_part signature and match arms - crates/multimodal/src/lib.rs: update re-export - crates/multimodal/tests/multimodal_tracker_test.rs: update test usage - model_gateway/src/routers/grpc/multimodal.rs: import MediaContentPart directly (no alias needed), update all construction sites and test assertions Signed-off-by: Simon Lin <simonslin@gmail.com> Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
d7959a5 to
28055f7
Compare
PR #776 added MediaContentPart to llm-multimodal but did not bump the crate version. The crates.io release failed because model_gateway imports MediaContentPart which does not exist in the published llm-multimodal 1.3.0. What changed: - crates/multimodal/Cargo.toml: 1.3.0 → 1.4.0 - Cargo.toml: update workspace dep to 1.4.0 Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
…mg-project#776) Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
…mg-project#776) Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
2 participants
Summary
Wire the existing multimodal processing infrastructure into the Messages API pipeline, enabling vision model usage through the Messages API (
/v1/messages).Closes #XXX
What changed
multimodal.rs: Add Messages API detection (has_multimodal_content_messages), extraction (extract_content_parts_messages), and processing entry point (process_multimodal_messages). Refactorprocess_multimodalto delegate to a sharedprocess_multimodal_partscore so both chat and messages pipelines share identical fetch → preprocess → expand → intermediate logic.messages/preparation.rs: Add Step 3.5 multimodal processing block mirroringchat/preparation.rs— detect multimodal content, resolve tokenizer source, process images, update token_ids with expanded tokens, storemultimodal_intermediateonProcessedMessages.messages/request_building.rs: Assemble backend-specific multimodal data from the intermediate viaassemble_multimodal_data()and pass it tobuild_messages_request()instead ofNone.Why
Messages API requests containing images (base64 or URL via
InputContentBlock::Image) were silently ignored — the pipeline passedNonefor multimodal data. This blocked vision model usage through the Messages API.How
Reuse the entire existing multimodal stack. Only the top-of-funnel detection and extraction differ (
InputMessage/InputContentBlocktypes vsChatMessage/ContentParttypes). Everything fromMultimodalIntermediatedownward — image preprocessing, token expansion, backend-specific assembly (sglang/vllm/trtllm), proto conversion — is shared via the newprocess_multimodal_partsfunction.Key design decisions:
ImageSource::Base64is converted to data URLs (data:{media_type};base64,{data}) for the media connector, matching how chat handles inline imagesImageSource::Urlis passed through directlyInputContentBlock::Imagetriggers multimodal (notDocumentblocks — those need separate PDF processing)Role::Usermessages only, matching Anthropic's API semanticsTest plan
cargo check -p smg— compiles clean with zero warningsSummary by CodeRabbit
New Features
Refactor
Tests