Repository navigation
fix(multimodal): follow-ups to the 2026-09-22 merges #2642
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 10 commits
664c877
5c54cee
36d43d9
3a00b1f
5bac697
e0dff57
c1a9234
7c20fd8
e8f1d80
76bdde1
3ac330c
d50f12e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,6 @@ | ||
| use std::{collections::HashMap, sync::OnceLock}; | ||
|
|
||
| use base64::{engine::general_purpose::STANDARD as BASE64_STANDARD, Engine}; | ||
| use serde_json::Value; | ||
| use thiserror::Error; | ||
|
|
||
|
|
@@ -8,7 +9,8 @@ use crate::{ | |
| encoder_inputs::PreprocessedEncoderInputs, | ||
| media::FrameSampling, | ||
| types::{ | ||
| EncoderFieldLayouts, FieldLayout, Modality, PromptReplacement, TokenId, VideoSamplingInfo, | ||
| EncoderFieldLayouts, FieldLayout, MediaContentPart, Modality, PromptReplacement, TokenId, | ||
| VideoSamplingInfo, | ||
| }, | ||
| vision::PreProcessorConfig, | ||
| }; | ||
|
|
@@ -42,6 +44,8 @@ pub enum ModelRegistryError { | |
| spec: &'static str, | ||
| modality: Modality, | ||
| }, | ||
| #[error("{format} images are not accepted by model spec {spec}")] | ||
| UnsupportedImageFormat { spec: &'static str, format: String }, | ||
| } | ||
|
|
||
| pub type RegistryResult<T> = Result<T, ModelRegistryError>; | ||
|
|
@@ -73,6 +77,40 @@ fn modality_limit_override(modality: Modality) -> Option<usize> { | |
| } | ||
| } | ||
|
|
||
| /// The image format a content part names: sniffed from the bytes (inline, | ||
| /// or decoded from a data URL) before the client's media type is believed, | ||
| /// else the media type in any case, else the URL path's extension. `None` | ||
| /// when nothing says (a URL without an extension, another modality). | ||
| fn image_format_of(part: &MediaContentPart) -> Option<image::ImageFormat> { | ||
| let from_media_type = |media_type: &str| { | ||
| image::ImageFormat::from_mime_type(media_type.trim().to_ascii_lowercase()) | ||
| }; | ||
| match part { | ||
| MediaContentPart::ImageData { | ||
| data, mime_type, .. | ||
| } => image::guess_format(data) | ||
| .ok() | ||
| .or_else(|| mime_type.as_deref().and_then(from_media_type)), | ||
| MediaContentPart::ImageUrl { url, .. } => { | ||
| if let Some(rest) = url.strip_prefix("data:") { | ||
| let (header, payload) = rest.split_once(',').unwrap_or((rest, "")); | ||
| let mut parameters = header.split(';'); | ||
| let media_type = parameters.next().unwrap_or_default(); | ||
| let sniffed = parameters | ||
| .any(|parameter| parameter.trim().eq_ignore_ascii_case("base64")) | ||
| .then(|| BASE64_STANDARD.decode(payload.trim()).ok()) | ||
| .flatten() | ||
| .and_then(|bytes| image::guess_format(&bytes).ok()); | ||
| return sniffed.or_else(|| from_media_type(media_type)); | ||
| } | ||
| let path = url.split(['?', '#']).next().unwrap_or_default(); | ||
|
Comment on lines
+94
to
+118
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Nit: MIME types are case-insensitive (RFC 2045), but image::ImageFormat::from_mime_type(media_type.to_ascii_lowercase())(
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 76bdde1: the media type is trimmed and ASCII-lowercased before |
||
| let extension = path.rsplit('/').next()?.rsplit_once('.')?.1; | ||
| image::ImageFormat::from_extension(extension) | ||
| } | ||
| _ => None, | ||
| } | ||
| } | ||
|
|
||
| fn check_media_counts( | ||
| spec: &'static str, | ||
| limits: &HashMap<Modality, usize>, | ||
|
|
@@ -255,6 +293,34 @@ pub trait ModelProcessorSpec: Send + Sync { | |
| }) | ||
| } | ||
|
|
||
| /// Image formats the model's vendor refuses; the gateway refuses them | ||
| /// up front, judged from the bytes, the data URL's type or the URL path. | ||
| fn rejected_image_formats(&self) -> &'static [image::ImageFormat] { | ||
| &[] | ||
| } | ||
|
|
||
| /// Refuse an image part whose format is one of | ||
| /// [`Self::rejected_image_formats`]. | ||
| fn validate_image_formats(&self, parts: &[MediaContentPart]) -> RegistryResult<()> { | ||
| let rejected = self.rejected_image_formats(); | ||
| if rejected.is_empty() { | ||
| return Ok(()); | ||
| } | ||
| for part in parts { | ||
| if let Some(format) = image_format_of(part).filter(|f| rejected.contains(f)) { | ||
| return Err(ModelRegistryError::UnsupportedImageFormat { | ||
| spec: self.name(), | ||
| format: format | ||
| .extensions_str() | ||
| .first() | ||
| .map_or("unknown", |s| *s) | ||
| .to_string(), | ||
| }); | ||
| } | ||
| } | ||
| Ok(()) | ||
| } | ||
|
|
||
| fn processor_kwargs(&self, metadata: &ModelMetadata) -> RegistryResult<Value>; | ||
|
|
||
| /// Build the audio preprocessor for this model, if it supports audio. | ||
|
|
@@ -506,6 +572,97 @@ mod tests { | |
| assert!(!TestSpec.worker_expandable(Modality::Video)); | ||
| } | ||
|
|
||
| #[test] | ||
| fn image_formats_are_accepted_unless_a_spec_rejects_them() { | ||
| assert!(TestSpec.rejected_image_formats().is_empty()); | ||
| let bmp = MediaContentPart::ImageData { | ||
| data: b"BM\x00\x00".to_vec(), | ||
| mime_type: None, | ||
| uuid: None, | ||
| detail: None, | ||
| }; | ||
| assert!(TestSpec | ||
| .validate_image_formats(std::slice::from_ref(&bmp)) | ||
| .is_ok()); | ||
|
|
||
| struct NoBmp; | ||
| impl ModelProcessorSpec for NoBmp { | ||
| fn name(&self) -> &'static str { | ||
| "no-bmp" | ||
| } | ||
| fn matches(&self, _metadata: &ModelMetadata) -> bool { | ||
| true | ||
| } | ||
| fn placeholder_token(&self, _metadata: &ModelMetadata) -> RegistryResult<String> { | ||
| Ok("<img>".to_string()) | ||
| } | ||
| fn placeholder_token_id(&self, _metadata: &ModelMetadata) -> RegistryResult<TokenId> { | ||
| Ok(1) | ||
| } | ||
| fn modality_limits( | ||
| &self, | ||
| _metadata: &ModelMetadata, | ||
| ) -> RegistryResult<HashMap<Modality, usize>> { | ||
| Ok(HashMap::from([(Modality::Image, 1)])) | ||
| } | ||
| fn processor_kwargs(&self, _metadata: &ModelMetadata) -> RegistryResult<Value> { | ||
| Ok(Value::Null) | ||
| } | ||
| fn prompt_replacements( | ||
| &self, | ||
| _metadata: &ModelMetadata, | ||
| _preprocessed: &PreprocessedEncoderInputs, | ||
| ) -> RegistryResult<Vec<PromptReplacement>> { | ||
| Ok(Vec::new()) | ||
| } | ||
| fn rejected_image_formats(&self) -> &'static [image::ImageFormat] { | ||
| &[image::ImageFormat::Bmp] | ||
| } | ||
| } | ||
| let png = MediaContentPart::ImageUrl { | ||
| url: "https://a/x.png".to_string(), | ||
| detail: None, | ||
| uuid: None, | ||
| max_long_side_pixel: None, | ||
| }; | ||
| assert!(NoBmp | ||
| .validate_image_formats(std::slice::from_ref(&png)) | ||
| .is_ok()); | ||
| // Sniffed from the bytes (inline, or decoded from a data URL, before | ||
| // the client's media type is believed), from a data URL's type in | ||
| // any case, or from the URL path. | ||
| let data_url = |url: &str| MediaContentPart::ImageUrl { | ||
| url: url.to_string(), | ||
| detail: None, | ||
| uuid: None, | ||
| max_long_side_pixel: None, | ||
| }; | ||
| let rejected = [ | ||
| bmp, | ||
| MediaContentPart::ImageData { | ||
| data: b"zz".to_vec(), | ||
| mime_type: Some("IMAGE/BMP".to_string()), | ||
| uuid: None, | ||
| detail: None, | ||
| }, | ||
| data_url("data:image/bmp;base64,Qk0AAA=="), | ||
| data_url("data:image/BMP;base64,Qk0AAA=="), | ||
| // BMP bytes behind a lying media type. | ||
| data_url("data:image/png;base64,Qk0AAAAA"), | ||
| data_url("https://a/scan.BMP?x=1"), | ||
| ]; | ||
| for part in &rejected { | ||
| assert_eq!( | ||
| NoBmp.validate_image_formats(std::slice::from_ref(part)), | ||
| Err(ModelRegistryError::UnsupportedImageFormat { | ||
| spec: "no-bmp", | ||
| format: "bmp".to_string(), | ||
| }), | ||
| "{part:?}" | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn validation_accepts_any_declared_modality_subset() { | ||
| assert_eq!(validate(&TestSpec, &[(Modality::Image, 2)]), Ok(())); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,6 +11,17 @@ use validator; | |
| // Default value helpers | ||
| // ============================================================================ | ||
|
|
||
| /// A typed request as a `Value` with every number as the client wrote it. | ||
| /// `serde_json::to_value` stores an `f32` widened to `f64`, so `"top_p": | ||
| /// 0.95` would go upstream as `0.949999988079071`; the writer prints each | ||
| /// `f32` in its shortest round-trip form, so encode with it and parse that | ||
| /// back. Only typed `f32` fields are exact this way: numbers kept as | ||
| /// `Value` (an `other` map, nested values) were normalized at parse time | ||
| /// (`1e2` is `100.0`, `-0` is `-0.0`). | ||
| pub fn to_value_exact<T: Serialize>(value: &T) -> serde_json::Result<Value> { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Nit:
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 76bdde1: |
||
| serde_json::from_slice(&serde_json::to_vec(value)?) | ||
| } | ||
|
|
||
| /// Default model for endpoints where model is optional (e.g., /generate). | ||
| /// Uses UNKNOWN_MODEL_ID so routers treat it as "any available worker." | ||
| pub fn default_unknown_model() -> String { | ||
|
|
@@ -1079,6 +1090,26 @@ mod tests { | |
|
|
||
| use super::*; | ||
|
|
||
| #[test] | ||
| fn to_value_exact_keeps_f32_fields_as_the_client_wrote_them() { | ||
| let request: crate::chat::ChatCompletionRequest = serde_json::from_value(json!({ | ||
| "model": "glm-5.3-flash", | ||
| "messages": [{"role": "user", "content": "hi"}], | ||
| "top_p": 0.95, | ||
| "temperature": 0.7 | ||
| })) | ||
| .unwrap(); | ||
| // The widening this guards against: 0.95f32 is 0.949999988079071 as f64. | ||
| assert_ne!( | ||
| serde_json::to_value(&request).unwrap()["top_p"], | ||
| json!(0.95) | ||
| ); | ||
| let exact = to_value_exact(&request).unwrap(); | ||
| assert_eq!(exact["top_p"], json!(0.95)); | ||
| assert_eq!(exact["temperature"], json!(0.7)); | ||
| assert_eq!(exact["model"], json!("glm-5.3-flash")); | ||
| } | ||
|
|
||
| #[derive(Deserialize)] | ||
| struct NullableBoolTest { | ||
| #[serde(default, deserialize_with = "deserialize_null_as_false")] | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 Nit: This decodes the entire base64 payload just to sniff the magic bytes —
image::guess_formatonly reads the first few dozen bytes, but a multi-megabyte data-URL image gets a full transient copy allocated and thrown away at validation time, on every image part, before the image is decoded again downstream. Decoding a bounded prefix keeps the sniff O(1):(Cutting on a 4-char boundary keeps a prefix of valid standard base64 valid; slicing
as_bytes()also avoids astrslice panic on a malformed multi-byte payload.)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 3ac330c: the sniff decodes a bounded prefix (128 base64 characters, 96 bytes, whole quads), not the payload; a test case with a 4 KiB BMP data URL still sniffs as BMP.