Deprecate rope_scaling assignment and normalize legacy RoPE keys by jinwukong · Pull Request #49515 · huggingface/transformers

jinwukong · 2026-10-12T08:30:58Z

What does this PR do?

Assigning v4-style rope_scaling on a v5 config replaced the whole rope_parameters dictionary: rope_theta and partial_rotary_factor were dropped, the legacy type key was not normalised, so rotary-embedding construction failed with KeyError: 'rope_type' and a save/load round trip silently came back with the default theta (#49519).

Following the review, the setter now emits a FutureWarning pointing callers to rope_parameters, assigns the supplied value, and calls standardize_rope_params() only when the value is not None. It no longer tries to infer missing parameters or adds its own rope_theta validation. A complete dictionary still works and legacy type is normalised to rope_type; assigning None leaves rope_parameters as None.

The two regression tests cover the deprecation warning and legacy-key normalisation on a complete dictionary, and assigning None without standardisation.

Validation

Linux x86_64 and macOS, CPU, Python 3.12, PyTorch 2.14.

tests/utils/test_modeling_rope_utils.py: 37 passed after the change (35 existing + 2 new).
ruff format / ruff check on both files: clean.

Code Agent Policy

The Transformers repo is currently being overwhelmed by a large number of PRs and issue comments written by
code agents. These often are low-quality, or fix extremely minor issues that occur rarely or never in practice.
As a result, we're instituting a rule that first-time contributors should not use code agents to submit PRs or issues.
We'd also ask autonomous "OpenClaw"-like agents not to open any PRs or issues.

Issues/PRs from first-time contributors that violate this rule will probably just be closed without review, and we
might block you, especially if you open more than one or appear to be deliberately ignoring this. We especially do not
want new contributors to jump in on random issues to contribute an agent-written fix. This creates lots of noise
for reviewers and other users and will almost certainly get you blocked.

For more information, please read CONTRIBUTING.md.

(First-time contributors only): I confirm that this PR description and code is not written by an LLM or code agent

Before submitting

This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
Did you read the contributor guideline and the
Pull Request checks?
Was this discussed/approved via a Github issue or the forum? Please add a link
to it if that's the case.
Did you make sure to update the documentation with your changes according to the guidelines?
Did you write any new necessary tests?

Who can review?

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.

jinwukong · 2026-10-12T10:10:44Z

@zucchini-nlp @Cyrilvallez Could you take a look at this rope_scaling compatibility fix? #49519 has a CPU-only repro and the downstream impact from SGLang. All 45 local RoPE tests pass, including ten new regressions, and the PR CI run has passed.

zucchini-nlp · 2026-10-12T10:29:46Z

+        current = getattr(self, "rope_parameters", None) or {}
+        current_layer_keys = self.nested_rope_parameter_keys(current)
+        value = copy.deepcopy(value)
+        layer_keys = self.nested_rope_parameter_keys(value or {})
+        for parameters in [value[key] for key in layer_keys] or [value]:
+            if parameters is not None and "type" in parameters:
+                parameters.setdefault("rope_type", parameters.pop("type"))
+
+        base_keys = ("rope_theta", "partial_rotary_factor")
+        if current_layer_keys and not layer_keys:
+            self.rope_parameters = {
+                key: {name: parameters[name] for name in base_keys if name in parameters}
+                if (parameters := current[key]) is not None
+                else None
+                for key in current_layer_keys
+            }
+            self.convert_rope_params_to_dict(rope_scaling=value)
+        else:
+            parameters = value or {}
+            for key in layer_keys or [None]:
+                previous = current.get(key) or {} if key is not None else current
+                updated = parameters[key] if key is not None else parameters
+                if updated is not None:
+                    for name in base_keys:
+                        if name in previous:
+                            updated.setdefault(name, previous[name])
+            self.rope_parameters = parameters
+            self.convert_rope_params_to_dict()


wow! Not really sure why users are passing rope_scaling as override when sglang has pinned v5, since the "breaking" as intentional for major release in some way
I don't think it's disallowed on sglangs to override with rope_parameters no?

Thanks for taking a look! This isn't really about sglang: the reproduction is plain transformers. On LlamaConfig(rope_theta=500000.0), assigning config.rope_scaling = {"type": "yarn", "factor": 4.0, ...} leaves rope_parameters without rope_theta (and with type instead of rope_type), so LlamaRotaryEmbedding(config) raises KeyError: 'rope_type', and a save_pretrained / from_pretrained round trip comes back with the default theta without any error.

v5 still exposes rope_scaling as a property on PreTrainedConfig, and the __init__ kwargs path (convert_rope_params_to_dict) still accepts rope_scaling and keeps the base parameters; only the setter replaces the dict wholesale, so the two entry points disagree. If the alias is meant to be gone, raising or warning on assignment would be clearer than accepting it and corrupting the config.

As for why people still write rope_scaling: it is what the recipes say. sglang's cookbook (Qwen3-Next 1M context, Ling YaRN) launches with --json-model-override-args '{"rope_scaling": ...}', vLLM has the same report (vllm-project/vllm#58675), and both engines are now adding their own merge workaround (sgl-project/sglang#43067). Fixing the alias here removes the need for that.

If you'd rather steer users to rope_parameters, I'm happy to add a FutureWarning on the setter in this PR. The part I'd like to keep is that assigning through the alias no longer drops rope_theta silently.

If you'd rather steer users to rope_parameters

yes, exactly, that was my point since v5 was deliberately slightly breaking. We can update the cookbook in sglang as well since they have pinned v5 already

also maybe we just raise the warning/error instead of a long auto-infer code, wdyt?

Agreed, dropped the inference. Pushed the small version in d0a0a6a: the setter now emits a FutureWarning pointing to rope_parameters, raises a ValueError if rope_theta is missing, and otherwise assigns the value and calls standardize_rope_params() so type is normalised. I went with a warning rather than a hard error for the complete-dict case because SGLang and vLLM both assign rope_scaling today (--json-model-override-args / --hf-overrides), and raising would break them on the next release before the cookbooks are updated; happy to switch to raising if you prefer. Tests are down to two. I'll send the cookbook change to sglang as well.

Cookbook side is up: sgl-project/sglang#44155 moves the Qwen3-Next and Ling YaRN recipes to rope_parameters (with rope_theta / partial_rotary_factor included).

because SGLang and vLLM both assign rope_scaling today (--json-model-override-args / --hf-overrides)

you mean in the cookbook right rather than in actual prod code or in tests?

Production code, in both. vLLM's get_config() applies --hf-overrides with config.update(hf_overrides_kw) (vllm/transformers_utils/config.py), and update() does a setattr per key, so a user's {"rope_scaling": ...} lands in this setter at load time. SGLang's get_config() does the same with --json-model-override-args (python/sglang/srt/utils/hf_transformers/config.py); since sgl-project/sglang#43067 (merged yesterday) it merges rope_scaling / rope_parameters dicts with the loaded values before the setattr, so SGLang now always hands this setter a complete dict, while vLLM still passes whatever the user wrote. The cookbooks are only where people learn to write those overrides, hence the docs PR. Fine with warn or raise, your call.

github-actions · 2026-10-13T08:55:44Z

Thank you for your contribution 🤗!

CI Security Gate — waiting for maintainer approval

CI is approved automatically only for authors who already have a contribution merged in this repository. A maintainer will review this PR and approve CI to run on it.

zucchini-nlp · 2026-10-15T06:01:15Z

+            "instead, with `rope_type` and `rope_theta` included.",
+            FutureWarning,
+        )
+        parameters = [value[key] for key in self.nested_rope_parameter_keys(value or {})] or [value]


i suppose this is for None as value - lets early exit if none

Done in 1b3766a: None returns right after the warning and leaves rope_parameters untouched.

zucchini-nlp · 2026-10-15T06:02:18Z

+        if any(p is not None and "rope_theta" not in p for p in parameters):
+            raise ValueError("`rope_scaling` no longer fills in `rope_theta`; set `rope_parameters` with it included.")


standardize also checks self.rope-theta if available, so i thnk we can call standardize and let it raise an index/key error

Done, the check is gone; it is assign + standardize_rope_params() now (1b3766a). One thing to flag: a v5 config has no rope_theta attribute any more, so for a dict without rope_theta standardize does setdefault("rope_theta", None) instead of raising, and validate_rope is happy because the key exists; the failure then surfaces at rotary init as TypeError: unsupported operand type(s) for ** or pow(): 'NoneType' and 'Tensor' (checked with LlamaConfig(rope_theta=500000.0) and {"type": "yarn", "factor": 4.0}). Fine by me if that is acceptable; otherwise a one-line check here, or rope_theta joining required_keys in validate_rope, would make it fail at assignment. The requires_rope_theta test is replaced by a None test; 37 pass locally.

zucchini-nlp · 2026-10-15T06:25:55Z

+        if value is None:
+            return
        self.rope_parameters = value


we should set None if asked - just not ry to standardize :)

Done in b84f86d: None is assigned as-is (rope_parameters becomes None) and standardize_rope_params() only runs for a dict; the None test now asserts that. 37 pass locally.

zucchini-nlp

oke

HuggingFaceDocBuilderDev · 2026-10-16T00:37:44Z

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.

jinwukong · 2026-10-18T01:34:43Z

Thanks for the review. I updated the title and description to match the reviewed setter.

@ydshieh could you rerun tokenizer shard 4/8? It exits 137 in NllbTokenizationTest::test_chat_template, then the container hook fails while serializing a TLSSocket. I could not find a test assertion failure in that log.

github-actions · 2026-10-20T03:59:19Z

CI recap

Dashboard: View test results in Grafana
Latest run: 39365136118:2
Result: failure | Jobs: 7 | Tests: 16,790 | Failures: 0 | Duration: 1m 14s

Preserve RoPE base parameters in the rope_scaling setter

baddca3

This was referenced Oct 12, 2026

[Bugfix] Keep rope_theta when --json-model-override-args overrides rope_scaling sgl-project/sglang#43067

Merged

Assigning rope_scaling drops existing RoPE base parameters #49519

Open

zucchini-nlp reviewed Oct 12, 2026

View reviewed changes

Deprecate the rope_scaling setter instead of merging base parameters

d0a0a6a

jinwukong mentioned this pull request Oct 14, 2026

[Docs] Use rope_parameters for the YaRN overrides in the Qwen3-Next and Ling cookbooks sgl-project/sglang#44155

Open

3 of 5 tasks

zucchini-nlp reviewed Oct 15, 2026

View reviewed changes

Early-exit on None and let standardize_rope_params handle the rest

1b3766a

zucchini-nlp reviewed Oct 15, 2026

View reviewed changes

Assign None through the rope_scaling setter without standardizing

b84f86d

zucchini-nlp approved these changes Oct 16, 2026

View reviewed changes

zucchini-nlp enabled auto-merge October 16, 2026 00:28

jinwukong changed the title ~~Preserve RoPE base parameters in the rope_scaling setter~~ Deprecate rope_scaling assignment and normalize legacy RoPE keys Oct 18, 2026

zucchini-nlp added 2 commits October 20, 2026 11:33

Merge branch 'main' into fix/rope-scaling-setter

753b4f2

Merge branch 'main' into fix/rope-scaling-setter

93d5be6

zucchini-nlp added this pull request to the merge queue Oct 20, 2026

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Oct 20, 2026

zucchini-nlp added this pull request to the merge queue Oct 20, 2026

Merged via the queue into huggingface:main with commit 55928cf Oct 20, 2026
216 of 218 checks passed

stevhliu mentioned this pull request Oct 23, 2026

[docs] Translation workflow #49889

Closed

zhtmike mentioned this pull request Oct 27, 2026

[RFC][tracking] Upstream patch debt: audited ledger of temporary compatibility patches verl-project/verl-omni#445

Open

38 tasks

This was referenced Oct 28, 2026

Fix rope_scaling setter for configs that assign it during init #50080

Merged

[CI] Bump Transformers version to 5.21.1 vllm-project/vllm#64049

Merged

winglian mentioned this pull request Oct 29, 2026

chore: upgrade to transformers 5.21.1 axolotl-ai-cloud/axolotl#4223

Merged

3 tasks done

		if any(p is not None and "rope_theta" not in p for p in parameters):
		raise ValueError("`rope_scaling` no longer fills in `rope_theta`; set `rope_parameters` with it included.")

Conversation

jinwukong commented Oct 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Validation

Code Agent Policy

Before submitting

Who can review?

Uh oh!

jinwukong commented Oct 12, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Oct 13, 2026

CI Security Gate — waiting for maintainer approval

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zucchini-nlp left a comment

Choose a reason for hiding this comment

Uh oh!

HuggingFaceDocBuilderDev commented Oct 16, 2026

Uh oh!

jinwukong commented Oct 18, 2026

Uh oh!

github-actions Bot commented Oct 20, 2026

CI recap

Uh oh!

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jinwukong commented Oct 12, 2026 •

edited

Loading