Skip to content

[RoPE] allow models to configure local RoPE - #39397

Closed
zucchini-nlp wants to merge 18 commits into
huggingface:mainfrom
zucchini-nlp:fix-broken-ci
Closed

[RoPE] allow models to configure local RoPE#39397
zucchini-nlp wants to merge 18 commits into
huggingface:mainfrom
zucchini-nlp:fix-broken-ci

Conversation

@zucchini-nlp

Copy link
Copy Markdown
Member

What does this PR do?

Failing tests reported by CI bot after removing deprecations in #38838

The main fixes are in test files and in ModernBert/Llama. The test was failing because RoPE needs to use local theta in local attention layers. I think at this point we can allow local_rope_theta, looks like ModernBert isn't the only arch using two different RoPE configurations

@zucchini-nlp
zucchini-nlp requested a review from gante July 14, 2025 08:51
@zucchini-nlp zucchini-nlp changed the title fix tests and allow local rope [RoPE] allow models to configure local RoPE Jul 14, 2025
@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.

@zucchini-nlp

Copy link
Copy Markdown
Member Author

run-slow: arcee, aria, bamba, bitnet, cohere, cohere2, csm, deepseek_v3, dia, diffllama, doge, dots1, emu3, falcon, falcon_h1, fuyu

@github-actions

Copy link
Copy Markdown
Contributor

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

models: ['models/arcee', 'models/aria', 'models/bamba', 'models/bitnet', 'models/cohere', 'models/cohere2', 'models/csm', 'models/deepseek_v3', 'models/dia', 'models/diffllama', 'models/doge', 'models/dots1', 'models/emu3', 'models/falcon', 'models/falcon_h1', 'models/fuyu']
quantizations: [] ...

@zucchini-nlp

Copy link
Copy Markdown
Member Author

Failing slow tests are failing on main as well

@gante gante left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Two overall comments:

1 -- unbloating

In the spirit of unbloating: can we move boilerplate code into a separate model-agnostic function?

e.g. instead of

rope_scaling_dict = (
    getattr(config, "rope_scaling", None) if is_global else getattr(config, "local_rope_scaling", None)
) 
if rope_scaling_dict is not None and isinstance(config.rope_scaling, dict):
    self.rope_type = rope_scaling_dict.get("rope_type", rope_scaling_dict.get("type"))
else:
    self.rope_type = "default"

something like

self.rope_type = extract_rope_type_from_config(config, is_global)

2 -- future-proofing

This PR adds is_global: bool = True as a toggle to select the right config key to load RoPE parameterization from. What if we make it rope_config_key: str = "rope_theta" instead? That way, we can enable as many levels of RoPE as we want.

(or would this be too complex? WDYT?)

@zucchini-nlp

Copy link
Copy Markdown
Member Author

What if we make it rope_config_key: str = "rope_theta" instead?

It is not only the theta that changes, but the RoPE type might be different. For ex in gemma3 local attention is simple RoPE and the global attention applies scaling. Thus we have config keys:
rope_scaling_global="linear" and rope_scaling_local=None

Happy to simplify it if you have any ideas. Maybe we can do smth like rope_scaling = {"global" : {global_dict_here}, "local": None}? Though we need to make sure RoPE can change dynamically within one model, so imo is_global is the most straightforward way to allow it

@gante

gante commented Jul 24, 2025

Copy link
Copy Markdown
Contributor

@zucchini-nlp I see 🤔 In general, my concern is about the case where more types of RoPE are added, but the flag we add in this PR wouldn't be able to accommodate it. Secondarily, we should try to nudge users into nesting parameterization, since RoPE can take several parameters and things can get messy really fast (i.e. instead of xxx / local_xxx, have the structure you suggested by default, rope_scaling = {"global" : {global_dict_here}, "local": None}).

Perhaps rope_config_key: str = "global", with some extra logic in the rope files to parse global/local parameters when they are not nested? Then we would simultaneously:

  • support all existing formats, no BC breaking
  • allow as many rope types as the user wishes
  • nudge users into nested parameterization

Potentially missing: some attribute in the dict to control which layers use a certain rope type? (similar to the layer_types attribute)

@github-actions

Copy link
Copy Markdown
Contributor

[For maintainers] Suggested jobs to run (before merge)

run-slow: arcee, aria, bamba, bitnet, cohere, cohere2, csm, deepseek_v2, deepseek_v3, dia, diffllama, doge, dots1, emu3, ernie4_5, ernie4_5_moe

@zucchini-nlp

Copy link
Copy Markdown
Member Author

There is a cleaner PR

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