-
-
Couldn't load subscription status.
- Fork 10.9k
[CLI][Doc] Formalize --mm-encoder-tp-mode
#23190
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
[CLI][Doc] Formalize --mm-encoder-tp-mode
#23190
Conversation
…oder-tp-mode` Signed-off-by: DarkLight1337 <tlleungac@connect.ust.hk>
| # DEPRECATED | ||
| enable_prompt_adapter: bool = False |
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.
Prompt adapters have been fully removed so I removed this unused code along the way.
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.
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.
Oops, thanks for fixing it
|
👋 Hi! Thank you for contributing to the vLLM project. 💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels. Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging. To run CI, PR reviewers can either: Add 🚀 |
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 refactors the --enable-multimodal-encoder-data-parallel argument to --mm-encoder-tp-mode for better clarity. The changes involve updating configuration classes, argument parsing, and model implementations to use the new argument. The old argument is correctly deprecated with backward compatibility support. However, there is a critical issue where the new mm_encoder_tp_mode value is not propagated from ModelConfig to MultiModalConfig, which will cause the feature to not work as intended. This needs to be fixed.
Signed-off-by: DarkLight1337 <tlleungac@connect.ust.hk>
Signed-off-by: DarkLight1337 <tlleungac@connect.ust.hk>
--enable-multimodal-encoder-data-parallel to --mm-encoder-tp-mode--mm-encoder-tp-mode
Signed-off-by: DarkLight1337 <tlleungac@connect.ust.hk> Signed-off-by: Duncan Moss <djm.moss@gmail.com>
Signed-off-by: DarkLight1337 <tlleungac@connect.ust.hk>
Signed-off-by: DarkLight1337 <tlleungac@connect.ust.hk>
Signed-off-by: DarkLight1337 <tlleungac@connect.ust.hk> Signed-off-by: Xiao Yu <xiao.yu@amd.com>
Signed-off-by: DarkLight1337 <tlleungac@connect.ust.hk>
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
Formalize, resolve #22743
To avoid confusion between
--enable-multimodal-encoder-data-paralleland--data-parallel-size(since it actually uses TP ranks), I have decided to rename this argument to--mm-encoder-tp-mode.I have also updated the Optimization docs to introduce this feature.
cc @jennyyyyzhen @houseroad #18368
cc @zzh142857 #22697
cc @tjtanaa #22742
cc @david6666666 #23168
cc @ywang96 @Isotr0py for review
Test Plan
Test Result
(Optional) Documentation Update
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.