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

[Misc] Standardize RoPE handling for Qwen2-VL #9250

Merged
merged 14 commits into from
Oct 16, 2024
Merged

[Misc] Standardize RoPE handling for Qwen2-VL #9250

merged 14 commits into from
Oct 16, 2024
+102 −200

Conversation

DarkLight1337
Copy link
Member

@DarkLight1337 DarkLight1337 commented Oct 10, 2024 •
edited
Loading

huggingface/transformers#33401 has been fixed in Transformers v4.45.2, but the devs have clarified that M-ROPE is intended to be configured as rope_type="default". To avoid future compatibility issues, this PR updates the RoPE code in vLLM to follow their specification.

@github-actions GitHub Actions
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.

🚀

@DarkLight1337 DarkLight1337 changed the title [Misc] Update LoRA handling for Qwen2-VL to conform to Transformers [Misc] Update RoPE handling for Qwen2-VL to conform to Transformers Oct 10, 2024
DarkLight1337
DarkLight1337 commented Oct 10, 2024
View reviewed changes
vllm/model_executor/layers/rotary_embedding.py
Comment on lines -927 to -928
if scaling_type not in {"su", "longrope"}:
scaling_factor = rope_scaling.get("factor", 1.0)
Copy link
Member Author

Choose a reason for hiding this comment

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

It is not clear which RoPE implementations need this, so I've moved this code into the individual if blocks.

@DarkLight1337 DarkLight1337 changed the title [Misc] Update RoPE handling for Qwen2-VL to conform to Transformers [Misc] Standardize RoPE handling for Qwen2-VL Oct 10, 2024
@DarkLight1337 DarkLight1337 added the ready ONLY add when PR is ready to merge/full CI is needed label Oct 10, 2024
@DarkLight1337
Copy link
Member Author

@ywang96 PTAL when you have time.

@ZhangYaoFu
Copy link

Can mrope be completed using specialized CUDA operators? According to the profile results, this part is quite time-consuming.

@DarkLight1337
Copy link
Member Author

Can mrope be completed using specialized CUDA operators? According to the profile results, this part is quite time-consuming.

Can you open a new issue for this? Thanks

ywang96
ywang96 reviewed Oct 13, 2024
View reviewed changes
Copy link
Member

@ywang96 ywang96 left a comment

Choose a reason for hiding this comment

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

Left two comments - PTAL!

vllm/config.py Outdated Show resolved Hide resolved
vllm/config.py Show resolved Hide resolved
Isotr0py
Isotr0py approved these changes Oct 16, 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.

After testing on Qwen2-VL and Phi-3-medium, I'm fine with these changes since su and mrope are both handled with patching correctly.

@Isotr0py Isotr0py merged commit 7e7eae3 into main Oct 16, 2024
76 checks passed
@DarkLight1337 DarkLight1337 deleted the qwen2vl-lora branch October 16, 2024 05:57
@DarkLight1337 DarkLight1337 mentioned this pull request Oct 22, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ywang96 ywang96 ywang96 left review comments

@Isotr0py Isotr0py Isotr0py approved these changes

@WoosukKwon WoosukKwon Awaiting requested review from WoosukKwon WoosukKwon is a code owner

@tlrmchlsmth tlrmchlsmth Awaiting requested review from tlrmchlsmth tlrmchlsmth is a code owner

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.
4 participants
@DarkLight1337 @ZhangYaoFu @Isotr0py @ywang96