-
-
Notifications
You must be signed in to change notification settings - Fork 11.8k
[Renderer] Separate out RendererConfig from ModelConfig
#30145
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: DarkLight1337 <tlleungac@connect.ust.hk>
RendererConfig from ModelConfig`RendererConfig from ModelConfig
|
Documentation preview: https://vllm--30145.org.readthedocs.build/en/30145/ |
|
/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 is a significant refactoring that separates renderer-specific configurations into a new RendererConfig class, moving them out of ModelConfig. The changes are extensive and touch many files, but they appear to be consistent with the goal of this refactoring. I've identified two critical issues in the new vllm/config/renderer.py file that could affect loading models and tokenizers from S3 URIs (a Run:ai feature). My review comments provide details and suggestions for fixing these issues.
|
/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 is a significant refactoring that separates renderer-specific configurations from ModelConfig into a new RendererConfig class. The changes are extensive and applied consistently across the codebase, improving separation of concerns. However, I've identified a critical issue in the recalculate_max_model_len method's signature which could lead to a TypeError during engine initialization if max_model_len is not explicitly provided by the user. The proposed fixes address this issue.
Signed-off-by: DarkLight1337 <tlleungac@connect.ust.hk>
|
/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 is a large but well-executed refactoring that separates renderer-specific configurations from the core model configuration into a new RendererConfig class. This improves code structure and separation of concerns. The changes are applied consistently across the codebase. I have found one potential issue that needs attention.
|
This pull request has merge conflicts that must be resolved before it can be |
Signed-off-by: DarkLight1337 <tlleungac@connect.ust.hk>
Signed-off-by: DarkLight1337 <tlleungac@connect.ust.hk>
RendererConfig from ModelConfigRendererConfig from ModelConfig
Signed-off-by: DarkLight1337 <tlleungac@connect.ust.hk>
Signed-off-by: DarkLight1337 <tlleungac@connect.ust.hk>
Signed-off-by: DarkLight1337 <tlleungac@connect.ust.hk>
Purpose
Move out renderer-specific fields into a new config.
ModelConfig.tokenizer -> RendererConfig.tokenizerModelConfig.tokenizer_mode -> RendererConfig.tokenizer_modeModelConfig.tokenizer_revision -> RendererConfig.tokenizer_revisionModelConfig.skip_tokenizer_init -> RendererConfig.skip_tokenizer_initModelConfig.io_processor_plugin -> RendererConfig.io_processor_pluginMultiModalConfig.media_io_kwargs -> RendererConfig.media_io_kwargsModelConfig.allowed_local_media_path -> RendererConfig.allowed_local_media_pathModelConfig.allowed_media_domains -> RendererConfig.allowed_media_domainsSince renderer may still need to access model config fields (like
hf_config),RendererConfigcontainsModelConfig. This also means a bunch of tests need to be updated to passRendererConfig(model_config=model_config)explicitly toVllmConfig.Related changes:
ModelConfig.maybe_pull_model_tokenizer_for_runaiintoModelConfig.maybe_pull_model_for_runaiandRendererConfig.maybe_pull_tokenizer_for_runai. Also fix the latter usingmodelinstead oftokenizer.ModelConfig.get_and_verify_max_lenintoModelConfig.recalculate_max_model_lenand call it twice during initialization: insideModelConfig.__post_init__without tokenizer, and afterRendererConfigis constructed with the tokenizer.renderer_configinstead ofmodel_config.renderer_configinstead ofmodel_config.SupportsTranscriptioninterface methods to acceptrenderer_configinstead ofmodel_config._HfExamplesInfo.build_model_configand_HfExamplesInfo.build_renderer_configconvenience methods for testing.Prepare for #22880
Test Plan
Test Result
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.