Skip to content

Read rope_theta from rope_parameters across Inference V2 - #8345

Open
alanhuangyoo wants to merge 4 commits into
deepspeedai:masterfrom
alanhuangyoo:fix/v2-rope-theta-from-rope-parameters
Open

Read rope_theta from rope_parameters across Inference V2#8345
alanhuangyoo wants to merge 4 commits into
deepspeedai:masterfrom
alanhuangyoo:fix/v2-rope-theta-from-rope-parameters

Conversation

@alanhuangyoo

Copy link
Copy Markdown
Contributor

Follow-up to #8341, which covers the same drift in the v1 kernel-injection policy. Separate engine, separate change; #8341 noted this exposure but did not touch it.

What breaks

transformers 5.0 folded the rotary settings into config.rope_parameters and dropped the rope_theta attribute. Eight V2 models still read the attribute.

Seven of them read it directly and raise; exaone4 reads it through a getattr default and quietly uses that default instead:

V2 model on master (transformers 5.8.0) with this PR
llama_v2 AttributeError: 'LlamaConfig' object has no attribute 'rope_theta' 10000.0
mistral AttributeError: 'MistralConfig' … 10000.0
mixtral AttributeError: 'MixtralConfig' … 1000000.0
phi AttributeError: 'PhiConfig' … 10000.0
phi3 AttributeError: 'Phi3Config' … 10000.0
qwen_v2 AttributeError: 'Qwen2Config' … 10000.0
qwen_v2_moe AttributeError: 'Qwen2MoeConfig' … 10000.0
exaone4 silently 1000000.0 10000.0

exaone4 is the worse of the two. Exaone4Config carries rope_parameters['rope_theta'] == 10000.0, so the getattr default puts a 100x rotary base into RotateHalfConfig with nothing raised — wrong frequencies rather than a startup failure.

requirements-inf.txt asks for transformers>=4.32.1 with no upper bound, so 5.x is in range.

The fix

exaone4_5 already reads both spellings — added with the model in #8121:

theta = rope_parameters.get("rope_theta", getattr(config, "rope_theta", None))

Hoist the same lookup onto DSTransformerModelBase as a rope_theta property. DSMoETransformerModelBase extends it, so all eight models are covered by one place and each call site becomes theta_base=self.rope_theta.

The attribute is tried first, so pre-5.0 installs take exactly the path they take today. When neither spelling carries a base it raises instead of guessing one — that is the only behaviour change beyond the fix, and it replaces exaone4's silent 1e6.

exaone4_5 is left alone; its own helper also handles the nested per-layer sliding_attention dict, which is specific to that model.

Test

tests/unit/inference/v2/model_implementations/test_rope_theta.py — the three layouts, the precedence between them, the raise, plus a parametrization over the eight real configs from the installed transformers:

13 passed
tests/unit/inference/v2/model_implementations/   19 passed
yapf --diff / flake8                             clean

I exercised the property and the configs, not a full V2 engine run against downloaded checkpoints.

transformers 5.0 folded the rotary settings into config.rope_parameters
and dropped the rope_theta attribute. Eight V2 models still read the
attribute:

  llama_v2, mistral, mixtral, phi, phi3, qwen_v2, qwen_v2_moe
      self._config.rope_theta       -> AttributeError on 5.x
  exaone4
      getattr(self._config, "rope_theta", 1000000.0)
      -> no error, but 1e6 instead of the 1e4 the config carries

exaone4 is the worse of the two: a 100x rotary base is a silent
numerical error, not a startup failure.

exaone4_5 already reads both spellings, added with the model in deepspeedai#8121.
Hoist the same lookup onto DSTransformerModelBase so every model that
reaches it through the base gets it, and raise instead of guessing a
base when neither spelling carries one.

Same drift as deepspeedai#8341, which covers the v1 kernel-injection policy.

Signed-off-by: alanhuangyoo <alanhuangyoo@gmail.com>
Comment thread deepspeed/inference/v2/model_implementations/inference_transformer_base.py Outdated
A config that sets RoPE per layer type nests the settings one level deeper,
keyed by the layer type, and standardize_rope_params leaves the class default
at the top level of the same dict:

  rope_parameters: {'sliding_attention': {'rope_theta': 1000000.0, ...},
                    'full_attention':    {'rope_theta': 16000000.0, ...},
                    'rope_theta': 10000.0, 'rope_type': 'default'}

Reading the top level returns 10000.0, which is the class default rather than
anything the checkpoint asked for, so the property resolved to a wrong base
quietly instead of raising. The nested entries now win.

When the layer types disagree the config is refused rather than resolved to
one of them. Every caller of this property feeds a single
RotateHalfConfig.theta_base for the whole model, so there is no shape in which
picking either base is right for the layers using the other one.

Released EXAONE-4 configs carry a flat rope_parameters and are unaffected.

Signed-off-by: alanhuangyoo <alanhuangyoo@gmail.com>
@alanhuangyoo
alanhuangyoo force-pushed the fix/v2-rope-theta-from-rope-parameters branch from ef14c37 to 92ab1af Compare September 4, 2026 04:23
@alanhuangyoo

Copy link
Copy Markdown
Contributor Author

@tohtana — this is the inference/v2 companion to #8341, which you reviewed. Flagging it since it has had no reviewer on it for a week and the two are related.

#8341 is the injection-policy path for Llama; this one is the same rope_theta / rope_parameters move across the eight inference/v2 model implementations that read it. It is independent of #8341 and does not depend on how that one resolves — but if the conclusion there is that scaled rope types have to be refused rather than served, the same question applies here and I would rather hear it once than fix it twice.

@alanhuangyoo

Copy link
Copy Markdown
Contributor Author

New information rather than a ping: #8341 merged this morning, and it is the same bug one layer up.

rope_theta moved into config.rope_parameters in transformers 5.x, and getattr(config, "rope_theta", <default>) now silently returns the default for every checkpoint — the value in config.json never reaches the kernel. #8341 fixed that in the AutoTP injection policy (module_inject/containers/llama.py) and @tohtana merged it, so the reasoning has been accepted; this PR is the same fix at a different layer, with no file overlap:

PR layer
#8341 (merged) module_inject/containers/llama.py — injection policy
#8345 inference/v2/model_implementations/* — 9 model implementations
#8373 ops/transformer/inference/op_binding/ — the rotary op binding

Merged current master in just now and re-ran on 1×H20; nothing has gone stale.

tests/unit/inference/v2/model_implementations/test_rope_theta.py: 15 passed.

@tohtana tohtana left a comment

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.

Hi @alanhuangyoo, thanks for following up, and sorry I missed your earlier ping before merging #8341.

Yes, this fix is still needed, can we apply the same approach regarding RoPE scaling as in #8341?

In #8341, we ended up rejecting unsupported scaled RoPE configurations to avoid silently producing incorrect results. Could you apply the same protection here? The new rope_theta property currently accepts configurations such as rope_type="llama3" and returns only the scalar theta, while these V2 callers do not propagate the additional scaling parameters. Please also add regression coverage for this rejection through both rope_parameters and the legacy rope_scaling layout, while preserving support for unscaled configurations.

Requested by @tohtana: apply the protection deepspeedai#8341 added for kernel injection here too.

Every caller of this property builds `RotateHalfConfig(theta_base=self.rope_theta)`,
and that config carries `use_trained_freqs`, `theta_base` and `rotate_dim` and nothing
else. A config asking for `rope_type="llama3"` has nowhere to put its scaling
parameters, so returning the base alone ran the model with unscaled positions and no
error -- the same silent wrong answer deepspeedai#8341 refused.

`_rope_types` reads both layouts, since transformers 5.x keeps these in
`rope_parameters` and 4.x in `rope_scaling`, both spellings of the key (`rope_type`
and `type`), and the per-layer-type entries nested inside either. The check runs
before the attribute read, or a 4.x config carrying both `rope_theta` and a scaled
`rope_scaling` would return early and never reach it.

Coverage, as asked: rejection through `rope_parameters`, through legacy `rope_scaling`
under both key spellings, nested per layer type, and with the attribute present; plus
the unscaled spellings (absent and `"default"`) still resolving in both layouts.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: alanhuangyoo <alanhuangyoo@gmail.com>
@alanhuangyoo

Copy link
Copy Markdown
Contributor Author

@tohtana Done in 41a8e41, and no need to apologise — #8341 landing first is what made the shape obvious.

I checked the premise before writing it rather than taking it on faith: every caller of this property does

return RotateHalfConfig(theta_base=self.rope_theta)

and RotateHalfConfig carries use_trained_freqs, theta_base and rotate_dim — nothing else. So a scaled config's parameters have nowhere to go, exactly as you said, and returning the base alone is the silent wrong answer #8341 refused.

What the check reads. _rope_types collects every variant a config asks for across:

  • rope_parameters (transformers 5.x) and rope_scaling (4.x)
  • both spellings of the key, rope_type and type
  • the per-layer-type dicts nested inside either

It runs before the rope_theta attribute read. A 4.x config carrying both rope_theta and a scaled rope_scaling would otherwise return early and never reach the check, which is the case I would most expect in the wild.

Coverage, as requested:

rope_parameters llama3, linear, dynamic, yarn, longrope
legacy rope_scaling both key spellings
nested per layer type a yarn entry beside a default one
attribute present alongside a scaled config refused rather than short-circuited
unscaled still resolves absent and "default", in both layouts

Verified they fail for the right reason on the pre-review commit rather than only that they pass now:

9 failed, 17 passed     all nine: Failed: DID NOT RAISE ValueError

Whole directory: master 6 passed, this branch 32 passed (6 + the 26 here), no regressions.

One thing worth your call: this refuses a model that runs today, silently and wrongly. exaone4 in particular used to fall back to 1000000.0 via a getattr default, so a scaled EXAONE-4 config would have been served with unscaled positions rather than refused. I think that is the right trade — it is the same one #8341 made — but it is a behaviour change for anyone who has been unknowingly running one of these, so tell me if you would rather it warn first for a release.

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