-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
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
[Model]Refactor MiniCPMV #7020
[Model]Refactor MiniCPMV #7020
Conversation
👋 Hi! Thank you for contributing to the vLLM project. Once the PR is approved and ready to go, please make sure to run full CI as it is required to merge (or just use auto-merge). To run full CI, you can do one of these:
🚀 |
Hey @jeejeelee Thanks for the contribution! Were you able to verify if the model works on TP=1,2,4,8? (I will verify this myself later, but I was curious if this PR is ready for testing) Also I'm curious if you have seen any significant speedup by sharding the ViT |
Just verified on TP=1,2,4 . Currently, I do not have available 8-gpu resources and have not yet verified TP=8 |
This will cause significant merge conflicts with #6995. Would it be better for you to incorporate my changes into your PR instead of releasing them separately? |
I agree with this too. |
Ok, I will incorporate your changes ASAP |
@ywang96 I have not yet tested the speedup effect of TP, but I will provide test results once available. |
@DarkLight1337 It seems #6995 has a bug. After incorporating your changes, the generated results have become poor. |
Try reverting the lines where |
I have figured it out, this snippet has a bug, and have fixed it |
My bad, I forgot to rename the intermediate variables. Thanks for fixing this! To avoid confusion, I have closed the other PR. |
Thanks, could you help review my implementation? I want to complete this PR asap. My final goal is actually to make minicpmv2.5 support LoRA |
Sry for late. Really appreciate your contribution! I'll check these modifications. |
I think it's truly great to have |
@DarkLight1337 After updating the latest changes, I'm still encountering errors [rank0]: File "/mypath/vllm/vllm/model_executor/models/minicpmv.py", line 588, in _parse_and_validate_inputs
[rank0]: raise ValueError(f"Inconsistent flattened lengths, found: {lens}")
[rank0]: ValueError: Inconsistent flattened lengths, found: [0, 16, 16] |
@DarkLight1337 @HwwwwwwwH It's getting late here, so I'll log off for now. Thank you for all your hard work |
Sure, sorry for interfering with your own testing... |
After some offline discussion with @HwwwwwwwH , apparently the dummy data doesn't contain image tokens while providing the image. I have updated the validation to allow this for now, we will revisit the dummy data generation in a later PR. |
Thank you too. Good night! |
@jeejeelee Hi there. Could you run the model correctly now? I got some problems. |
Me too. |
@DarkLight1337 Thank you for your hard work. It works fine for me now. |
Hello @jeejeelee! Just a follow-up question: are you interested in implementing Idefics3 eventually? (It's not available on |
I'd be happy to implement this, but I might not be able to start working on it until next week. |
@jeejeelee No rush at all and thank you for the interest. Most likely we'll have to wait for the |
Co-authored-by: Cyrus Leung <[email protected]>
Co-authored-by: Cyrus Leung <[email protected]>
Co-authored-by: Cyrus Leung <[email protected]> Signed-off-by: Alvant <[email protected]>
I have completed the following modification:
Idefics2VisionTransformer
ping @ywang96 @DarkLight1337 @HwwwwwwwH