Repository navigation
Conversation
…cannot run, and warn about two ignored knobs distributed.fsdp_strategy accepted no_shard and hybrid_shard (the latter described as the multi-node option). transformers 5.10 made FSDP2 the default, and FSDP2 raises "fsdp=hybrid_shard is only available with FSDP1" when TrainingArguments is built, so such a config passed --dry-run and then exited 2 after the model was loaded. The strategy is now a Literal of full_shard and shard_grad_op, so the config is refused at load time with exit 1. fsdp_auto_wrap: false and fsdp_backward_prefetch: backward_post are still accepted but change nothing under FSDP2 (the plugin always applies the transformer-layer wrap policy and takes no prefetch policy; checked against TrainingArguments on 5.10.1 and 5.13.0). The CLI now logs a warning when either is set away from its default, and the field descriptions, reference pages, user manuals and config_template.yaml say so. The warning lives in cli/_config_load.py because config.py and trainer.py are at their deferred-split line budgets. A new test builds TrainingArguments for all 32 accepted variants through ForgeTrainer._apply_distributed_config, so a config the schema accepts can no longer be one TrainingArguments refuses. Moving to the FSDP2 form (fsdp=True plus an explicit fsdp_config) is not part of this change. Depends on the transformers floor moving to 5.10.1 (#109); the docs say "transformers 5.10+, which ForgeLM requires". Refs #104 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 5 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 8 minutes. View limit details
Comment |
Reviewer's GuideThis PR makes FSDP2 compatibility explicit by rejecting the unsupported Sequence diagram for FSDP configuration loading and dispatchsequenceDiagram
participant CLI
participant ConfigLoader
participant Validator
participant WarningChecker
participant Dispatcher
participant Transformers
CLI->>ConfigLoader: _load_config_or_exit()
ConfigLoader->>Validator: Validate DistributedConfig
alt unsupported fsdp_strategy
Validator-->>ConfigLoader: Validation error
ConfigLoader-->>CLI: EXIT_CONFIG_ERROR
else supported fsdp_strategy
ConfigLoader-->>CLI: config
CLI->>WarningChecker: _warn_inert_fsdp_options(config)
alt inert option configured
WarningChecker-->>CLI: logger.warning()
end
CLI->>Dispatcher: Dispatch training or dry-run
Dispatcher->>Transformers: Build TrainingArguments
Transformers-->>Dispatcher: FSDP2-compatible arguments
end
Flow diagram for FSDP2 configuration validation and warningsflowchart TD
A[Load configuration] --> B{fsdp_strategy supported?}
B -->|no_shard or hybrid_shard| C[Reject configuration with EXIT_CONFIG_ERROR]
B -->|full_shard or shard_grad_op| D[Apply offline flag]
D --> E{FSDP strategy enabled?}
E -->|No| F[Continue dispatch]
E -->|Yes| G{Inert option set?}
G -->|fsdp_auto_wrap=false| H[Log warning]
G -->|fsdp_backward_prefetch=backward_post| I[Log warning]
G -->|Neither| F
H --> F
I --> F
File-Level Changes
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
distributed.fsdp_strategyacceptedno_shardandhybrid_shard(the latter described as the multi-node option). transformers 5.10 made FSDP2 the default, and FSDP2 raises`fsdp=hybrid_shard` is only available with FSDP1whenTrainingArgumentsis built. A config with either value passed--dry-runand then exited 2 after the model was loaded. It is now refused when the config loads (exit 1). Two more knobs that FSDP2 silently ignores now log a warning.Related issues
Refs #104 — the config-time rejection and the "no knob is silently dead" half. Moving to the FSDP2 form (
fsdp=Trueplus an explicitfsdp_config) is deliberately not done here; the string form is deprecated in transformers and slated for removal in v5.20, so that migration remains open on the issue.Merge order: the documentation says "transformers 5.10+, which ForgeLM requires". That is true once #989 (the transformers floor) is merged; please merge it first, or this one will briefly overstate the requirement.
Changes
forgelm/config.py:fsdp_strategyisLiteral["full_shard", "shard_grad_op"]. The three field descriptions say what FSDP2 ignores. All edits are in place, soconfig.py(zero line headroom on the module-size ratchet) is unchanged in size.forgelm/cli/_config_load.py+forgelm/cli/_dispatch.py:_warn_inert_fsdp_options()logs a warning whenfsdp_auto_wrap: falseorfsdp_backward_prefetch: backward_postis set. It lives in the CLI layer for the same budget reason.config_template.yaml.Checked against transformers
Real
TrainingArguments, stringfsdp, on 5.10.1 and 5.13.0:fsdp_strategyfull_shard,shard_grad_opreshard_after_forwardisTrue/False;state_dict_typehonouredno_shard,hybrid_shardValueError("only available with FSDP1")With
fsdp_auto_wrap: falseorbackward_postthe plugin is identical to the default one (alwaysTRANSFORMER_BASED_WRAP, no prefetch argument), which is why those two are warnings and not errors: they are accepted, they change nothing.Type
Testing
tests/test_distributed.py:--dry-runof such a config exitsEXIT_CONFIG_ERROR;TrainingArgumentsthroughForgeTrainer._apply_distributed_config, so what the schema accepts can no longer be somethingTrainingArgumentsrefuses;distributedblock, and a real--dry-runreaches it.main()fails the dry-run test.CLAUDE.mdgauntlet from a clean worktree of this branch: exit 0.Checklist
[Unreleased]🤖 Generated with Claude Code
Summary by Sourcery
Validate FSDP settings against transformers FSDP2 and surface configuration options that no longer have an effect.
Bug Fixes:
Enhancements:
Documentation:
Tests:
Chores: