Repository navigation
Conversation
…rmers 5
`training.rope_scaling` (documented, offered by the wizard for contexts above 4096, and in config_template.yaml) was
passed to `from_pretrained(rope_scaling=...)`. On transformers 5 the RoPE settings live in `config.rope_parameters`:
the documented `{type: linear, factor: 4}` raises `KeyError: 'rope_type'` while the model is built, and
`{rope_type: ..., factor: ...}` replaces the whole dictionary so `rope_theta` is lost and the frequency computation
raises `TypeError`. `--dry-run` passes, then the run exits 2 at model construction.
The requested keys are now merged over the model's own `rope_parameters` (so `rope_theta` is kept and can still be
overridden), the legacy key `type` is read as `rope_type`, the settings go to the text sub-config of a multimodal
checkpoint, and the finished config object is what is loaded. With a config object, `from_pretrained` hands unknown
keyword arguments to the model constructor, so the sliding-window override is applied to the config as well.
Reproduced both failures and checked linear, dynamic and yarn on transformers 5.13.0 with a tiny local Llama config on
CPU, building the model and running a forward pass. The tests do the same, and use no network. The 5.3-5.9 range the
declared floor still admits is not exercised here.
Refs #103
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Sorry @cemililik, you've used your own review budget of 250,000 diff characters for the last 7 days. You can request another review in 13 hours and 38 minutes by commenting |
Contributor
|
Warning Review limit reachedEnable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. Next included review available in 40 minutes. View limit details
Comment |
Reviewer's GuideThe PR fixes Transformers 5 model-loading failures by loading and mutating the model config before construction, merging rope scaling safely into existing parameters, routing related overrides through that config, and adding mocked plus end-to-end local-Llama regression tests. Sequence diagram for Transformers 5 model loading with RoPE scalingsequenceDiagram
participant Builder as _build_model_kwargs
participant AutoConfig
participant ModelConfig
participant Model as from_pretrained
Builder->>_config_with_rope_scaling: _config_with_rope_scaling(name_or_path, rope_scaling, trust_remote_code, revision)
_config_with_rope_scaling->>AutoConfig: AutoConfig.from_pretrained(name_or_path, trust_remote_code, revision)
AutoConfig-->>_config_with_rope_scaling: model_config
_config_with_rope_scaling->>ModelConfig: get_text_config()
_config_with_rope_scaling->>ModelConfig: merge rope_parameters with requested keys
_config_with_rope_scaling-->>Builder: finished model_config
opt sliding_window_attention
Builder->>ModelConfig: set sliding_window
end
Builder->>Model: from_pretrained(config=model_config)
Model-->>Builder: constructed model
Flow diagram for Transformers 5 RoPE scaling compatibilityflowchart TD
A[training.rope_scaling configured] --> B[Load model config]
B --> C{Multimodal checkpoint?}
C -->|Yes| D[Use text sub-config]
C -->|No| E[Use model config]
D --> F{Has rope_parameters?}
E --> F
F -->|Yes| G[Normalize type to rope_type and merge requested keys]
F -->|No| H[Set legacy rope_scaling]
G --> I[Apply sliding_window override to config]
H --> I
I --> J[from_pretrained with config]
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
5 tasks done
Collaborator
Author
|
Superseded by #999: the wave-0 fixes are combined by theme into five pull requests to cut the review load. This branch's commits are included there unchanged apart from the |
cemililik
added a commit
that referenced
this pull request
Oct 18, 2026
…n.enforce, transformers CVE floor, FSDP strategies, purge output dir (#999) ## Summary Six code fixes combined into one pull request, replacing #984, #985, #987, #989, #990 and #992 (to be closed as superseded). Each fixes a case where the code failed, or a documented option did nothing, without saying so. Each was mutation-checked on its own; together they pass the full gauntlet. ## Related issues Refs #103, #95, #92, #98, #109, #104, #102. Not closed by this change: the top-of-range problems named in #109 (`warmup_ratio` on transformers 5.15+, the string `fsdp` form removed in 5.20), the FSDP2 migration (#104), and reading `training.output_dir` for the purge memorisation check (#102). ## Changes | # | Fix | Where | |---|---|---| | #984 | `training.rope_scaling` crashed every model load on transformers 5 (`KeyError: 'rope_type'`, or `rope_theta` lost). The requested keys are merged **over** the model's `rope_parameters` and the finished config is loaded; `sliding_window_attention` travels in the same config object. | `forgelm/model.py` | | #985 | The CLI sets `HF_HUB_DISABLE_TELEMETRY=1` (via `setdefault`) before `huggingface_hub` is imported, so TRL's trainer no longer sends Hugging Face's anonymous usage ping the privacy page says does not exist. Library use is not covered; the air-gap page says so. | `cli/_config_load.py`, `cli/_dispatch.py` | | #987 | `retention.enforce` was documented as a gate (`warn_on_excess`, `block_on_excess` → `EXIT_EVAL_FAILURE`) but no command reads it. Docs, field description, both wizards say it is reserved; the CLI warns when a config sets either of the last two. | `config.py` (in place), `cli/_config_load.py`, `wizard/_collectors.py` | | #989 | `transformers>=5.3.0` was called the CVE floor but admitted CVE-2026-5241 and CVE-2026-9856 (a crafted `chat_template` name writes outside the save directory). Floor is now `>=5.10.1` (5.10.0 is yanked), `huggingface_hub>=1.5.0`, the nightly floor job pins both, and tests keep it from drifting. | `pyproject.toml`, `nightly.yml`, `tests/` | | #990 | `fsdp_strategy: no_shard` / `hybrid_shard` passed `--dry-run` and exited 2 after the model loaded (FSDP2 rejects them). They are now rejected when the config loads (exit 1), and two knobs FSDP2 ignores (`fsdp_auto_wrap: false`, `backward_post`) log a warning. | `config.py` (in place), `cli/_config_load.py` | | #992 | `forgelm purge --row-id` without `--output-dir` silently pointed the memorisation check at the corpus directory. It now warns and names the path. The purge module's line budget is raised by 8 with a `budget_history` note (the project's documented option). | `cli/subcommands/_purge.py`, `tools/check_module_size.py` | `config.py` (0 lines of headroom on the module-size ratchet) and `trainer.py` change only by in-place string edits; new warnings live in `cli/_config_load.py`. ### Merge-conflict handling The six branches touched `CHANGELOG.md`, `tests/test_cli.py` (two appended test classes) and the import line in `cli/_dispatch.py`; all three were resolved by keeping both sides (the import line now imports `_disable_hub_telemetry` and `_warn_inert_fsdp_options`). The affected test modules pass together (621 tests). ## Type - [x] Bug fix - [x] Dependency ## Testing - The full `CLAUDE.md` gauntlet from a clean worktree of this branch: exit 0. - Per-fix tests as in the original pull requests, including: a real tiny Llama on CPU for `rope_scaling` (linear, dynamic, yarn); the traversal reproduction (**fails on transformers 5.3.0**, passes on 5.10.1); all 32 accepted FSDP variants building real `TrainingArguments`. - transformers-facing test modules pass on a 5.10.1 / `huggingface_hub` 1.5.0 overlay (443 tests). The nightly minimum-versions job runs on its schedule after merge; I did not dispatch it. ## Checklist - [x] English and Turkish mirrors updated together - [x] CHANGELOG entries under `[Unreleased]` - [x] Tests added 🤖 Generated with [Claude Code](https://claude.com/claude-code) ## Summary by Sourcery Harden CLI behavior and dependency compatibility across model loading, privacy, retention, distributed training, and purge workflows. Bug Fixes: - Restore Transformers 5 compatibility for RoPE scaling and sliding-window model configuration. - Disable Hugging Face Hub telemetry by default for CLI runs while preserving explicit operator opt-in. - Warn when reserved retention enforcement modes are configured instead of implying unsupported enforcement behavior. - Reject unsupported FSDP1-only strategies during configuration and warn about FSDP2 options that have no effect. - Warn when purge uses the corpus directory as its implicit output directory and clarify the memorisation-check scope. Enhancements: - Document the actual retention, FSDP, telemetry, and purge behavior in English and Turkish user-facing documentation. Build: - Raise the minimum runtime dependencies to transformers>=5.10.1 and huggingface_hub>=1.5.0 to address relevant security advisories. - Update the nightly minimum-version job and packaging checks to enforce the new dependency floors. Tests: - Add coverage for telemetry initialization, reserved retention warnings, FSDP validation, purge output-directory warnings, Transformers 5 RoPE handling, and chat-template path traversal protection. Chores: - Increase the purge module size budget for the new output-directory warning. --------- Co-authored-by: Claude Sonnet 5.5 <noreply@anthropic.com>
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.
Summary
training.rope_scalingis documented, offered by the wizard for contexts above 4096 tokens, and present inconfig_template.yaml. On transformers 5 it crashed every model load, while the config validator and--dry-runaccepted it, so a run exited 2 only after the model started to build.Related issues
Fixes #103
Changes
forgelm/model.py: new_config_with_rope_scaling()loads the model's config and merges the requested keys over its ownrope_parameters._build_model_kwargsnow passes that finished config tofrom_pretrained(config=...)instead ofrope_scaling=....typeis read asrope_type, so the form the docs show keeps working.rope_parametersgets the legacyrope_scalingattribute.from_pretrainedhands unknown keyword arguments to the model constructor, sotraining.sliding_window_attentionis applied to the same config object (it used to travel as a keyword).What went wrong before (reproduced on transformers 5.13.0)
training.rope_scaling{type: linear, factor: 4}(as documented)KeyError: 'rope_type'while the model is built{rope_type: linear, factor: 4}rope_thetais lost,TypeErrorin the frequency computationType
Testing
TestConfigWithRopeScalingintests/test_model.pybuilds a tiny local Llama on CPU (no network), runs a forward pass forlinear,dynamicandyarn, and checks thatrope_thetasurvives and can still be overridden, that the legacytypekey works, and that the sliding-window override lands on the config.CLAUDE.mdgauntlet from a clean worktree of this branch: exit 0.Not covered
The 5.3–5.9 range that the declared floor still admits was not exercised. #109 raises that floor.
Checklist
[Unreleased]🤖 Generated with Claude Code
Summary by Sourcery
Restore Transformers 5 compatibility for configured RoPE scaling during model loading.
Bug Fixes:
training.rope_scalingmodel loading on Transformers 5 by applying requested RoPE settings through the model configuration while preserving existing parameters such asrope_theta.typeRoPE configuration keys, multimodal text sub-configs, and older Transformers versions withoutrope_parameters.Tests: