fix(multimodal): fall back to config.model_type for aliased model IDs - #898
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
github-actions
Bot
added
grpc
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughAdded a config-based fallback for multimodal family detection: ModelMetadata gains Changes
Sequence DiagramsequenceDiagram
participant Client as gRPC Client
participant Handler as Multimodal Handler
participant Config as Config Loader
participant Registry as Spec Registry
participant Processor as Image Processor Registry
Client->>Handler: Request with custom model_id
Handler->>Config: Load tokenizer/config (config.json)
Config-->>Handler: config (may include model_type)
Handler->>Registry: lookup(metadata with model_id + config_model_type)
rect rgba(100, 150, 255, 0.5)
Note over Registry: Spec Matching
Registry->>Registry: Try match by model_id
alt model_id matches
Registry-->>Handler: Return spec
else no model_id match
Registry->>Registry: Try match by config_model_type
Registry-->>Handler: Return spec (if any)
end
end
Handler->>Processor: find(model_id, model_type)
rect rgba(150, 200, 150, 0.5)
Note over Processor: Processor Selection
Processor->>Processor: Search by model_id
alt model_id matches
Processor-->>Handler: Return processor
else no model_id match
Processor->>Processor: Fallback to model_type search
Processor-->>Handler: Return processor (if any)
end
end
Handler-->>Client: Proceed with multimodal processing / error if none found
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly Related PRs
Suggested Labels
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
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 resolves an issue where multimodal vision models could not be correctly identified when served under custom or aliased names. The core solution involves implementing a fallback mechanism that consults the Highlights
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. Footnotes
|
![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 enhances model identification by introducing a fallback mechanism to use the model_type from model configurations when the model_id doesn't directly match. This new logic is applied to various model specifications (Llama4, Llava, Phi3-V, Qwen3-VL, Qwen-VL) and the ImageProcessorRegistry, with corresponding updates in the model_gateway and new tests to validate the fallback. However, there are two high-severity issues identified: the LlavaSpec's prompt_replacements incorrectly calculates image tokens for LLaVA-NeXT models, and the Phi3VisionSpec's prompt_replacements might inaccurately recalculate image tokens, both of which should be updated to use the preprocessed.num_img_tokens for correctness and consistency.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Multimodal family detection failed when a vision model was served under a custom name (e.g. "custom-model") because both the model spec registry and image processor registry matched exclusively on the request-side model_id string. Add a config.model_type fallback to each spec's matches() method and to ImageProcessorRegistry::find(). The existing model_id substring check remains the fast path; config.model_type is only consulted when model_id does not match any known family. Unlike substring matching, model_type uses exact equality since it is a known fixed value from config.json. Also removes unused has_processor() from ImageProcessorRegistry. Fixes #754 Signed-off-by: Chang Su <chang.s.su@oracle.com>
97beee8 to
5ce2178
Compare
1 participant
Description
Problem
Multimodal family detection fails when a vision model is served under a custom/aliased name (e.g.
custom-model) because both the model spec registry and image processor registry match exclusively on the request-sidemodel_idstring. Even thoughconfig.jsonandpreprocessor_config.jsonare loaded successfully from the real model path, the family detection never consultsconfig.model_type.Solution
Add a
config.model_typefallback to each spec'smatches()method and toImageProcessorRegistry::find(). The existingmodel_idsubstring check remains the fast path;config.model_typeis only consulted whenmodel_iddoes not match any known family. Unlike themodel_idpath which uses substring matching,model_typeuses exact equality since it is a known fixed value fromconfig.json.Changes
config_model_type()helper toModelMetadatafor readingconfig.json'smodel_typefieldmatches()to fall back to exactconfig.model_typematching whenmodel_idsubstring check fails (qwen3_vl, qwen_vl, llava, llama4, phi3_v)ImageProcessorRegistry::find()to accept an optionalmodel_typeparameter and fall back to it whenmodel_iddoesn't matchphi3_vpattern in image processor defaults so model_type fallback can match itmodel_typefrom loaded config through the caller inmultimodal.rshas_processor()fromImageProcessorRegistrySupersedes #755
Fixes #754
Test Plan
cargo test -p llm-multimodal— all 220 tests passcargo check -p smg— gateway compiles cleanlycustom-modelChecklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspassesSummary by CodeRabbit
Fixes #754