-
-
Notifications
You must be signed in to change notification settings - Fork 11.7k
[Bugfix][CI] Fix config resolving logic with remote models #27610
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
Conversation
Signed-off-by: Roger Wang <[email protected]>
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.
Code Review
This pull request aims to fix a regression for remote models that don't have an architectures field in their configuration by relaxing a strict check on model_type. While the intention is correct, the implementation introduces a new issue by using "AutoModel" as a dummy architecture. This will cause a runtime error because "AutoModel" is a real class in the transformers library that doesn't conform to vLLM's expectations for model classes. I've provided a critical comment with a suggested fix to use a unique placeholder name and also simplify the logic.
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Signed-off-by: Roger Wang <[email protected]>
|
/gemini review |
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.
Code Review
This pull request addresses a regression where remote models lacking an architectures field in their configuration would fail to load, even when the architecture was supplied through hf_overrides. The fix correctly relaxes a strict check, replacing a ValueError with a logger.warning. This allows the model loading to proceed so that the hf_overrides can be applied later. The change is well-targeted and effectively resolves the described issue.
|
Sorry I missed it, thanks for fixing |
Purpose
#27566 introduced a regression where remote models (e.g,
deepseek-ai/deepseek-vl2) without architectures will be not deploy-able even with--hf_overrides '{"architectures": ["DeepseekVLV2ForCausalLM"]}'on vLLM because of a strict model type check. This PR relaxes this check and adds a warning when this happens.This also fixes the failed
entrypoints/test_chat_utils.py::test_resolve_content_format_fallbacks[deepseek-ai/deepseek-vl2-tiny-string]CI test on main.Test Plan
Test Result
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.