Repository navigation
Fix rope_scaling setter for configs that assign it during init - #50080
Conversation
Only standardize RoPE params in the rope_scaling setter after __post_init__, so configs that assign rope_scaling during init keep their original dict and rope_theta. Restore convert_to_rgb's pass-through for inputs that are neither PIL nor numpy. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
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. |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
rope_scaling setter and convert_to_rgb regressions for remote coderope_scaling setter for configs that assign it during init
| ) | ||
| self.rope_parameters = value | ||
| if value is not None: | ||
| if value is not None and getattr(self, "_post_init_done", False): |
There was a problem hiding this comment.
I hate this field 😭
yeah, so the goal is to allow re-setting field via rope_scaling name on local/new models in unified format and keep remote code happy by allowing them set whatever they want
Maybe we can standardize only if getattr rope_parameters is not None
There was a problem hiding this comment.
I don't think so. The issue is that we are validating rope too eagerly before custom configs have had the chance to set everything.
i.e. the following snippet raises an error with that suggestion:
class MyConfig(PreTrainedConfig):
def __init__(...):
self.rope_scaling = {...}
self.__post_init__()There was a problem hiding this comment.
iiuc these remote models they don't yet have any rope param set in init anyway, no?
Just don't want to add another private field to track if initialisation is done
There was a problem hiding this comment.
They do. Here's another example https://huggingface.co/microsoft/Phi-3.5-vision-instruct/blob/main/configuration_phi3_v.py#L166-L167L
def __init__(
...
):
self.rope_scaling = rope_scaling
self._rope_scaling_validation()That validation method checks the number of keys in rope_scaling, and eagerly calling standardize_rope_params breaks that.
zucchini-nlp
left a comment
There was a problem hiding this comment.
oke ig with a variety of custom code on the hub, this is the most reliable way
|
I think there should be a simpler solution that avoids the extra if getattr(self, "rope_parameters", None) is not None and value is not None:
self.rope_parameters = value
self.standardize_rope_params()
else:
self.rope_parameters = valueIt's not critical, so if we want to rush the current version into the patch that's fine, but this might be tidier, and I think it passes for @hmellor's snippets above |
|
The issue is not the existence of
|
) * Fix rope_scaling setter and convert_to_rgb regressions for remote code Only standardize RoPE params in the rope_scaling setter after __post_init__, so configs that assign rope_scaling during init keep their original dict and rope_theta. Restore convert_to_rgb's pass-through for inputs that are neither PIL nor numpy. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Revert convert_to_rgb pass-through Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
|
Yeah, that is also what i thought at first tho it seems remote models set and reset again the rope field several times, e.g. the custom standardisation from Harry's message above |
Picks up huggingface/transformers#50080. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
|
Hmmn, are there examples of failing ones? I think those criticisms are correct if I sent an agent to test it with Phi-3 / Phi-3.5-vision / Phi-3.5-MoE / Phi-4-multimodal / InternLM2.5 / Step-3.7 / Step5 / EAGLE3 and your |
|
The snippet you proposed would work if we could guarantee that all custom models would only ever write |
5.22.0 carries the rope_scaling setter fix (huggingface/transformers#50080) that 5.21.0 lacked, so the pin moves straight to it. The 5.21 adaptations already on this branch cover 5.22 as well: every transformers symbol SGLang imports still exists at v5.22.0, the chat-template mirror still matches apply_chat_template, and the shipped TP plan styles are unchanged.
What does this PR do?
Fixes a v5.21.0 regression that breaks remote-code configs (found bumping vLLM to 5.21, vllm-project/vllm#64049).
rope_scalingsetter (#49515). The setter now callsstandardize_rope_params()immediately. Remote code (and vLLM's speculators config) assignsrope_scalingin__init__beforesuper().__init__(), so standardizing there:rope_type/rope_theta, which fails remote validators that check its exact keys (Phi-3/3.5-vision, Phi-3.5-MoE, Phi-4-multimodal, InternVL2, InternLM2.5);rope_theta=Nonewhenrope_thetais assigned afterrope_scaling(EAGLE3 speculators, ERNIE-4.5-VL), crashing RoPE init;max_position_embeddingsbefore it exists (Step-3.7-Flash, Step-5).The setter now only standardizes after
__post_init__has finished, so assigningrope_scalingto an initialised config is still normalised as #49515 intended.__post_init__standardizes everything assigned before it, as in 5.20.Tests
test_rope_scaling_setter_during_initfails onmainand passes here.pytest tests/utils/test_configuration_utils.py tests/utils/test_modeling_rope_utils.py tests/test_image_transforms.py tests/models/llama/test_modeling_llama.py tests/models/phi3/test_modeling_phi3.py tests/models/qwen2_vl/test_image_processing_qwen2_vl.py tests/models/chameleon/test_image_processing_chameleon.py: 462 passed, 389 skipped.nm-testing/Llama3_2_1B_speculator.eagle3) getsrope_theta=500000.0again (Noneonmain), and vLLM'sStep5Configbuilds again.This PR was written with AI assistance (Claude Code).
Before submitting
Pull Request checks?
to it if that's the case.
🤖 Generated with Claude Code