Skip to content

[CI Failure] Fix Gemma3 RoPE configuration for sliding attention layers #29111

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
Nov 21, 2025
Merged

[CI Failure] Fix Gemma3 RoPE configuration for sliding attention layers #29111

vllm-bot merged 3 commits into from
Nov 21, 2025

Conversation

@hl475
Copy link
Contributor

@hl475 hl475 commented Nov 20, 2025 •
edited by github-actions bot
Loading

Purpose

#28542 introduced a regression in Gemma3 model that caused incorrect perplexity scores. The test test_gemma.py::test_ppl[model_info2] failed https://buildkite.com/vllm/ci/builds/39860/steps/table?jid=019aa011-0d25-4a55-9e34-b7caef1ee9df . Per investigation, we should use default RoPE with rope_local_base_freq (10000.0) and NO scaling if self.is_sliding

This PR changes if self.is_sliding branch to properly configure sliding attention layers.

Test Plan

pytest -v -s tests/models/language/generation_ppl_test/test_gemma.py::test_ppl[model_info2]

Test Result

1 passed, 2 warnings in 74.94s (0:01:14)
Essential Elements of an Effective PR Description Checklist
  • The purpose of the PR, such as "Fix some issue (link existing issues this PR will resolve)".
  • The test plan, such as providing test command.
  • The test results, such as pasting the results comparison before and after, or e2e results
  • (Optional) The necessary documentation update, such as updating supported_models.md and examples for a new model.
  • (Optional) Release notes update. If your change is user facing, please update the release notes draft in the Google Doc.

@hl475 hl475 marked this pull request as ready for review November 20, 2025 19:30
hmellor
hmellor reviewed Nov 20, 2025
View reviewed changes
Copy link
Member

@hmellor hmellor left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the fix. If we need default rope type and to remove the factor could we just create a new dict for sliding layers? This would allow us to remove the copy in the line above because we would no longer be modifying it in place

hmellor
hmellor reviewed Nov 20, 2025
View reviewed changes
@hl475
address comments
074c4f9 
Signed-off-by: Huamin Li <[email protected]>
@hl475 hl475 force-pushed the fix_gemma3_attention branch from c5a6b15 to 074c4f9 Compare November 20, 2025 20:26
@hl475
Copy link
Contributor Author

hl475 commented Nov 20, 2025

Thanks @hmellor ! Love your suggestion and I think it is more clean!

Please take another look!

@hl475 hl475 requested a review from hmellor November 20, 2025 20:27
hmellor
hmellor approved these changes Nov 20, 2025
View reviewed changes
Copy link
Member

@hmellor hmellor left a comment

Choose a reason for hiding this comment

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

LGTM!

hmellor
hmellor reviewed Nov 20, 2025
View reviewed changes
@hmellor hmellor enabled auto-merge (squash) November 20, 2025 21:58
@github-actions github-actions bot added the ready ONLY add when PR is ready to merge/full CI is needed label Nov 20, 2025
@hmellor
Copy link
Member

hmellor commented Nov 21, 2025

Failing Plamo3 test is failing on main and should be fixed by #29092

@vllm-bot vllm-bot merged commit 8ac3a41 into vllm-project:main Nov 21, 2025
42 of 50 checks passed
ywang96 pushed a commit to ywang96/vllm that referenced this pull request Nov 23, 2025
@hl475 @hmellor
@DarkLight1337 @ywang96
[CI Failure] Fix Gemma3 RoPE configuration for sliding attention laye…
7cfdaa2 
…rs (vllm-project#29111)

Signed-off-by: Huamin Li <[email protected]>
Signed-off-by: Harry Mellor <[email protected]>
Co-authored-by: Harry Mellor <[email protected]>
Co-authored-by: Cyrus Leung <[email protected]>
PatchouliTIS pushed a commit to PatchouliTIS/vllm that referenced this pull request Nov 24, 2025
@hl475 @hmellor @DarkLight1337
[CI Failure] Fix Gemma3 RoPE configuration for sliding attention laye…
c92e4b8 
…rs (vllm-project#29111)

Signed-off-by: Huamin Li <[email protected]>
Signed-off-by: Harry Mellor <[email protected]>
Co-authored-by: Harry Mellor <[email protected]>
Co-authored-by: Cyrus Leung <[email protected]>
Signed-off-by: PatchouliTaisa <[email protected]>
lpapavassiliou pushed a commit to lpapavassiliou/vllm that referenced this pull request Nov 24, 2025
@hl475 @hmellor
@DarkLight1337 @lpapavassiliou
[CI Failure] Fix Gemma3 RoPE configuration for sliding attention laye…
27bd379 
…rs (vllm-project#29111)

Signed-off-by: Huamin Li <[email protected]>
Signed-off-by: Harry Mellor <[email protected]>
Co-authored-by: Harry Mellor <[email protected]>
Co-authored-by: Cyrus Leung <[email protected]>
RunkaiTao pushed a commit to RunkaiTao/vllm that referenced this pull request Nov 24, 2025
@hl475 @hmellor
@DarkLight1337 @RunkaiTao
[CI Failure] Fix Gemma3 RoPE configuration for sliding attention laye…
fc3522f 
…rs (vllm-project#29111)

Signed-off-by: Huamin Li <[email protected]>
Signed-off-by: Harry Mellor <[email protected]>
Co-authored-by: Harry Mellor <[email protected]>
Co-authored-by: Cyrus Leung <[email protected]>
Signed-off-by: Runkai Tao <[email protected]>
bringlein pushed a commit to bringlein/vllm that referenced this pull request Nov 26, 2025
@hl475 @hmellor
@DarkLight1337 @bringlein
[CI Failure] Fix Gemma3 RoPE configuration for sliding attention laye…
cedd0a0 
…rs (vllm-project#29111)

Signed-off-by: Huamin Li <[email protected]>
Signed-off-by: Harry Mellor <[email protected]>
Co-authored-by: Harry Mellor <[email protected]>
Co-authored-by: Cyrus Leung <[email protected]>
devpatelio pushed a commit to SumanthRH/vllm that referenced this pull request Nov 29, 2025
@hl475 @hmellor
@DarkLight1337 @devpatelio
[CI Failure] Fix Gemma3 RoPE configuration for sliding attention laye…
252d6af 
…rs (vllm-project#29111)

Signed-off-by: Huamin Li <[email protected]>
Signed-off-by: Harry Mellor <[email protected]>
Co-authored-by: Harry Mellor <[email protected]>
Co-authored-by: Cyrus Leung <[email protected]>
kitaekatt pushed a commit to kitaekatt/vllm that referenced this pull request Dec 1, 2025
@hl475 @hmellor
@DarkLight1337 @kitaekatt
[CI Failure] Fix Gemma3 RoPE configuration for sliding attention laye…
c341c1a 
…rs (vllm-project#29111)

Signed-off-by: Huamin Li <[email protected]>
Signed-off-by: Harry Mellor <[email protected]>
Co-authored-by: Harry Mellor <[email protected]>
Co-authored-by: Cyrus Leung <[email protected]>
charlotte12l pushed a commit to charlotte12l/vllm that referenced this pull request Dec 5, 2025
@hl475 @hmellor
@DarkLight1337 @charlotte12l
[CI Failure] Fix Gemma3 RoPE configuration for sliding attention laye…
ea8abec 
…rs (vllm-project#29111)

Signed-off-by: Huamin Li <[email protected]>
Signed-off-by: Harry Mellor <[email protected]>
Co-authored-by: Harry Mellor <[email protected]>
Co-authored-by: Cyrus Leung <[email protected]>
Signed-off-by: Xingyu Liu <[email protected]>
Zhathw pushed a commit to Zhathw/vllm that referenced this pull request Dec 6, 2025
@hl475 @hmellor
@DarkLight1337 @Zhathw
[CI Failure] Fix Gemma3 RoPE configuration for sliding attention laye…
0b72dfd 
…rs (vllm-project#29111)

Signed-off-by: Huamin Li <[email protected]>
Signed-off-by: Harry Mellor <[email protected]>
Co-authored-by: Harry Mellor <[email protected]>
Co-authored-by: Cyrus Leung <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hmellor hmellor hmellor approved these changes

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

@hl475 @hmellor @vllm-bot @DarkLight1337