Skip to content
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

[Bugfix] Fix prefix strings for quantized VLMs #9772

Merged
merged 7 commits into from
Oct 29, 2024

Conversation

mgoin
Copy link
Collaborator

@mgoin mgoin commented Oct 28, 2024

The prefix strings were not being constructed correctly for many VLMs, particularly due to the use of sub-models. This was causing quant_config to be enabled for modules that are ignored within the quantization_config of the model, causing failures during weight loading.

It's important to note that the "XForCausalLM" classes can be used as sub-models in VLMs (Gemma and Llama are easy examples), so we need to have the ability to optionally specify the prefix to those classes. For example in Llama you could have a module name model.layers.0.mlp.down_proj, but when it is used within a VLM that module name should be language_model.model.layers.0.mlp.down_proj
I propose a maybe_prefix(prefix, name) function that handles this logic.

I think in the future we should consider making prefix a required argument in many cases to make issues less likely, rather than a default empty string.

Tested models:

  • nm-testing/MiniCPM-V-2_6-FP8-dynamic
  • nm-testing/paligemma-3b-mix-224-FP8-dynamic
  • neuralmagic/Llama-3.2-11B-Vision-Instruct-FP8-dynamic
  • nm-testing/Qwen2-VL-7B-Instruct-FP8-dynamic
  • nm-testing/pixtral-12b-FP8-dynamic
  • nm-testing/Phi-3.5-vision-instruct-FP8-dynamic
  • nm-testing/llava-onevision-qwen2-7b-ov-hf-FP8-dynamic

Copy link

👋 Hi! Thank you for contributing to the vLLM project.
Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run fastcheck CI which starts running only a small and essential subset of CI tests to quickly catch errors. You can run other CI tests on top of those by going to your fastcheck build on Buildkite UI (linked in the PR checks section) and unblock them. If you do not have permission to unblock, ping simon-mo or khluu to add you in our Buildkite org.

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 do one of these:

  • Add ready label to the PR
  • Enable auto-merge.

🚀

@mgoin mgoin changed the title Fix prefix strings for quantized VLMs [Bugfix] Fix prefix strings for quantized VLMs Oct 29, 2024
@mgoin mgoin marked this pull request as ready for review October 29, 2024 15:29
@mgoin mgoin added the ready ONLY add when PR is ready to merge/full CI is needed label Oct 29, 2024
@DarkLight1337
Copy link
Member

DarkLight1337 commented Oct 29, 2024

Thanks for taking the time and effort for adding this! I think this should solve most of the quantization-related GH issues?

(Phi3V and Llava-OneVision haven't been tested though)

@mgoin
Copy link
Collaborator Author

mgoin commented Oct 29, 2024

@DarkLight1337 A few of the issues you linked (and shared earlier with me) are specific to bitsandbytes support, so will require more work but I want to tackle those in future PRs since BNB is special

Copy link
Member

@DarkLight1337 DarkLight1337 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM then. See if @Isotr0py is ok with this approach as well.

Copy link
Collaborator

@Isotr0py Isotr0py left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

BTW, if we want to broadcast the BNB support to most of VLMs, we might need to implement quantized multi_modal_proj (if they have) as well. Because user will possibly create a fully quantized (even the lm_head and multi_modal_proj) BNB model if they refer to the guidance in transformers...

@Isotr0py Isotr0py enabled auto-merge (squash) October 29, 2024 17:11
@simon-mo simon-mo merged commit bc73e98 into vllm-project:main Oct 29, 2024
45 of 48 checks passed
rasmith pushed a commit to rasmith/vllm that referenced this pull request Oct 30, 2024
NickLucche pushed a commit to NickLucche/vllm that referenced this pull request Oct 31, 2024
NickLucche pushed a commit to NickLucche/vllm that referenced this pull request Oct 31, 2024
lk-chen pushed a commit to lk-chen/vllm that referenced this pull request Nov 4, 2024
lk-chen pushed a commit to lk-chen/vllm that referenced this pull request Nov 4, 2024
JC1DA pushed a commit to JC1DA/vllm that referenced this pull request Nov 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready ONLY add when PR is ready to merge/full CI is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants