fix(rope): tolerate missing original_max_position_embeddings - #4742
Open
lizamd wants to merge 1 commit into
Open
fix(rope): tolerate missing original_max_position_embeddings#4742lizamd wants to merge 1 commit into
lizamd wants to merge 1 commit into
Conversation
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.
Contributor
🏷️ CI GuideRuns automatically on every PR:
Extended tests (opt-in via labels):
|
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
get_rope()raisesKeyErrorand every engine dies at init when a model'srope_scalinglacksoriginal_max_position_embeddings:Root cause is a transformers v4 -> v5 change. On v4, unscaled models had
config.rope_scaling = None. On v5 it aliasesrope_parametersand is alwayspopulated, e.g. for GLM-4.7-Flash:
{'rope_theta': 1000000, 'partial_rotary_factor': 1.0, 'rope_type': 'default'}sglang's
DeepseekV2AttentionMLAtreats a truthyrope_scalingas "scaling isconfigured" and sets
rope_type = "deepseek_yarn", so an unscaled MLA modelreaches 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_ropenever received the equivalent, so on ROCm the sameconfig is a hard failure rather than a warning.
Technical Details
Adds
_get_rope_param(rope_scaling, key, default, scaling_type), mirroringsglang's helper: return the key if present, otherwise log a warning and fall
back to
max_position. Applied at all four call sites that readoriginal_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
factorabsent (defaults to 1.0) andoriginal_max_position == max_position, YaRN degenerates to standard RoPE — nointerpolation, and the MLA attention scaling is unchanged. Verified:
Test Plan
rope_type='default') on 8x MI355X, ROCm 7.2,transformers 5.12.1, sglang 0.5.17,
SGLANG_USE_AITER=1(rollout -> backward -> weight sync)
Test Result
Before: all 8 engines abort at init with the
KeyErrorabove.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=1throughout;no fallback to non-aiter paths was needed.
Submission Checklist
changes for configs that already carry the key