Skip to content

Only default rope_parameters to empty dict if there is something to put in it #42651

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
hmellor wants to merge 4 commits into
base: main
Choose a base branch
from
+7 −2
Open

Only default rope_parameters to empty dict if there is something to put in it #42651

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fc4db6e
Only default `rope_parameters` to empty `dict` if there is something …
hmellor Dec 5, 2025
8012d22
Add warning
hmellor Dec 5, 2025
10bfdf5
Also catch explicit `{}`
hmellor Dec 5, 2025
b70b891
Merge branch 'main' into standardise-rope-fix
hmellor Dec 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions src/transformers/modeling_rope_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -654,13 +654,18 @@ def standardize_rope_params(self):
Helper to standardize the config's rope params field by ensuring the params are defined for each
later type. For old model the fn will duplicate a single rope param in each layer type (backward compatibility)
"""
# Move `rope_theta` and `partial_rotary_factor` to the params dict, if not there yet
# Move `rope_theta` and `partial_rotary_factor` to the `rope_parameters`, if not there yet
rope_theta = getattr(self, "rope_theta", None)
partial_rotary_factor = getattr(self, "partial_rotary_factor", None)
rope_parameters = getattr(self, "rope_parameters", None) or {}

# Case 0: no RoPE params defined
if not (rope_parameters or rope_theta):
# partial_rotary_factor without rope_theta is invalid, so we don't check for it here
logger.warning("`standardize_rope_params` was called but no RoPE parameters were found.")
return
# Case 1: RoPE param keys do not intersect with possible `layer_types` -> one global dict
if getattr(self, "layer_types", None) is None or not set(rope_parameters.keys()).issubset(self.layer_types):
elif getattr(self, "layer_types", None) is None or not set(rope_parameters.keys()).issubset(self.layer_types):
Copy link
Member

@zucchini-nlp zucchini-nlp Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the custom model has no rope parameters (i.e. it is {}), we will get triggered by the original issue of evaluting to True, no?

Copy link
Member Author

@hmellor hmellor Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right that if rope_parameters is explicitly set to {} we'll still get here. But if rope_parameters is unset, it will remain None and this method will exit early with a warning

rope_parameters.setdefault("rope_type", rope_parameters.get("type", "default"))
rope_parameters.setdefault("rope_theta", rope_theta)
if partial_rotary_factor is not None:
Expand Down