Skip to content

[BugFix] llama4 qknorm should be not shared across head #16311

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
Merged
vllm-bot merged 3 commits into from
Apr 9, 2025
Merged

[BugFix] llama4 qknorm should be not shared across head #16311

vllm-bot merged 3 commits into from
Apr 9, 2025

Conversation

@luccafong
Copy link
Collaborator

@luccafong luccafong commented Apr 9, 2025 •
edited by github-actions bot
Loading

QKNorm should not be across head, we need to do qknorm per head, score goes up from 0.6858 to 0.7153.
This also resovles TP=4 Accuracy Issue #16300

MMLU Pro Before fix:

|       Tasks       |Version|    Filter    |n-shot|  Metric   |   |Value |   |Stderr|
|-------------------|------:|--------------|-----:|-----------|---|-----:|---|-----:|
|mmlu_pro           |    2.0|custom-extract|      |exact_match|↑  |0.6858|±  |0.0041|
| - biology         |    2.1|custom-extract|     5|exact_match|↑  |0.8215|±  |0.0143|
| - business        |    2.1|custom-extract|     5|exact_match|↑  |0.7414|±  |0.0156|
| - chemistry       |    2.1|custom-extract|     5|exact_match|↑  |0.7297|±  |0.0132|
| - computer_science|    2.1|custom-extract|     5|exact_match|↑  |0.6366|±  |0.0238|
| - economics       |    2.1|custom-extract|     5|exact_match|↑  |0.7879|±  |0.0141|
| - engineering     |    2.1|custom-extract|     5|exact_match|↑  |0.5810|±  |0.0159|
| - health          |    2.1|custom-extract|     5|exact_match|↑  |0.7029|±  |0.0160|
| - history         |    2.1|custom-extract|     5|exact_match|↑  |0.5879|±  |0.0252|
| - law             |    2.1|custom-extract|     5|exact_match|↑  |0.4478|±  |0.0150|
| - math            |    2.1|custom-extract|     5|exact_match|↑  |0.7698|±  |0.0115|
| - other           |    2.1|custom-extract|     5|exact_match|↑  |0.6418|±  |0.0158|
| - philosophy      |    2.1|custom-extract|     5|exact_match|↑  |0.5130|±  |0.0224|
| - physics         |    2.1|custom-extract|     5|exact_match|↑  |0.7537|±  |0.0120|
| - psychology      |    2.1|custom-extract|     5|exact_match|↑  |0.7556|±  |0.0152|

MMLU Pro After fix:

|       Tasks       |Version|    Filter    |n-shot|  Metric   |   |Value |   |Stderr|
|-------------------|------:|--------------|-----:|-----------|---|-----:|---|-----:|
|mmlu_pro           |    2.0|custom-extract|      |exact_match|↑  |0.7153|±  |0.0040|
| - biology         |    2.1|custom-extract|     5|exact_match|↑  |0.6974|±  |0.0172|
| - business        |    2.1|custom-extract|     5|exact_match|↑  |0.7693|±  |0.0150|
| - chemistry       |    2.1|custom-extract|     5|exact_match|↑  |0.7756|±  |0.0124|
| - computer_science|    2.1|custom-extract|     5|exact_match|↑  |0.6951|±  |0.0228|
| - economics       |    2.1|custom-extract|     5|exact_match|↑  |0.8211|±  |0.0132|
| - engineering     |    2.1|custom-extract|     5|exact_match|↑  |0.6378|±  |0.0154|
| - health          |    2.1|custom-extract|     5|exact_match|↑  |0.7274|±  |0.0156|
| - history         |    2.1|custom-extract|     5|exact_match|↑  |0.5932|±  |0.0252|
| - law             |    2.1|custom-extract|     5|exact_match|↑  |0.4741|±  |0.0151|
| - math            |    2.1|custom-extract|     5|exact_match|↑  |0.8150|±  |0.0106|
| - other           |    2.1|custom-extract|     5|exact_match|↑  |0.7089|±  |0.0150|
| - philosophy      |    2.1|custom-extract|     5|exact_match|↑  |0.5992|±  |0.0220|
| - physics         |    2.1|custom-extract|     5|exact_match|↑  |0.7791|±  |0.0115|
| - psychology      |    2.1|custom-extract|     5|exact_match|↑  |0.7707|±  |0.0149|
@luccafong
llama4 qknorm should be not shared across head
85e5412 
Signed-off-by: Lu Fang <fanglu@fb.com>
@luccafong luccafong changed the title [Bugfix] llama4 qknorm should be not shared across head Apr 9, 2025
@github-actions
Copy link

github-actions bot commented Apr 9, 2025

👋 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 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 either: Add ready label to the PR or enable auto-merge.

🚀
@luccafong luccafong changed the title [Bug] llama4 qknorm should be not shared across head Apr 9, 2025
ywang96
ywang96 reviewed Apr 9, 2025
View reviewed changes
@yeqcharlotte
Copy link
Collaborator

yeqcharlotte commented Apr 9, 2025

Goooood catch!
yeqcharlotte
yeqcharlotte reviewed Apr 9, 2025
View reviewed changes
houseroad
houseroad reviewed Apr 9, 2025
View reviewed changes
luccafong added 2 commits April 8, 2025 22:23
@luccafong
better format
4ef6157 
Signed-off-by: Lu Fang <fanglu@fb.com>
@luccafong
fix and merge qknorm
21a54ae 
Signed-off-by: Lu Fang <fanglu@fb.com>
@luccafong luccafong marked this pull request as ready for review April 9, 2025 05:31
@luccafong luccafong requested a review from ywang96 April 9, 2025 05:35
@yeqcharlotte yeqcharlotte mentioned this pull request Apr 9, 2025
Closed
1 task
houseroad
houseroad approved these changes Apr 9, 2025
View reviewed changes
Copy link
Collaborator

@houseroad houseroad left a comment

Choose a reason for hiding this comment

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

Looks good.
@houseroad houseroad added the ready ONLY add when PR is ready to merge/full CI is needed label Apr 9, 2025
@vllm-bot vllm-bot merged commit ec7da6f into vllm-project:main Apr 9, 2025
53 of 59 checks passed
zRzRzRzRzRzRzR pushed a commit to zRzRzRzRzRzRzR/vllm that referenced this pull request Apr 9, 2025
@luccafong @zRzRzRzRzRzRzR
[BugFix] llama4 qknorm should be not shared across head (vllm-project…
638e304 
…#16311)

Signed-off-by: Lu Fang <fanglu@fb.com>
Signed-off-by: zRzRzRzRzRzRzR <2448370773@qq.com>
@houseroad houseroad mentioned this pull request Apr 9, 2025
Closed
19 tasks
@sarckk sarckk mentioned this pull request Apr 11, 2025
Closed
1 task
yangw-dev pushed a commit to yangw-dev/vllm that referenced this pull request Apr 21, 2025
@luccafong @yangw-dev
[BugFix] llama4 qknorm should be not shared across head (vllm-project…
d683964 
…#16311)

Signed-off-by: Lu Fang <fanglu@fb.com>
Signed-off-by: Yang Wang <elainewy@meta.com>
jikunshang pushed a commit to jikunshang/vllm that referenced this pull request Apr 29, 2025
@luccafong @dbyoung18
[BugFix] llama4 qknorm should be not shared across head (vllm-project…
a6f6139 
…#16311)

Signed-off-by: Lu Fang <fanglu@fb.com>
lk-chen pushed a commit to lk-chen/vllm that referenced this pull request Apr 29, 2025
@luccafong @lk-chen
[BugFix] llama4 qknorm should be not shared across head (vllm-project…
e93a73b 
…#16311)

Signed-off-by: Lu Fang <fanglu@fb.com>
RichardoMrMu pushed a commit to RichardoMrMu/vllm that referenced this pull request May 12, 2025
@luccafong
[BugFix] llama4 qknorm should be not shared across head (vllm-project…
2c19c03 
…#16311)

Signed-off-by: Lu Fang <fanglu@fb.com>
Signed-off-by: Mu Huai <tianbowen.tbw@antgroup.com>
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

5 participants

@luccafong @yeqcharlotte @houseroad @ywang96 @vllm-bot