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.

Sign up for GitHub

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

Jump to bottom

[Bugfix] Fix MiniCPMV and Mllama BNB bug #9917

Merged
merged 7 commits into from
Nov 4, 2024
Merged

[Bugfix] Fix MiniCPMV and Mllama BNB bug #9917

merged 7 commits into from
Nov 4, 2024

Conversation

jeejeelee
Copy link
Collaborator

@jeejeelee jeejeelee commented Nov 1, 2024 •
edited
Loading

FILL IN THE PR DESCRIPTION HERE

FIX #9914

ping @mgoin

cc @chenqianfzh as well. Regarding multimodal models, additional BNB implementation logic may be required

@jeejeelee
Fix minicpmv BNB bug
bc999e0 
Signed-off-by: Jee Jee Li <[email protected]>
@jeejeelee
Fix minicpmv BNB bug
da40760 
Signed-off-by: Jee Jee Li <[email protected]>
@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 1, 2024

👋 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.

🚀

@jeejeelee
Copy link
Collaborator Author

Now, after fixing this, I can generate reasonable results using my local image

@jeejeelee
Copy link
Collaborator Author

Hmm， It seems that BNB has issues handling weights in ReplicatedLinear when TP > 1 @chenqianfzh

@jeejeelee
Add prefix arg
59cb7be 
Signed-off-by: Jee Jee Li <[email protected]>
@jeejeelee jeejeelee requested a review from mgoin November 1, 2024 15:29
@jeejeelee
Fix mllama BNB bug
5fadcf9 
Signed-off-by: Jee Jee Li <[email protected]>
@jeejeelee jeejeelee changed the title [Bugfix] Fix MiniCPMV BNB bug [Bugfix] Fix MiniCPMV and Mllama BNB bug Nov 1, 2024
@mgoin mgoin added the ready ONLY add when PR is ready to merge/full CI is needed label Nov 2, 2024
@jeejeelee
Fix BNB Bug
a12c16c 
Signed-off-by: Jee Jee Li <[email protected]>
@jeejeelee jeejeelee force-pushed the minicpmv-bnb-support branch from c0264e6 to a12c16c Compare November 2, 2024 16:52
@jeejeelee
Copy link
Collaborator Author

Hmm， It seems that BNB has issues handling weights in ReplicatedLinear when TP > 1 @chenqianfzh

I have handled this issue, @mgoin please review it again，thanks~

mgoin
mgoin approved these changes Nov 2, 2024
View reviewed changes
Copy link
Member

@mgoin mgoin left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me thanks, just curious about two things

vllm/model_executor/model_loader/loader.py Outdated Show resolved Hide resolved
vllm/model_executor/model_loader/loader.py
@@ -1005,16 +1007,21 @@ def _unquantized_generator(self, hf_weights_files, use_safetensors,
if any(target_module in weight_name for target_module in
self.target_modules) and weight_name.endswith(".weight"):
weight_name = weight_name.replace(".weight", ".qweight")
Copy link
Member

Choose a reason for hiding this comment

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

A general questions I have about BNB is why do we use .qweight in the BNBLinearMethod but the model checkpoints actually use .weight? It seems we could avoid some logic by having the quant method directly use .weight
Screenshot 2024-11-02 at 2 14 23 PM

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Perhaps it's to maintain consistency with other quantization algorithms like GPTQ implementations see:

vllm/vllm/model_executor/layers/quantization/gptq.py

Line 207 in 74b529c

layer.register_parameter("qweight", qweight)

Copy link
Member

Choose a reason for hiding this comment

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

Okay, let's try to remove this in the future if possible! The "consistency" is just coincidental - we usually aim for parameters to have the same name as in the checkpoint format to make weight loading simple

@jeejeelee jeejeelee requested a review from mgoin November 3, 2024 01:29
@jeejeelee jeejeelee force-pushed the minicpmv-bnb-support branch from efe89d8 to e410d9c Compare November 4, 2024 02:13
Isotr0py
Isotr0py approved these changes Nov 4, 2024
View reviewed changes
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 too!

@Isotr0py Isotr0py enabled auto-merge (squash) November 4, 2024 02:58
@Isotr0py Isotr0py merged commit c49f040 into vllm-project:main Nov 4, 2024
62 checks passed
@jeejeelee jeejeelee deleted the minicpmv-bnb-support branch November 4, 2024 03:37
@jeejeelee jeejeelee mentioned this pull request Nov 4, 2024
Merged
lk-chen pushed a commit to lk-chen/vllm that referenced this pull request Nov 4, 2024
@jeejeelee
[Bugfix] Fix MiniCPMV and Mllama BNB bug (vllm-project#9917)
bb9ea8f 
Signed-off-by: Jee Jee Li <[email protected]>
lk-chen pushed a commit to lk-chen/vllm that referenced this pull request Nov 4, 2024
@jeejeelee
[Bugfix] Fix MiniCPMV and Mllama BNB bug (vllm-project#9917)
32abbd6 
Signed-off-by: Jee Jee Li <[email protected]>
Signed-off-by: Linkun Chen <[email protected]>
richardsliu pushed a commit to richardsliu/vllm that referenced this pull request Nov 4, 2024
@jeejeelee @richardsliu
[Bugfix] Fix MiniCPMV and Mllama BNB bug (vllm-project#9917)
2623641 
Signed-off-by: Jee Jee Li <[email protected]>
Signed-off-by: Richard Liu <[email protected]>
bigPYJ1151 pushed a commit to bigPYJ1151/vllm that referenced this pull request Nov 5, 2024
@jeejeelee @bigPYJ1151
[Bugfix] Fix MiniCPMV and Mllama BNB bug (vllm-project#9917)
7eaf83f 
Signed-off-by: Jee Jee Li <[email protected]>
DarkLight1337 pushed a commit that referenced this pull request Nov 5, 2024
@jeejeelee @DarkLight1337
[Bugfix] Fix MiniCPMV and Mllama BNB bug (#9917)
f78d688 
Signed-off-by: Jee Jee Li <[email protected]>
hissu-hyvarinen pushed a commit to ROCm/vllm that referenced this pull request Nov 6, 2024
@jeejeelee @hissu-hyvarinen
[Bugfix] Fix MiniCPMV and Mllama BNB bug (vllm-project#9917)
af0dfcc 
Signed-off-by: Jee Jee Li <[email protected]>
JC1DA pushed a commit to JC1DA/vllm that referenced this pull request Nov 11, 2024
@jeejeelee @JC1DA
[Bugfix] Fix MiniCPMV and Mllama BNB bug (vllm-project#9917)
1f16e6b 
Signed-off-by: Jee Jee Li <[email protected]>
Signed-off-by: Loc Huynh <[email protected]>
sumitd2 pushed a commit to sumitd2/vllm that referenced this pull request Nov 14, 2024
@jeejeelee @sumitd2
[Bugfix] Fix MiniCPMV and Mllama BNB bug (vllm-project#9917)
e11c515 
Signed-off-by: Jee Jee Li <[email protected]>
Signed-off-by: Sumit Dubey <[email protected]>
KuntaiDu pushed a commit to KuntaiDu/vllm that referenced this pull request Nov 20, 2024
@jeejeelee
[Bugfix] Fix MiniCPMV and Mllama BNB bug (vllm-project#9917)
962585b 
Signed-off-by: Jee Jee Li <[email protected]>
mfournioux pushed a commit to mfournioux/vllm that referenced this pull request Nov 20, 2024
@jeejeelee @mfournioux
[Bugfix] Fix MiniCPMV and Mllama BNB bug (vllm-project#9917)
0757214 
Signed-off-by: Jee Jee Li <[email protected]>
Signed-off-by: Maxime Fournioux <[email protected]>
tlrmchlsmth pushed a commit to neuralmagic/vllm that referenced this pull request Nov 23, 2024
@jeejeelee @tlrmchlsmth
[Bugfix] Fix MiniCPMV and Mllama BNB bug (vllm-project#9917)
449e4e8 
Signed-off-by: Jee Jee Li <[email protected]>
Signed-off-by: Tyler Michael Smith <[email protected]>
sleepwalker2017 pushed a commit to sleepwalker2017/vllm that referenced this pull request Dec 13, 2024
@jeejeelee
[Bugfix] Fix MiniCPMV and Mllama BNB bug (vllm-project#9917)
298e4d7 
Signed-off-by: Jee Jee Li <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Isotr0py Isotr0py Isotr0py approved these changes

@mgoin mgoin Awaiting requested review from mgoin

Assignees
No one assigned
Labels
ready ONLY add when PR is ready to merge/full CI is needed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug]: minicpmv2.6 BNB in-flight quantization error
3 participants
@jeejeelee @mgoin @Isotr0py