Skip to content

fix(rope): tolerate missing original_max_position_embeddings - #4742

Open
lizamd wants to merge 1 commit into
ROCm:mainfrom
lizamd:fix/rope-v5-missing-original-max-position
Open

fix(rope): tolerate missing original_max_position_embeddings#4742
lizamd wants to merge 1 commit into
ROCm:mainfrom
lizamd:fix/rope-v5-missing-original-max-position

Conversation

@lizamd

@lizamd lizamd commented Aug 14, 2026

Copy link
Copy Markdown

Motivation

get_rope() raises KeyError and every engine dies at init when a model's
rope_scaling lacks original_max_position_embeddings:

File "aiter/rotary_embedding.py", line 1872, in get_rope
    original_max_position = rope_scaling["original_max_position_embeddings"]
KeyError: 'original_max_position_embeddings'

Root cause is a transformers v4 -> v5 change. On v4, unscaled models had
config.rope_scaling = None. On v5 it aliases rope_parameters and is always
populated, e.g. for GLM-4.7-Flash:

{'rope_theta': 1000000, 'partial_rotary_factor': 1.0, 'rope_type': 'default'}

sglang's DeepseekV2AttentionMLA treats a truthy rope_scaling as "scaling is
configured" and sets rope_type = "deepseek_yarn", so an unscaled MLA model
reaches the yarn branch without the keys that branch requires.

sglang already handles this in
sglang/srt/layers/rotary_embedding/factory.py::_get_rope_param (warn + default).
aiter's vendored get_rope never received the equivalent, so on ROCm the same
config is a hard failure rather than a warning.

Technical Details

Adds _get_rope_param(rope_scaling, key, default, scaling_type), mirroring
sglang's helper: return the key if present, otherwise log a warning and fall
back to max_position. Applied at all four call sites that read
original_max_position_embeddings (llama3, yarn, deepseek_yarn,
su/longrope) for parity with sglang.

The warning matters: silently defaulting would hide a genuinely mis-specified
config for a model that really does use scaling.

Correctness for the unscaled case: with factor absent (defaults to 1.0) and
original_max_position == max_position, YaRN degenerates to standard RoPE — no
interpolation, and the MLA attention scaling is unchanged. Verified:

compute_mla_mscale_scaling({'rope_type':'deepseek_yarn', ...}, 0.1234) -> 0.1234

Test Plan

  • GLM-4.7-Flash (MLA, rope_type='default') on 8x MI355X, ROCm 7.2,
    transformers 5.12.1, sglang 0.5.17, SGLANG_USE_AITER=1
  • Start all 8 sglang engines, then run an end-to-end GRPO training loop
    (rollout -> backward -> weight sync)

Test Result

Before: all 8 engines abort at init with the KeyError above.

After: engines initialize (max_total_num_tokens=2870263, context_len=202752)
and the training loop runs end to end, with the expected warning emitted once
per engine. All aiter kernels remain enabled — SGLANG_USE_AITER=1 throughout;
no fallback to non-aiter paths was needed.

Submission Checklist

  • Change is limited to the missing-key fallback; no kernel or numerics
    changes for configs that already carry the key
  • Verified on MI355X (gfx950)
  • Mirrors existing sglang behavior rather than introducing new semantics

transformers v5 exposes config.rope_scaling as an alias of rope_parameters,
which is populated even when no scaling is configured (rope_type='default').
Callers can therefore reach a scaled branch without the keys it expects,
raising KeyError and killing engine init (seen with GLM-4.7-Flash on MI355X).

Warn and fall back to max_position instead, mirroring sglang's
rotary_embedding.factory._get_rope_param.
@lizamd
lizamd requested a review from a team August 14, 2026 00:08
@github-actions

Copy link
Copy Markdown
Contributor

🏷️ CI Guide

Runs automatically on every PR:

  • ✅ Pre-checks (submodule verification, code formatting)
  • ✅ Aiter op tests (gfx942 + gfx950)
  • ✅ Triton tests on MI35X (only when aiter/ops/triton/** or related paths are changed)

Extended tests (opt-in via labels):

Label Tests
ci:gfx1250-ffm-triton Run the five-shard gfx1250 FFM Triton test suite
ci:triton-300x Run an additional Triton test job on MI300X in PRs; main branch always runs both MI35X and MI300X
ci:sglang SGLang integration tests: DeepSeek-R1-MXFP4 accuracy, Qwen 3.5 accuracy
ci:atom ATOM benchmark: DeepSeek-R1-0528, GPT-OSS-120B
ci:atom_full ATOM accuracy suite for PR and main models from ATOM models_accuracy.json
ci:vllm vLLM benchmark: GPT-OSS-120B, DeepSeek-R1-0528, Kimi-K2.5
ci:all All standard extended tests (excludes ci:atom_full)

Only add ci:atom_full for FlyDSL or Triton upgrades.
Add labels via the sidebar or gh pr edit 4742 --add-label <label>

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.

1 participant