Repository navigation
Conversation
…is set `retention.enforce` is documented, validated and offered by both wizards as a gate: `warn_on_excess` "adds a structured warning to stderr" and `block_on_excess` "aborts the trainer pre-flight with EXIT_EVAL_FAILURE (3)" so "a regulated CI gate does not silently extend the retention horizon". No command reads the field. The only consumer of the retention block is `forgelm purge --check-policy`, which reports artefacts past their horizon, always exits 0 and deletes nothing. A regulated team that set `block_on_excess` believed a gate existed. The field description, the RetentionConfig docstring, the wizard prompt, the configuration reference, the GDPR erasure guide, the user-manual reference and `config_template.yaml` now say that the horizons are reported and that `enforce` is reserved (English and Turkish). Loading a config that sets `warn_on_excess` or `block_on_excess` logs one warning saying nothing acts on it, instead of accepting it silently. Implementing the gate (a trainer pre-flight, an audit event and an exit-code decision) stays open on the issue. Refs #92, #98 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…n.enforce warning to the CLI The first version added a validator to RetentionConfig, which pushed forgelm/config.py to 1915 lines against its deferred-split budget of 1903 (tests/test_check_module_size.py caught it). The docstring and field description are now line-neutral and the warning is emitted where the config is loaded by the CLI, which is also where an operator sees it. Refs #92, #98 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 19 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 50 minutes. View limit details
Comment |
Reviewer's GuideThis PR corrects the retention contract to state that Sequence diagram for reserved retention enforcement warningsequenceDiagram
participant CLI
participant _load_config_or_exit
participant load_config
participant _warn_reserved_retention_enforce
participant Logger
CLI->>_load_config_or_exit: _load_config_or_exit(config_path, json_output)
_load_config_or_exit->>load_config: load_config(config_path)
load_config-->>_load_config_or_exit: config
_load_config_or_exit->>_warn_reserved_retention_enforce: _warn_reserved_retention_enforce(config)
alt retention.enforce = log_only
_warn_reserved_retention_enforce-->>_load_config_or_exit: no warning
else retention.enforce = warn_on_excess or block_on_excess
_warn_reserved_retention_enforce->>Logger: warning(mode is accepted but unused)
_warn_reserved_retention_enforce-->>_load_config_or_exit: no enforcement action
end
_load_config_or_exit-->>CLI: config
Flow diagram for read-only retention policy reportingflowchart LR
Config["retention.enforce is reserved"] --> Check["forgelm purge --check-policy"]
Check --> Report["Report horizons and excess artefacts"]
Report --> Exit["Always exit 0"]
Config --> NoAction["No deletion, blocking, warning, or scheduling"]
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
…des lack Second-review finding: the wizard labelled log_only as 'the only mode with an effect to promise', but no retention mode has an effect. It now says log_only is the default and behaves the same as the other two for now. Refs #92 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
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
retention.enforcewas documented, validated and offered by both wizards as a gate:warn_on_excess("structured stderr warning"),block_on_excess("abort trainer pre-flight withEXIT_EVAL_FAILURE= 3"). No command reads it. A config that setblock_on_excessbelieved a stale workspace would stop the run; it never did. This says so everywhere it was promised, and makes the CLI say so at load time rather than accept the value silently.Related issues
Refs #92, #98 — the code, documentation and wizard halves for the CLI and the browser-free wizard. The website copy (
site/js/translations.jsretention strings andsite/js/wizard.js, six locales) is a separate change; whetherenforceshould ever become a real gate is left open on the issues.Changes
forgelm/config.py—RetentionConfigdocstring andenforcedescriptiondocs/reference/configuration{,-tr}.md,docs/usermanuals/{en,tr}/reference/configuration.md,config_template.yamlblock_on_excessaborts withEXIT_EVAL_FAILURE= 3forgelm purge --check-policy, which is read-only and always exits 0docs/guides/gdpr_erasure{,-tr}.mdforgelm/wizard/_collectors.py)log_onlyforgelm/cli/_config_load.py(new)WARNINGwhen a loaded config setswarn_on_excessorblock_on_excess: "accepted but no command acts on it yet"The warning lives in the CLI layer, not the schema:
config.pyis on the module-size ratchet with zero headroom, so the description edits are line-neutral and the behaviour sits in_config_load.py(second commit).What this changes for a deployer
A DPIA or audit answer can no longer cite ForgeLM for "retention is enforced" or "the pipeline aborts on a stale workspace". The pages say what it does give: a report of what is past its horizon.
Not done here
enforcea real gate (a code-and-contract decision).site/js/wizard.js.Type
Testing
tests/test_cli.py::TestReservedRetentionEnforceWarning: silent for the default, one warning naming the mode forwarn_on_excess/block_on_excess, and fired by_load_config_or_exit.tests/test_config.py::TestRetentionConfig: the description no longer promises a gate.CLAUDE.mdgauntlet from a clean worktree of this branch: exit 0.Checklist
[Unreleased]🤖 Generated with Claude Code
Summary by Sourcery
Clarify that retention enforcement modes are reserved and warn users when unsupported non-default modes are configured.
Bug Fixes:
Enhancements:
Documentation:
Tests:
Chores: