Repository navigation
feat(router): add --mm-per-request-image-limit override for spec image limits #2381
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 all commits
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 |
|---|---|---|
|
|
@@ -218,9 +218,25 @@ pub trait ModelProcessorSpec: Send + Sync { | |
| &self, | ||
| metadata: &ModelMetadata, | ||
| requested: &[(Modality, usize)], | ||
| ) -> RegistryResult<()> { | ||
| self.validate_media_request_with_limits(metadata, requested, &HashMap::new()) | ||
| } | ||
|
|
||
| /// [`Self::validate_media_request`] with caller-supplied per-modality limit | ||
| /// overrides; each replaces the spec limit and beats the env override. | ||
| fn validate_media_request_with_limits( | ||
|
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: The trait now has two validation entry points, and the only production caller ( Cheap ways to keep that from rotting: note in the |
||
| &self, | ||
| metadata: &ModelMetadata, | ||
| requested: &[(Modality, usize)], | ||
| limit_overrides: &HashMap<Modality, usize>, | ||
| ) -> RegistryResult<()> { | ||
| let limits = self.modality_limits(metadata)?; | ||
| check_media_counts(self.name(), &limits, requested, modality_limit_override) | ||
| check_media_counts(self.name(), &limits, requested, |modality| { | ||
| limit_overrides | ||
| .get(&modality) | ||
| .copied() | ||
| .or_else(|| modality_limit_override(modality)) | ||
| }) | ||
| } | ||
|
|
||
| fn processor_kwargs(&self, metadata: &ModelMetadata) -> RegistryResult<Value>; | ||
|
|
@@ -408,6 +424,55 @@ mod tests { | |
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn caller_limit_override_replaces_spec_limit() { | ||
| let tokenizer = TestTokenizer::new(&[]); | ||
| let config = json!({}); | ||
| let metadata = ModelMetadata { | ||
| model_id: "test-model", | ||
| tokenizer: &tokenizer, | ||
| config: &config, | ||
| }; | ||
| let overrides = HashMap::from([(Modality::Image, 5)]); | ||
|
|
||
| // TestSpec declares Image=2; the caller override wins in both directions. | ||
| assert_eq!( | ||
| TestSpec.validate_media_request_with_limits( | ||
| &metadata, | ||
| &[(Modality::Image, 5)], | ||
| &overrides | ||
| ), | ||
| Ok(()) | ||
| ); | ||
| assert_eq!( | ||
| TestSpec.validate_media_request_with_limits( | ||
| &metadata, | ||
| &[(Modality::Image, 6)], | ||
| &overrides | ||
| ), | ||
| Err(ModelRegistryError::ModalityLimitExceeded { | ||
| spec: "test", | ||
| modality: Modality::Image, | ||
| limit: 5, | ||
| requested: 6, | ||
| }) | ||
| ); | ||
| // Unoverridden modalities keep the spec limit. | ||
| assert_eq!( | ||
| TestSpec.validate_media_request_with_limits( | ||
| &metadata, | ||
| &[(Modality::Audio, 2)], | ||
| &overrides | ||
| ), | ||
| Err(ModelRegistryError::ModalityLimitExceeded { | ||
| spec: "test", | ||
| modality: Modality::Audio, | ||
| limit: 1, | ||
| requested: 2, | ||
| }) | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn limit_override_raises_declared_limit() { | ||
| let limits = HashMap::from([(Modality::Image, 2)]); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -557,6 +557,11 @@ struct CliArgs { | |
| #[arg(long, help_heading = "Multimodal")] | ||
| multimodal_shm_min_bytes: Option<usize>, | ||
|
|
||
| /// Per-request image-count limit applied to every model, replacing each | ||
| /// spec's built-in limit (e.g. to match the engine's `--limit-mm-per-prompt`). | ||
| #[arg(long, value_parser = clap::value_parser!(u64).range(1..), help_heading = "Multimodal")] | ||
| mm_per_request_image_limit: Option<u64>, | ||
|
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: Naming is off-pattern for this help group. The two sibling flags under Worth settling now rather than later — the name is baked into |
||
|
|
||
| // ==================== Service Discovery (Kubernetes) ==================== | ||
| /// Enable Kubernetes service discovery | ||
| #[arg( | ||
|
|
@@ -1816,6 +1821,7 @@ impl CliArgs { | |
| .engine_metrics(self.engine_metrics) | ||
| .multimodal_tensor_transport(self.multimodal_tensor_transport) | ||
| .multimodal_shm_min_bytes(self.multimodal_shm_min_bytes) | ||
| .mm_per_request_image_limit(self.mm_per_request_image_limit.map(|v| v as usize)) | ||
| .max_concurrent_requests(self.max_concurrent_requests) | ||
| .queue_size(self.queue_size) | ||
| .queue_timeout_secs(self.queue_timeout_secs) | ||
|
|
@@ -2689,6 +2695,8 @@ mod tests { | |
| "shm", | ||
| "--multimodal-shm-min-bytes", | ||
| "1024", | ||
| "--mm-per-request-image-limit", | ||
| "128", | ||
| ]); | ||
|
|
||
| let router_config = cli.to_router_config(vec![], vec![]).unwrap(); | ||
|
|
@@ -2698,6 +2706,7 @@ mod tests { | |
| "transport mode must reach RouterConfig via to_router_config" | ||
| ); | ||
| assert_eq!(router_config.multimodal_shm_min_bytes, Some(1024)); | ||
| assert_eq!(router_config.mm_per_request_image_limit, Some(128)); | ||
|
|
||
| let server_config = cli.to_server_config(router_config).unwrap(); | ||
| assert_eq!( | ||
|
|
@@ -2709,6 +2718,13 @@ mod tests { | |
| server_config.router_config.multimodal_shm_min_bytes, | ||
| Some(1024) | ||
| ); | ||
| assert_eq!( | ||
| server_config.router_config.mm_per_request_image_limit, | ||
| Some(128) | ||
| ); | ||
|
|
||
| // clap rejects a zero limit outright. | ||
| assert!(Cli::try_parse_from(["smg", "--mm-per-request-image-limit", "0"]).is_err()); | ||
| } | ||
|
|
||
| /// Default is off: the flag stays false through both conversions so | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,12 +1,12 @@ | ||
| //! Multimodal model configuration: the shared config-file registry and the | ||
| //! per-router component bundle (media connector + processor/model registries). | ||
|
|
||
| use std::{path::Path, sync::Arc}; | ||
| use std::{collections::HashMap, path::Path, sync::Arc}; | ||
|
|
||
| use anyhow::{Context, Result}; | ||
| use dashmap::DashMap; | ||
| use llm_multimodal::{ | ||
| MediaConnector, MediaConnectorConfig, ModelRegistry, PreProcessorConfig, | ||
| MediaConnector, MediaConnectorConfig, Modality, ModelRegistry, PreProcessorConfig, | ||
| VisionProcessorRegistry, | ||
| }; | ||
| use tracing::{debug, warn}; | ||
|
|
@@ -241,12 +241,17 @@ pub(crate) struct MultimodalComponents { | |
| pub config_registry: Arc<MultimodalConfigRegistry>, | ||
| /// Optional host-DRAM cache of preprocessed per-image encoder inputs. | ||
| pub pixel_cache: Option<Arc<PixelCache>>, | ||
| /// Router-configured per-modality media-count limits replacing spec limits. | ||
| pub modality_limit_overrides: HashMap<Modality, usize>, | ||
| } | ||
|
|
||
| impl MultimodalComponents { | ||
| /// Create multimodal components with default registries and a reference | ||
| /// to the shared `MultimodalConfigRegistry` owned by `AppContext`. | ||
| pub fn new(config_registry: Arc<MultimodalConfigRegistry>) -> Result<Self> { | ||
| pub fn new( | ||
| config_registry: Arc<MultimodalConfigRegistry>, | ||
| image_limit_override: Option<usize>, | ||
| ) -> Result<Self> { | ||
| let client = reqwest::Client::builder() | ||
| .timeout(std::time::Duration::from_secs(30)) | ||
| .build() | ||
|
|
@@ -260,6 +265,9 @@ impl MultimodalComponents { | |
| model_registry: Arc::new(ModelRegistry::default()), | ||
| config_registry, | ||
| pixel_cache: pixel_cache_from_env(), | ||
| modality_limit_overrides: image_limit_override | ||
|
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: This
|
||
| .map(|limit| HashMap::from([(Modality::Image, limit)])) | ||
| .unwrap_or_default(), | ||
| }) | ||
| } | ||
| } | ||
|
|
||
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: The help text promises "Must be >= 1", but
type=intaccepts0and negatives, so the two entry points reject bad input at different places with different messages:claprange(1..)→ clean parse-time error naming the flag.0: accepted by argparse, accepted by the pyo3 constructor, and only rejected atRouter.start()asConfiguration validation failed: ....-1:OverflowErrorout of the pyo3Option<usize>extraction at_Router(**args_dict).Both do fail loudly, so nothing is silently wrong — but a small
type=validator would give the Python path the same parse-time rejection as the Rust one: