Repository navigation
fix(models): lift transformers 5 rope_parameters for every family - #2448
Merged
Merged
Conversation
transformers 5 saves rope_theta, rope_scaling and partial_rotary_factor only under a nested rope_parameters block. Only Qwen3 and GraniteMoeHybrid lifted it (#2439), so about 80 config structs silently ran at their serde default base and drifted on long prompts. There is no raw-Value choke point (families read config.json directly and VLMs nest the block under text_config), so the type is the choke point. The lifter moves to mlxcel_core::rope_parameters with an impl_rope_lifting_deserialize! macro, and every uncovered struct gets #[serde(remote = "Self")] plus one macro line. The lifter also reads the per-layer-type form (Gemma 3, Gemma 3n, OLMo 3), lifts partial_rotary_factor, keeps MRoPE mrope_section, and writes the scheme under type only so alias-both fields do not hit a duplicate-field error. DFlashConfig::from_json, the Idefics2 defaults and both vlm_qwen ad hoc lifts use it; JinaVlmTextConfig::from_json falls back to the nested base, and the Phi-4 text-field copy now carries rope_parameters. EXAONE-MoE and Mistral 4 now let a flat key win, and exaone4 accepts a missing rope_scaling. A table test covers every struct; all four new tests fail with the fix stashed. Transformers-5 copies of llama-3.2-1b-instruct-4bit and qwen3-30b-a3b-4bit decode a 20 KB greedy prompt byte-identically to the originals; main produces different output on the copies. Closes #2443
Review follow-up for #2443. transformers 5.20 writes DeepSeek-V4 RoPE as {"main": {...}, "compress": {...}} with the compress-layer YaRN block under compress. The lifter only recognized full_attention / sliding_attention, so it treated this as a single block and synthesized an all-None rope_scaling out of the two sub-objects. A block whose values are all objects is now read as per-layer-type: full_attention or main supplies the base and scaling, sliding_attention gives rope_local_base_freq, compress gives compress_rope_theta and, when main names no scheme, rope_scaling. A "type": null key no longer hides a rope_type scheme. New lifter and DeepSeek-V4 ModelArgs tests pass, as do the affected family filters, clippy -D warnings for mlxcel and mlxcel-core, and the Llama 3.2 1B / Qwen3-30B-A3B transformers-5 copies, which still decode byte-identically to the originals. Refs #2443
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.
Closes #2443
transformers 5 saves
rope_theta,rope_scalingandpartial_rotary_factoronly under a nestedrope_parametersblock. Only Qwen3 and GraniteMoeHybrid lifted it (#2439). Every other struct that reads the flat keys fell back to its serde default base without an error: a transformers-5 copy of Llama 3.2 1B or Qwen3-30B-A3B onmainproduces garbage on a 20 KB prompt (below).Approach
I looked for a shared parse helper that every site already calls and found none: about 125 family
loadfunctions readconfig.jsondirectly, and VLMs nest the block undertext_config. So the type is the choke point, as the issue proposes.lift_rope_parameters/deserialize_lifting_rope_parametersmove tomlxcel_core::rope_parameters(re-exported frommodels::rope_utils), plus animpl_rope_lifting_deserialize!(Type)macro. Each uncovered struct gets#[serde(remote = "Self")]and one macro line: 86 structs, including the VLM text configs and the three Qwen3-Omni speech configs. Pipeline and tensor-parallel sites parse these same types, so they are covered. No struct gains a field.partial_rotary_factorand reads the per-layer-type forms, meaning a block whose values are all objects: for Gemma 3 / Gemma 3n / OLMo 3,full_attentiongives the base and scaling andsliding_attentiongivesrope_local_base_freq; for DeepSeek-V4,maingives the base andcompressgivescompress_rope_thetaand the YaRNrope_scaling. It buildsrope_scalingfrom a non-default scheme (Llama 3llama3included) or from MRoPE extras likemrope_section. The scheme goes undertypeonly (readtypefirst, thenrope_type), because the deepseek_v32 / deepseek_v4 / nemotronalias = "type", alias = "rope_type"fields reject a block that spells both.exaone4was the one reader that checked onlyrope_type, and it now acceptstypetoo.DFlashConfig::from_json, the Idefics2 Mistral defaults (which used to shadow the nested base), and bothvlm_qwen.rsad hoc lifts now use the shared lifter.JinaVlmTextConfig::from_jsonfalls back to the nested base, and the Phi-4 text-field copy now carriesrope_parameters.exaone4'srope_scalingis now#[serde(default)], because a default-scheme transformers 5 save has none.Validation
rope_parameters_lift_tests::every_flat_rope_theta_config_lifts_rope_parameters_and_flat_winsis table-driven over all 89 parse paths (the 86 lifted structs, Mistral 4, and the DFlash and Jinafrom_jsonpaths). It checks that nested-only gives 123456 and flat 500000 plus nested gives 500000. Required non-RoPE fields are filled from serde's own errors, neverrope_theta/rope_scaling. More tests: Llama 3 nestedllama3resolvesRopeScalingKind::Llama3, Gemma 3 per-layer-type, Phipartial_rotary_factor, and DeepSeek-V4compressYaRN (added in review; not revert-run). With the fix stashed and only the tests kept, the first four all fail, and every one of the 89 rows reports a failure. The lifter unit tests are inmlxcel-core.~/models_archive/mlx/llama-3.2-1b-instruct-4bitand~/models_archive/mlx/qwen3-30b-a3b-4bit, with onlyconfig.jsonrewritten (flat keys moved into arope_parametersblock identical to the one transformers 5.20.1 emits for that config) and the weights symlinked. Command:MLXCEL_SDPA_DETERMINISTIC=1 gpu-lock run -- mlxcel generate -m <dir> -p "<first 20000 bytes of CONTRIBUTING.md + a question>" -n 64 --temp 0 --show-reasoning. On the branch, both copies match their originals byte for byte, and the originals matchmain. Onmain(self-parity checked), the Llama copy degenerates intoand, and, ...and the Qwen3 copy into. The. The. ....--lib --tests -D warningsfor mlxcel and mlxcel-core), the affected-family test filters (647 tests) and the repo contract checks pass locally.Flat-wins follows the issue. transformers 5 itself gives the nested value priority, but the two only disagree when a config carries both with different values, and none of the reference checkpoints does.
Changes during review
The review found no CRITICAL or HIGH issues. Two follow-ups were applied: the DeepSeek-V4
main/compresslayout, which the first version turned into an all-Nonerope_scaling, and a"type": nullkey that hid therope_typescheme.Not covered: vision-encoder configs (Pixtral, Llama 4 vision, etc.) and
mlxcel-xla, which the issue does not list. DBRX is lifted atattn_config, where transformers 5 still keepsrope_theta, but its top-levelrope_parametersis not copied there.