Conversation
Shared gateway-local cache for config.json + preprocessor_config.json, used as the preload target for GetTokenizer bundles and the lookup surface for multimodal request handlers. Not yet wired into AppContext or MultimodalComponents; that happens in the following commits. Signed-off-by: key4ng <rukeyang@gmail.com>
Expose MultimodalConfigRegistry at AppContext so tokenizer registration (preload path) and request handlers (lookup path) can share one cache. Signed-off-by: key4ng <rukeyang@gmail.com>
Drop per-router model_configs DashMap from MultimodalComponents; hold an Arc<MultimodalConfigRegistry> reference instead. Multimodal helpers now take tokenizer_id + tokenizer_source (resolved once at the stage boundary) so the canonical cache key is the TokenizerEntry UUID, not the ambiguous request-time model_id. Signed-off-by: key4ng <rukeyang@gmail.com>
Read config.json + preprocessor_config.json from the extracted bundle tempdir before cleanup, and insert the parsed MultimodalModelConfig into AppContext.multimodal_config_registry keyed by tokenizer UUID. Fixes multimodal requests in IGW/K8s mode where the worker-reported tokenizer source is unreachable from the gateway. Signed-off-by: key4ng <rukeyang@gmail.com>
Asserts that an entry preloaded into MultimodalConfigRegistry under the tokenizer UUID is served to MultimodalComponents.config_registry lookups without falling through to tokenizer_source. Stops the original IGW bug from returning silently. Signed-off-by: key4ng <rukeyang@gmail.com>
Silence private_interfaces warnings: the registry is held in Arc on a pub AppContext field, but its methods don't need to be callable outside the model_gateway crate. Struct stays pub; methods become pub(crate). Signed-off-by: key4ng <rukeyang@gmail.com>
Nightly rustfmt required minor layout fixes to the tokenizer_registration.rs preload helpers and the multimodal.rs tests module. service_discovery.rs gained a `use` import for MultimodalConfigRegistry to silence the clippy::absolute_paths lint triggered by the long crate::-prefixed path. No behavior change. Signed-off-by: key4ng <rukeyang@gmail.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThis pull request introduces a shared Changes
Sequence Diagram(s)sequenceDiagram
participant Router as Multimodal<br/>Router
participant Prep as Preparation<br/>Stage
participant Registry as Multimodal<br/>Config Registry
participant TokenSrc as Tokenizer<br/>Source
participant Worker as Tokenizer<br/>Registration
Worker->>Registry: preload_config(tokenizer_id, config)
Note over Registry: Cache entry created
Router->>Prep: process_message(tokenizer_id, tokenizer_source)
Prep->>Registry: get_or_load(tokenizer_id, tokenizer_source)
alt Config in cache
Registry-->>Prep: return Arc<MultimodalModelConfig>
else Cache miss
Registry->>TokenSrc: resolve_model_config_dir(tokenizer_source)
TokenSrc-->>Registry: path
Registry->>TokenSrc: read config.json + preprocessor_config.json
TokenSrc-->>Registry: parsed config
Registry->>Registry: insert(tokenizer_id, config)
Registry-->>Prep: return Arc<MultimodalModelConfig>
end
Prep->>Prep: process_multimodal(tokenizer_id, config)
Prep-->>Router: processed_messages
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Code Review
This pull request introduces a centralized MultimodalConfigRegistry within the AppContext to manage and cache multimodal model configurations across the gateway. The changes update the tokenizer registration workflow to preload these configurations from bundles and refactor the multimodal processing pipeline to utilize the shared registry via tokenizer IDs. Review feedback highlights a potential cache stampede vulnerability in the registry's loading logic and identifies several instances of synchronous file I/O that should be converted to asynchronous or offloaded to prevent blocking the async runtime.
| pub(crate) async fn get_or_load( | ||
| &self, | ||
| model_id: &str, | ||
| tokenizer_id: &str, | ||
| tokenizer_source: &str, | ||
| ) -> Result<Arc<MultimodalModelConfig>> { |
There was a problem hiding this comment.
The get_or_load method is susceptible to a cache stampede (thundering herd) because multiple concurrent requests for the same tokenizer_id can simultaneously pass the cache check and trigger redundant loading operations. Consider using a synchronization mechanism (e.g., tokio::sync::OnceCell or an async-aware cache) to ensure that only one loading operation is performed per key.
| let config: serde_json::Value = std::fs::read_to_string(&config_path) | ||
| .with_context(|| format!("Failed to read config.json at {}", config_path.display())) |
| let preprocessor_config = std::fs::read_to_string(&pp_config_path) | ||
| .with_context(|| { |
| @@ -305,7 +371,19 @@ async fn fetch_tokenizer_from_worker( | |||
| ); | |||
|
|
|||
| match load_tokenizer_from_bundle(&bundle) { | |||
There was a problem hiding this comment.
load_tokenizer_from_bundle performs synchronous file IO and tokenizer creation, which blocks the async executor when called directly from fetch_tokenizer_from_worker. This should be wrapped in tokio::task::spawn_blocking to maintain runtime responsiveness. Note that data passed to spawned background tasks must have a 'static lifetime; use owned types or reference-counted pointers like Arc instead of passing references to ensure the data outlives the task.
References
- Data passed to spawned background tasks must have a 'static lifetime. Use owned types or reference-counted pointers like Arc instead of passing references to ensure the data outlives the task.
Description
Problem
In IGW/K8s mode the gateway loads tokenizers via
GetTokenizergRPC streaming. The bundle containsconfig.jsonandpreprocessor_config.json, but those files are extracted to a tempdir that is deleted immediately after the tokenizer is loaded into memory.When a multimodal request then arrives,
multimodal.rstries to read these files from disk using the tokenizer source — which in IGW is a worker-only path the gateway can't reach. The HF fallback also fails in air-gapped environments. Multimodal therefore breaks for any model whose tokenizer was loaded viaGetTokenizer.On top of that, the cache lived inside
MultimodalComponents(per-router, unreachable from tokenizer registration) and was keyed by the request-timemodel_id— which is ambiguous because callers may pass either a tokenizer name or a tokenizer UUID.Solution
MultimodalConfigRegistryowned byAppContext, keyed byTokenizerEntry.id(UUID).MultimodalComponentsnow holds anArcreference to the shared registry instead of its own cache.GetTokenizerbundle extraction now reads both JSON files out of the tempdir before cleanup and inserts the parsedMultimodalModelConfiginto the registry under the tokenizer UUID. Failure is non-fatal — text-only tokenizers legitimately have nopreprocessor_config.json.TokenizerEntryonce at the preparation stage boundary and threads bothtokenizer_idandtokenizer_sourceinto the multimodal helpers.Changes
model_gateway/src/routers/grpc/multimodal.rs— addMultimodalConfigRegistry; dropmodel_configsfromMultimodalComponents; route multimodal lookups through the shared registry keyed by tokenizer UUID.model_gateway/src/app_context.rs— newmultimodal_config_registry: Arc<MultimodalConfigRegistry>field initialized inAppContextBuilder::build().model_gateway/src/workflow/tokenizer_registration.rs— read and parse mm config files inside the bundle-extraction closure (before tempdir cleanup); insert into the registry under the tokenizer UUID.model_gateway/src/routers/grpc/router.rs+pd_router.rs— pass the shared registry intoMultimodalComponents::new.model_gateway/src/routers/grpc/regular/stages/chat/preparation.rs+messages/preparation.rs— resolveTokenizerEntryonce; passtokenizer_id+tokenizer_sourcedownstream.model_gateway/src/service_discovery.rs— update test fixture for the new field.Test Plan
cargo test -p smg --lib routers::grpc::multimodal::tests— 14/14 passing.cargo test -p smg --lib— full gateway library suite green.cargo check -p smg --all-targets— clean.rg 'get_or_load_config|model_configs' model_gateway/src— zero orphaned references.New tests (all in `model_gateway/src/routers/grpc/multimodal.rs`):
Checklist
Summary by CodeRabbit