Repository navigation
fix(grpc_servicer): return served_model_name in vLLM GetModelInfo response - #727
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. |
📝 WalkthroughWalkthroughGetModelInfo now includes a new response field Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 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)
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 addresses an issue where the vLLM backend, when integrated with the smg gateway, incorrectly reported its model identifier as the filesystem path rather than the intended served_model_name. By adjusting the GetModelInfo response, the gateway will now correctly register the worker using the user-defined model name, improving clarity and consistency in model identification. 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
|
There was a problem hiding this comment.
Code Review
This pull request addresses a bug where the vLLM gRPC servicer would incorrectly return the model's filesystem path instead of the user-provided served-model-name. The fix correctly prioritizes served_model_name in the GetModelInfo response, falling back to the model path, which ensures that the gateway registers the worker with the intended model identifier. The change is correct and aligns with the problem description.
…ponse The gRPC servicer was returning only model_config.model in the model_path field, with no served_model_name. This caused smg to register workers with the raw filesystem path instead of the user-specified --served-model-name. Add a served_model_name field to the vLLM GetModelInfoResponse proto and populate it from model_config.served_model_name. This keeps model_path as the filesystem path (needed for tokenizer loading) while providing the served name separately for model_id resolution. Signed-off-by: Chang Su <chang.s.su@oracle.com>
03a36d7 to
a90f492
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
grpc_servicer/smg_grpc_servicer/vllm/servicer.py (1)
268-274:⚠️ Potential issue | 🔴 CriticalUpdate
model_pathtoo, or the gateway bug remains.Line 269 still returns
model_config.modelformodel_path, so existingGetModelInfoconsumers will keep seeing the filesystem path. Addingserved_model_namealongside it does not fix the registration flow described in this PR.Suggested fix
+ served_model_name = model_config.served_model_name or model_config.model return vllm_engine_pb2.GetModelInfoResponse( - model_path=model_config.model, + model_path=served_model_name, is_generation=model_config.runner_type == "generate", max_context_length=model_config.max_model_len, vocab_size=model_config.get_vocab_size(), supports_vision=model_config.is_multimodal_model, - served_model_name=model_config.served_model_name or model_config.model, + served_model_name=served_model_name, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@grpc_servicer/smg_grpc_servicer/vllm/servicer.py` around lines 268 - 274, The GetModelInfoResponse is still returning the filesystem path via model_config.model for the model_path field, so update the model_path assignment to prefer model_config.served_model_name (falling back to model_config.model) so consumers receive the served model name; change the model_path in the GetModelInfoResponse construction (in the GetModelInfoResponse return block) to something like served_model_name or model as a fallback using model_config.served_model_name or model_config.model, keeping served_model_name logic for served_model_name unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@grpc_servicer/smg_grpc_servicer/vllm/servicer.py`:
- Around line 268-274: The GetModelInfoResponse is still returning the
filesystem path via model_config.model for the model_path field, so update the
model_path assignment to prefer model_config.served_model_name (falling back to
model_config.model) so consumers receive the served model name; change the
model_path in the GetModelInfoResponse construction (in the GetModelInfoResponse
return block) to something like served_model_name or model as a fallback using
model_config.served_model_name or model_config.model, keeping served_model_name
logic for served_model_name unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3144910a-0867-4fa1-91bc-1e886a6a5a73
📒 Files selected for processing (2)
crates/grpc_client/proto/vllm_engine.protogrpc_servicer/smg_grpc_servicer/vllm/servicer.py
Description
Problem
When using
smg serve --backend vllm --served-model-name vllm-model, the gateway registers the worker with the raw filesystem model path (e.g./raid/models/...) instead of the user-specified--served-model-name.This happens because the vLLM gRPC servicer returns
model_config.model(always the filesystem path) in theGetModelInforesponse, ignoring theserved_model_namethat vLLM sets when--served-model-nameis provided.Solution
Use
model_config.served_model_name(which vLLM sets viaget_served_model_name()— falls back tomodel_config.modelwhen not specified) instead ofmodel_config.modelin theGetModelInforesponse.Changes
grpc_servicer/smg_grpc_servicer/vllm/servicer.py: Returnmodel_config.served_model_name or model_config.modelinmodel_pathfield ofGetModelInfoResponseTest Plan
--served-model-name vllm-modelvllm-modelinstead of the filesystem pathBefore

After


Checklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspassesSummary by CodeRabbit