Repository navigation
Conversation
TRL's trainers call huggingface_hub's `send_telemetry` when they are built, and huggingface_hub sends a small anonymous usage ping to huggingface.co unless `HF_HUB_DISABLE_TELEMETRY` is true or offline mode is on. The privacy page says ForgeLM "does not collect telemetry, does not send usage data anywhere", and the air-gap page said only that ForgeLM has no telemetry service of its own. A default online training run therefore contradicted the page. `main()` now sets `HF_HUB_DISABLE_TELEMETRY=1` before anything imports huggingface_hub, which reads it once at import; importing `forgelm.cli` does not import it (a test pins that, so a later top-level import cannot silently defeat the default). `setdefault` leaves an operator's own value alone, so `HF_HUB_DISABLE_TELEMETRY=0` opts back in. Using ForgeLM as a library does not go through `main()` and is not covered; the air-gap page (English and Turkish) says so. Checked against the installed TRL 1.7.1 and huggingface_hub: both read the variable. Whether huggingface_hub makes other background requests beyond the telemetry ping was not examined. Refs #95 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 33 minutes by commenting |
|
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 35 minutes. View limit details
Comment |
Reviewer's GuideThe PR prevents the Sequence diagram for CLI telemetry suppressionsequenceDiagram
participant Operator
participant CLI as forgelm CLI
participant Dispatch as main()
participant Hub as huggingface_hub
participant HF as huggingface.co
Operator->>CLI: invoke forgelm
CLI->>Dispatch: main()
Dispatch->>Dispatch: _disable_hub_telemetry()
Note over Dispatch: os.environ.setdefault(HF_HUB_DISABLE_TELEMETRY, 1)
Dispatch->>Hub: import after environment setup
Hub->>Hub: send_telemetry()
alt HF_HUB_DISABLE_TELEMETRY=1
Hub-->>HF: no anonymous usage ping
else operator set HF_HUB_DISABLE_TELEMETRY=0
Hub->>HF: anonymous usage ping
end
Flow diagram for CLI and library telemetry behaviorflowchart TD
Start[ForgeLM usage] --> Mode{CLI or library?}
Mode -->|CLI| Main["main()"]
Main --> Default["_disable_hub_telemetry()"]
Default --> Env[setdefault HF_HUB_DISABLE_TELEMETRY=1]
Env --> Trainers[TRL trainers and huggingface_hub]
Trainers --> Suppressed[Anonymous usage ping suppressed]
Mode -->|Library| Library[Library entry point]
Library --> Hub[TRL trainers and huggingface_hub]
Hub --> Caller[Caller configures telemetry behavior]
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
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 |
…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>
Summary
The privacy page says ForgeLM "does not collect telemetry, does not send usage data anywhere". A default online
forgelmrun did send one: TRL's trainers callhuggingface_hub'ssend_telemetrywhen they are built, andhuggingface_hubsends a small anonymous usage ping to huggingface.co unlessHF_HUB_DISABLE_TELEMETRYis true or offline mode is on. This makes the claim true for the CLI instead of softening it.Related issues
Fixes #95 for the CLI. The site's privacy strings need no change for the CLI path; using ForgeLM as a library is covered in the Telemetry row below.
Changes
forgelm/cli/_config_load.py:_disable_hub_telemetry()doesos.environ.setdefault("HF_HUB_DISABLE_TELEMETRY", "1").forgelm/cli/_dispatch.py:main()calls it first, before anything importshuggingface_hub(which reads the variable once, at import).docs/usermanuals/{en,tr}/operations/air-gap.md: the Telemetry row now says what the CLI sets and that library use does not.Trade-offs
setdefaultleaves an operator's own value alone, soHF_HUB_DISABLE_TELEMETRY=0opts back in. This is an environment default for a dependency, not environment-driven ForgeLM behaviour, so it does not break the "config-driven" principle, but it is worth a look.main()and is not covered; the air-gap page says so.huggingface_hubmakes other background requests beyond the telemetry ping was not examined.Type
Testing
TestHubTelemetryDefaultintests/test_cli.py: the variable is set to1bymain(), an existing value is kept, and importingforgelm.clidoes not importhuggingface_hub(a later top-level import would silently defeat the default).huggingface_hub: both read the variable.CLAUDE.mdgauntlet from a clean worktree of this branch: exit 0.Checklist
[Unreleased]🤖 Generated with Claude Code
Summary by Sourcery
Disable Hugging Face anonymous telemetry by default for CLI invocations to keep ForgeLM’s CLI privacy behavior consistent with its documentation.
Bug Fixes:
forgelmCLI from sending Hugging Face’s anonymous usage telemetry by default while preserving explicit operator opt-in values.Documentation:
Tests:
Chores: