Skip to content

MRoPE continued - #48594

Merged
zucchini-nlp merged 16 commits into
huggingface:mainfrom
zucchini-nlp:rope-tests-cont
Sep 9, 2026
Merged

zucchini-nlp merged 16 commits into
huggingface:mainfrom
zucchini-nlp:rope-tests-cont

Conversation

@zucchini-nlp

@zucchini-nlp zucchini-nlp commented Sep 7, 2026 •

Copy link
Copy Markdown
Member

CPU CI GPU run-slow

What does this PR do?

Un-skips rope tests for 3D positions and reverts the no_grad() on rope, i don't know how and when it got deleted 😭

Thought we could also add test for each model config that validate_rope() raises no warnings (given GH a few issues i've seen), though we can't test with real weight so too much work for little gain, leaving that out

@HuggingFaceDocBuilderDev

Copy link
Copy Markdown

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

Comment thread tests/vlm_tester.py
Comment on lines +119 to +121
# do not use the same rope config as LM backbone, vision will always use axial rope (i hope)
if "rope_parameters" in kwargs:
kwargs["rope_parameters"] = {"rope_type": "axial", "rope_theta": 10_000}

@zucchini-nlp zucchini-nlp Sep 7, 2026 •

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

not cool in general that we can't set different values for the same key on text/vision/audio subconfigs. The only way is to override the whole config construction

Should be improved in the future!

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

definitely, a bit awkward 😬

"hidden_act": "silu",
"max_position_embeddings": 512,
"rope_parameters": {"type": "default", "mrope_section": [2, 2], "partial_rotary_factor": 1.0},
"rope_parameters": {"type": "default", "mrope_section": [2, 3, 3], "partial_rotary_factor": 1.0},

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

the code hardcoded 3 and even official ckpt uses 3 sections

Comment on lines +217 to +218
class Cosmos3EdgeTextRotaryEmbedding(Qwen3VLTextRotaryEmbedding):
pass

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

was really identical, we gotta be careful when reviewing models so contribs don't re-invent the wheel

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Sadly will and can happen at times. This is why it's more important than ever to have a more uniform recipe 🫡

Comment on lines -428 to -431
# Interleave them instead of usual shape
cos = cos[..., : cos.shape[-1] // 2].repeat_interleave(2, dim=-1)
sin = sin[..., : sin.shape[-1] // 2].repeat_interleave(2, dim=-1)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

this is so weird 😆
It was doing too much so I moved interleave where it belongs, sadly though we still can't apply rope kernel

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Argh I swear we can get there tho! I will maybe check whether I can work on the kernels side for this

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Maybe open an issue for me :D

Comment on lines +589 to +596
def recomposition_frequencies(self, freq):
"""
Recompose the frequencies into the final spatial layout used per each grid.
"""
freq = torch.cat(
[m[i % len(self.mrope_section)] for i, m in enumerate(freq.split(self.mrope_section, dim=-1))], dim=-1
)
return torch.cat((freq, freq), dim=-1)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

basically qwen but the num_axis depends on config instead of hardcoded 3, and the init is a bit different

Comment thread src/transformers/modeling_rope_utils.py Resolved
Comment on lines -789 to +824
rope_parameters["partial_rotary_factor"] = partial_rotary_factor
rope_parameters.setdefault("partial_rotary_factor", partial_rotary_factor)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

don't force override if the dict has a value set, mirror setdefault(rope_theta) above, a missed bug from months ago

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

uh oh do we maybe have a test for that

@zucchini-nlp
zucchini-nlp requested a review from vasqu September 8, 2026 10:09
@zucchini-nlp

Copy link
Copy Markdown
Member Author

run-slow: cosmos3_edge, glm4v, glm4v_moe, glm_image, glm_ocr, hunyuan_vl, kimi_k25, mistral3, pixtral

@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Workflow Run ⚙️

This comment contains run-slow, running the specified jobs:

models: ["models/cosmos3_edge", "models/glm4v", "models/glm4v_moe", "models/glm_image", "models/glm_ocr", "models/hunyuan_vl", "models/kimi_k25", "models/mistral3", "models/pixtral"]
quantizations: []

@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Workflow Run ⚙️

This comment contains run-slow, running the specified jobs on AMD:

models: ["models/cosmos3_edge", "models/glm4v", "models/glm4v_moe", "models/glm_image", "models/glm_ocr", "models/hunyuan_vl", "models/kimi_k25", "models/mistral3", "models/pixtral"]

@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

CI Results (AMD)

Workflow Run ⚙️

Commit Info

Context Commit Description
RUN 62b31cc8 workflow commit (merge commit)
PR 82461b0a branch commit (from PR)
main ada0a91f base commit (on main)

Model CI Report

❌ 17 new failed tests from this PR 😭

  • glm4v:
    tests/models/glm4v/test_modeling_glm4v.py::Glm4vIntegrationTest::test_small_model_integration_test (✅ ⟹ ❌)
    tests/models/glm4v/test_modeling_glm4v.py::Glm4vIntegrationTest::test_small_model_integration_test_batch (❌ ⟹ ❌)
    tests/models/glm4v/test_modeling_glm4v.py::Glm4vIntegrationTest::test_small_model_integration_test_batch_different_resolutions (✅ ⟹ ❌)
    tests/models/glm4v/test_modeling_glm4v.py::Glm4vIntegrationTest::test_small_model_integration_test_batch_flashatt2 (❌ ⟹ ❌)
    tests/models/glm4v/test_modeling_glm4v.py::Glm4vIntegrationTest::test_small_model_integration_test_batch_wo_image (❌ ⟹ ❌)
    tests/models/glm4v/test_modeling_glm4v.py::Glm4vIntegrationTest::test_small_model_integration_test_batch_wo_image_flashatt2 (❌ ⟹ ❌)
    tests/models/glm4v/test_modeling_glm4v.py::Glm4vIntegrationTest::test_small_model_integration_test_expand (❌ ⟹ ❌)
    tests/models/glm4v/test_modeling_glm4v.py::Glm4vIntegrationTest::test_small_model_integration_test_with_video (✅ ⟹ ❌)

  • glm_image:
    tests/models/glm_image/test_modeling_glm_image.py::GlmImageIntegrationTest::test_flash_attention_generation (✅ ⟹ ❌)
    tests/models/glm_image/test_modeling_glm_image.py::GlmImageIntegrationTest::test_text_to_image_generation (✅ ⟹ ❌)

  • hunyuan_vl:
    tests/models/hunyuan_vl/test_modeling_hunyuan_vl.py::HunYuanVLForConditionalGenerationIntegrationTest::test_small_model_integration_test (❌ ⟹ ❌)
    tests/models/hunyuan_vl/test_modeling_hunyuan_vl.py::HunYuanVLForConditionalGenerationIntegrationTest::test_small_model_integration_test_batch (❌ ⟹ ❌)
    tests/models/hunyuan_vl/test_modeling_hunyuan_vl.py::HunYuanVLForConditionalGenerationIntegrationTest::test_small_model_integration_test_batch_different_resolutions (❌ ⟹ ❌)
    tests/models/hunyuan_vl/test_modeling_hunyuan_vl.py::HunYuanVLForConditionalGenerationIntegrationTest::test_small_model_integration_test_batch_matches_single (❌ ⟹ ❌)
    tests/models/hunyuan_vl/test_modeling_hunyuan_vl.py::HunYuanVLForConditionalGenerationIntegrationTest::test_small_model_integration_test_multi_image (❌ ⟹ ❌)
    tests/models/hunyuan_vl/test_modeling_hunyuan_vl.py::HunYuanVLForConditionalGenerationIntegrationTest::test_small_model_integration_test_multi_image_nested (❌ ⟹ ❌)

  • kimi_k25:
    tests/models/kimi_k25/test_video_processing_kimi_k25.py::Kimi_k25VideoProcessingTest::test_can_compile_fast_video_processor (❌ ⟹ ❌)

@zucchini-nlp

Copy link
Copy Markdown
Member Author

run-slow: glm4v, glm4v_moe, glm_image, glm_ocr, hunyuan_vl

@huggingface huggingface deleted a comment from github-actions Bot Sep 8, 2026
@huggingface huggingface deleted a comment from github-actions Bot Sep 8, 2026
@huggingface huggingface deleted a comment from github-actions Bot Sep 8, 2026
@huggingface huggingface deleted a comment from github-actions Bot Sep 8, 2026
@huggingface huggingface deleted a comment from github-actions Bot Sep 8, 2026
@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

CI Results (AMD)

Workflow Run ⚙️

Commit Info

Context Commit Description
RUN 1d3922e8 workflow commit (merge commit)
PR d200bae4 branch commit (from PR)
main 0a959de1 base commit (on main)

✅ No failing test specific to this PR 🎉 👏 !

@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

CI Results

Workflow Run ⚙️

Commit Info

Context Commit Description
RUN 1d3922e8 workflow commit (merge commit)
PR d200bae4 branch commit (from PR)
main 0a959de1 base commit (on main)

Model CI Report

❌ 2 new failed tests from this PR 😭

  • hunyuan_vl:
    tests/models/hunyuan_vl/test_modeling_hunyuan_vl.py::HunYuanVLForConditionalGenerationIntegrationTest::test_small_model_integration_test (❌ ⟹ ❌)
    tests/models/hunyuan_vl/test_modeling_hunyuan_vl.py::HunYuanVLForConditionalGenerationIntegrationTest::test_small_model_integration_test_multi_image_nested (❌ ⟹ ❌)

@zucchini-nlp

zucchini-nlp commented Sep 9, 2026 •

Copy link
Copy Markdown
Member Author

I am so lost with slow CI reports, so is it green or failing

edit: ah one is for AMD...

@huggingface huggingface deleted a comment from github-actions Bot Sep 9, 2026
@zucchini-nlp

Copy link
Copy Markdown
Member Author

run-slow: hunyuan_vl

@huggingface huggingface deleted a comment from github-actions Bot Sep 9, 2026
@huggingface huggingface deleted a comment from github-actions Bot Sep 9, 2026
@huggingface huggingface deleted a comment from github-actions Bot Sep 9, 2026
@huggingface huggingface deleted a comment from github-actions Bot Sep 9, 2026
@huggingface huggingface deleted a comment from github-actions Bot Sep 9, 2026
@huggingface huggingface deleted a comment from github-actions Bot Sep 9, 2026
@zucchini-nlp

Copy link
Copy Markdown
Member Author

run-slow: hunyuan_vl

@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Workflow Run ⚙️

This comment contains run-slow, running the specified jobs on AMD:

models: ["models/hunyuan_vl"]

@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Workflow Run ⚙️

This comment contains run-slow, running the specified jobs:

models: ["models/hunyuan_vl"]
quantizations: []

@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

CI recap

Dashboard: View test results in Grafana
Latest run: 34329469866:1
Result: success | Jobs: 1 | Tests: 98 | Failures: 12 | Duration: 4m 36s

@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

CI Results

Workflow Run ⚙️

Commit Info

Context Commit Description
RUN 8e1c0c70 workflow commit (merge commit)
PR 3c1af418 branch commit (from PR)
main 5f47b5aa base commit (on main)

✅ No failing test specific to this PR 🎉 👏 !

@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

CI Results (AMD)

Workflow Run ⚙️

Commit Info

Context Commit Description
RUN 8e1c0c70 workflow commit (merge commit)
PR 3c1af418 branch commit (from PR)
main 5f47b5aa base commit (on main)

✅ No failing test specific to this PR 🎉 👏 !

@zucchini-nlp

Copy link
Copy Markdown
Member Author

Yea, this is truly weird with casting rope into dtype, uncasting back to rotate and downcasting QK again 🫠
anyway, small diffs affect tests so reverted everything, to keep the initial model design

@vasqu

vasqu commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator

Maybe you can move the dtype casting to apply rope but yea no worries. Sometimes we have to do weird things to keep up with the original

@zucchini-nlp

Copy link
Copy Markdown
Member Author

Yeah, that is what I did at the end but I really wanted to keep it copied from Llama (not today) 😆

@zucchini-nlp
zucchini-nlp added this pull request to the merge queue Sep 9, 2026
Merged via the queue into huggingface:main with commit 5b7dcb0 Sep 9, 2026
114 checks passed
@zucchini-nlp
zucchini-nlp deleted the rope-tests-cont branch September 9, 2026 13:29
sbucaille pushed a commit to sbucaille/transformers that referenced this pull request Sep 16, 2026
* add and fix

* wait what, why would they even override if it is same as qwen?!

* LOL why would we cat if rope is interleaved

* fix last

* axial but needs a fix

* maybe last fix?

* fix for the test, also this is more "standard"

* skip

* lost partial rotary factor

* just noticed - kinda weird rope but oke

* that still doesn't mul each value, gotta iterate

* nit

* keep float32 for rope, this should still be equivalent i hope

* really didnt want it, but num precision matters more than i thought
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants