Repository navigation
release: v0.5.7 — SFT trainer trl-modernisation fix + roadmap consolidation - #44
Conversation
…ses.md Tidy-up of the docs/roadmap/ directory: every completed phase file (Phase 10 / 10.5 / 11 / 11.5 / 12 / 12.5 / 12.6) is now absorbed into the canonical completed-phases.md archive with full content (heading shift +1 level), and the standalone files are removed. The roadmap directory now carries only the planning surface — completed-phases.md + releases.md + risks-and-decisions.md + the two PLANNED phase files (phase-13-pro-cli.md, phase-14-pipeline-chains.md). Changes: - docs/roadmap/completed-phases.md: 656 → 1285 lines. Replaces the earlier Phase 10 stub with the full phase-10-post-training.md content; appends Phase 10.5, 11, 11.5, 12, 12.5, 12.6 in chronological order, each as a top-level "## Phase N" section. - docs/roadmap/phase-10-post-training.md (deleted) - docs/roadmap/phase-10-5-quickstart.md (deleted) - docs/roadmap/phase-11-data-ingestion.md (deleted) - docs/roadmap/phase-11-5-backlog.md (deleted) - docs/roadmap/phase-12-data-curation-maturity.md (deleted) - docs/roadmap/phase-12-5-backlog.md (deleted) - docs/roadmap/phase-12-6-closure-cycle.md (deleted) References redirected to completed-phases.md (with internal anchors where useful): - CHANGELOG.md (3 references) - docs/roadmap.md + docs/roadmap-tr.md (table + tree visualisation) - docs/roadmap/phase-14-pipeline-chains.md - docs/roadmap/releases.md - docs/standards/documentation.md - docs/standards/release.md - tools/bench_simhash.py - tests/test_phase12_5.py - forgelm/ingestion.py - .claude/skills/add-trainer-feature/SKILL.md Verified: ruff format/check clean, all 6 doc CI guards green (anchor-resolution 266 files, bilingual-parity 45 pairs, cli-help 439 invocations, wizard-defaults 3/8, no-analysis-refs, dry-run). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ⓘ You've reached your Qodo monthly free-tier limit. Reviews pause until next month — upgrade your plan to continue now, or link your paid account if you already have one. |
Reviewer's GuideConsolidates all completed roadmap phase docs (10, 10.5, 11, 11.5, 12, 12.5, 12.6) into docs/roadmap/completed-phases.md, deletes the individual phase files, and updates all internal references and comments to point at the single canonical archive file instead of per-phase markdown files. Flow diagram for docs/roadmap directory structure before vs after consolidationflowchart LR
subgraph Before
direction TB
rp_before["docs/roadmap/"]
rp10_before["phase-10-post-training.md"]
rp105_before["phase-10-5-quickstart.md"]
rp11_before["phase-11-data-ingestion.md"]
rp115_before["phase-11-5-backlog.md"]
rp12_before["phase-12-data-curation-maturity.md"]
rp125_before["phase-12-5-backlog.md"]
rp126_before["phase-12-6-closure-cycle.md"]
rpc_before["completed-phases.md (1-9)"]
rp13_before["phase-13-pro-cli.md"]
rp14_before["phase-14-pipeline-chains.md"]
rprel_before["releases.md"]
rp_before --> rpc_before
rp_before --> rp10_before
rp_before --> rp105_before
rp_before --> rp11_before
rp_before --> rp115_before
rp_before --> rp12_before
rp_before --> rp125_before
rp_before --> rp126_before
rp_before --> rp13_before
rp_before --> rp14_before
rp_before --> rprel_before
end
subgraph After
direction TB
rp_after["docs/roadmap/"]
rpc_after["completed-phases.md (1-12.6)"]
rp13_after["phase-13-pro-cli.md"]
rp14_after["phase-14-pipeline-chains.md"]
rprel_after["releases.md"]
rp_after --> rpc_after
rp_after --> rp13_after
rp_after --> rp14_after
rp_after --> rprel_after
end
Before --> After
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughConsolidates Phase 10–12.6 roadmap into one archive, adds a shared NumPy↔torch ABI checker and doctor probe with a training preflight early-fail, makes ForgeTrainer runtime-detect the correct SFTConfig sequence-length kwarg, adds regression tests, bumps to v0.5.7, and updates cross-references and notebooks. Changesv0.5.7 Release & Documentation Consolidation
Sequence Diagram(s)sequenceDiagram
participant UserCLI as forgelm CLI (_run_training_pipeline)
participant Preflight as _preflight_numpy_torch_abi
participant ABI as forgelm.cli._abi_check
UserCLI->>Preflight: call preflight(json_output)
Preflight->>ABI: compute_numpy_torch_abi_status()
ABI-->>Preflight: (status, torch_ver, numpy_ver)
alt status == ABI_BROKEN
Preflight->>UserCLI: emit structured JSON / log remediation
Preflight-->>UserCLI: SystemExit(EXIT_TRAINING_ERROR)
else
Preflight-->>UserCLI: return (continue startup)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| BestPractice | 1 medium 2 minor |
| CodeStyle | 1 minor |
🟢 Metrics 75 complexity · 2 duplication
Metric Results Complexity 75 Duplication 2
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="docs/roadmap/completed-phases.md" line_range="721-722" />
<code_context>
+> **Filename history:** Originally `phase-12-quickstart.md` (the slot Quickstart held in early planning); renamed to (see relevant Phase section above/below) in the v0.5.2 cycle so the filename matches the current phase number. The Phase 12 slot now belongs to [Data Curation Maturity](#phase-12-data-curation-maturity).
</code_context>
<issue_to_address>
**issue (typo):** Clarify or fix the incomplete "renamed to" clause in the filename history sentence.
The fragment `renamed to (see relevant Phase section above/below)` is grammatically incomplete and confusing. Either (a) specify the new filename explicitly, or (b) remove `to` and rewrite so it just states when/why it was renamed, while keeping the sentence smooth and readable.
```suggestion
> **Note:** This file details a single phase. For a summary of all phases, see [../roadmap.md](../roadmap.md).
> **Filename history:** Originally `phase-12-quickstart.md` (the slot Quickstart held in early planning); renamed during the v0.5.2 cycle so the filename matches the current phase number. The Phase 12 slot now belongs to [Data Curation Maturity](#phase-12-data-curation-maturity).
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Code Review
This pull request consolidates multiple individual roadmap phase files into a single completed-phases.md archive and updates all internal references across documentation, code comments, and tests to point to this new centralized location. The review feedback focuses on cleaning up artifacts from the consolidation process, specifically replacing placeholders like (see relevant Phase section above/below) with proper internal anchor links or phase names, and ensuring language consistency by translating Turkish terms (e.g., "fazlar") to English within the English documentation files.
| > **Status:** ✅ **DONE** — shipped as `v0.4.5` on 2026-04-26 (PyPI). Module: [`forgelm/quickstart.py`](../../forgelm/quickstart.py); five bundled templates under [`forgelm/templates/`](../../forgelm/templates/); CLI: `forgelm quickstart <template>`; tests: [`tests/test_quickstart.py`](../../tests/test_quickstart.py); CI smoke in [nightly.yml](../../.github/workflows/nightly.yml). | ||
|
|
||
| > **Note:** This file details a single phase. For a summary of all phases, see [../roadmap.md](../roadmap.md). | ||
| > **Filename history:** Originally `phase-12-quickstart.md` (the slot Quickstart held in early planning); renamed to (see relevant Phase section above/below) in the v0.5.2 cycle so the filename matches the current phase number. The Phase 12 slot now belongs to [Data Curation Maturity](#phase-12-data-curation-maturity). |
There was a problem hiding this comment.
The placeholder (see relevant Phase section above/below) was likely a relative file link in the original standalone document. Now that the content is consolidated, it should be replaced with the actual phase name.
| > **Filename history:** Originally `phase-12-quickstart.md` (the slot Quickstart held in early planning); renamed to (see relevant Phase section above/below) in the v0.5.2 cycle so the filename matches the current phase number. The Phase 12 slot now belongs to [Data Curation Maturity](#phase-12-data-curation-maturity). | |
| > **Filename history:** Originally `phase-12-quickstart.md` (the slot Quickstart held in early planning); renamed to Phase 10.5 in the v0.5.2 cycle so the filename matches the current phase number. The Phase 12 slot now belongs to [Data Curation Maturity](#phase-12-data-curation-maturity). |
| > and 12.5 — see [releases.md](releases.md#v050-document-ingestion-data-curation-pipeline). | ||
| > Version-label references below preserve the historical planning trail. | ||
| > | ||
| > **Status:** ✅ **Tier 1 DONE** — landed on `development` for the (originally) `v0.5.2` cycle. All five must-have tasks shipped: MinHash LSH dedup option (`[ingestion-scale]` extra via `datasketch`), markdown-aware splitter (`--strategy markdown`), code/secrets leakage tagger (`[ingestion-secrets]` extra via `detect-secrets` with regex fallback), heuristic quality filter (`--quality-filter`), DOCX/Markdown table preservation. Tier 2 (Presidio adapter, Croissant metadata) and Tier 3 (`--all-mask` composite, wizard "audit first" hook) are deferred to a follow-up **Phase 12.5** backlog file (analogous to Phase 11.5 → (see relevant Phase section above/below)). Modules: [`forgelm/data_audit.py`](../../forgelm/data_audit/), [`forgelm/ingestion.py`](../../forgelm/ingestion.py), [`forgelm/cli.py`](../../forgelm/cli/); tests: [`tests/test_data_audit_phase12.py`](../../tests/test_data_audit_phase12.py), [`tests/test_ingestion_phase12.py`](../../tests/test_ingestion_phase12.py); CLI tests added in [`tests/test_cli_subcommands.py`](../../tests/test_cli_subcommands.py). |
There was a problem hiding this comment.
The placeholder (see relevant Phase section above/below) should be replaced with an internal anchor link to the Phase 11.5 section within this document.
| > **Status:** ✅ **Tier 1 DONE** — landed on `development` for the (originally) `v0.5.2` cycle. All five must-have tasks shipped: MinHash LSH dedup option (`[ingestion-scale]` extra via `datasketch`), markdown-aware splitter (`--strategy markdown`), code/secrets leakage tagger (`[ingestion-secrets]` extra via `detect-secrets` with regex fallback), heuristic quality filter (`--quality-filter`), DOCX/Markdown table preservation. Tier 2 (Presidio adapter, Croissant metadata) and Tier 3 (`--all-mask` composite, wizard "audit first" hook) are deferred to a follow-up **Phase 12.5** backlog file (analogous to Phase 11.5 → (see relevant Phase section above/below)). Modules: [`forgelm/data_audit.py`](../../forgelm/data_audit/), [`forgelm/ingestion.py`](../../forgelm/ingestion.py), [`forgelm/cli.py`](../../forgelm/cli/); tests: [`tests/test_data_audit_phase12.py`](../../tests/test_data_audit_phase12.py), [`tests/test_ingestion_phase12.py`](../../tests/test_ingestion_phase12.py); CLI tests added in [`tests/test_cli_subcommands.py`](../../tests/test_cli_subcommands.py). | |
| Tier 2 (Presidio adapter, Croissant metadata) and Tier 3 (--all-mask composite, wizard "audit first" hook) are deferred to a follow-up **Phase 12.5** backlog file (analogous to [Phase 11.5](#phase-115-ingestion-audit-polish)). Modules: [forgelm/data_audit.py](../../forgelm/data_audit/), [forgelm/ingestion.py](../../forgelm/ingestion.py), [forgelm/cli.py](../../forgelm/cli/); tests: [tests/test_data_audit_phase12.py](../../tests/test_data_audit_phase12.py), [tests/test_ingestion_phase12.py](../../tests/test_ingestion_phase12.py); CLI tests added in [tests/test_cli_subcommands.py](../../tests/test_cli_subcommands.py). |
|
|
||
| ### Out of scope (still) | ||
|
|
||
| The "Won't-do" list at the bottom of [(see Phase 12 — Data Curation Maturity section)](#phase-12-data-curation-maturity) is not weakened by this backlog. VLM PDF parsing, embedding-based semantic dedup, built-in OCR, ML quality classifiers, GPU/Ray scale, and HTML extractor stay out of scope as deliberately as Phase 12 declared them. |
There was a problem hiding this comment.
The link text here is awkward and contains a parenthetical placeholder. It should be cleaned up to use natural phrasing.
| The "Won't-do" list at the bottom of [(see Phase 12 — Data Curation Maturity section)](#phase-12-data-curation-maturity) is not weakened by this backlog. VLM PDF parsing, embedding-based semantic dedup, built-in OCR, ML quality classifiers, GPU/Ray scale, and HTML extractor stay out of scope as deliberately as Phase 12 declared them. | |
| The "Won't-do" list at the bottom of the [Phase 12 — Data Curation Maturity](#phase-12-data-curation-maturity) section is not weakened by this backlog. VLM PDF parsing, embedding-based semantic dedup, built-in OCR, ML quality classifiers, GPU/Ray scale, and HTML extractor stay out of scope as deliberately as Phase 12 declared them. |
|
|
||
| --- | ||
|
|
||
| ## Phase 12.6 — Closure Cycle (38 fazlar across 5 waves) |
There was a problem hiding this comment.
This file is the canonical English archive of completed phases, but the Phase 12.6 section contains several Turkish terms (e.g., "fazlar", "faz") and introductory notes (lines 645, 786, 934) that were absorbed during consolidation. These should be translated to English for consistency.
| ## Phase 12.6 — Closure Cycle (38 fazlar across 5 waves) | |
| ## Phase 12.6 — Closure Cycle (38 phases across 5 waves) |
| | ✅ Done | [Phase 10.5 — Quickstart Layer & Onboarding](roadmap/completed-phases.md) | `forgelm quickstart <template>`, 5 bundled templates with seed datasets — shipped `v0.4.5` | | ||
| | ✅ Done | [Phase 11 + 11.5 + 12 + 12.5 — Document Ingestion & Data Curation Pipeline](roadmap/releases.md#v050-document-ingestion-data-curation-pipeline) | `forgelm ingest`, `forgelm audit`, PII regex + simhash dedup, LSH banding, streaming reader, PII severity tiers, wizard ingest+audit, MinHash LSH dedup, markdown splitter, code/secrets scan, quality heuristics, DOCX table preservation, `--all-mask`, Croissant 1.0, Presidio NER — shipped `v0.5.0` (PyPI 2026-04-30) | | ||
| | ✅ Done | [Phase 12.6 — Closure Cycle (37 content fazlar + 1 release tag = 38 entries across 5 waves)](roadmap/phase-12-6-closure-cycle.md) | Library API, GDPR purge + reverse-pii, ISO 27001 / SOC 2 alignment, doctor + cache subcommands, compliance verification toolbelt, bilingual mirror sweep + 4 CI guards, supply-chain security, cross-OS release matrix — bundled into `v0.5.5` (PyPI 2026-05-10) | | ||
| | ✅ Done | [Phase 12.6 — Closure Cycle (37 content fazlar + 1 release tag = 38 entries across 5 waves)](roadmap/completed-phases.md) | Library API, GDPR purge + reverse-pii, ISO 27001 / SOC 2 alignment, doctor + cache subcommands, compliance verification toolbelt, bilingual mirror sweep + 4 CI guards, supply-chain security, cross-OS release matrix — bundled into `v0.5.5` (PyPI 2026-05-10) | |
There was a problem hiding this comment.
The term "fazlar" is Turkish and should be translated to "phases" to maintain language consistency in this English roadmap file.
| | ✅ Done | [Phase 12.6 — Closure Cycle (37 content fazlar + 1 release tag = 38 entries across 5 waves)](roadmap/completed-phases.md) | Library API, GDPR purge + reverse-pii, ISO 27001 / SOC 2 alignment, doctor + cache subcommands, compliance verification toolbelt, bilingual mirror sweep + 4 CI guards, supply-chain security, cross-OS release matrix — bundled into `v0.5.5` (PyPI 2026-05-10) | | |
| | ✅ Done | [Phase 12.6 — Closure Cycle (37 content phases + 1 release tag = 38 entries across 5 waves)](roadmap/completed-phases.md) | Library API, GDPR purge + reverse-pii, ISO 27001 / SOC 2 alignment, doctor + cache subcommands, compliance verification toolbelt, bilingual mirror sweep + 4 CI guards, supply-chain security, cross-OS release matrix — bundled into v0.5.5 (PyPI 2026-05-10) | |
…cleanup PR #44 review absorption: gemini-code-assist + sourcery-ai flagged five medium issues introduced by the consolidation script's intermediate placeholders + Turkish-term drift in the now-canonical English-only completed-phases.md archive. Fixed (5): - completed-phases.md:722 "renamed to (see relevant Phase section above/below)" → "renamed to its current filename slug" (the filename history sentence is now grammatical and self-contained) - completed-phases.md:932 "(analogous to Phase 11.5 → (see relevant Phase section above/below))" → resolves to the in-document anchor "[Phase 11.5 section above](#phase-115-ingestion-audit-polish)" - completed-phases.md:1133 "[(see relevant Phase section above/below)] (#section-below)" → "[Phase 12 — Data Curation Maturity] (#phase-12-data-curation-maturity)" - completed-phases.md (Phase 12.6 closure-cycle section) "fazlar" / "Faz 33" / "Faz 30" / "Faz inventory" → "tasks" / "Task 33" / "Task 30" / "Task inventory". Closure-cycle had 38 numbered tasks internally; "task" reads naturally and avoids overlap with the outer "Phase 12.6" heading. - completed-phases.md (Phase 10/11/12 sections) — three orphaned Turkish "Not: Bu dosya tek bir planlanan fazı detaylandırır..." intro lines absorbed from the deleted phase files. Removed (the claim is no longer true after consolidation); the Phase 12 lineage note is preserved in English ("Phase 11 + 11.5 built the ingestion / audit lineage ..."). Also fixed: - docs/roadmap.md:13 "37 content fazlar + 1 release tag" → "37 content phases + 1 release tag" - docs/roadmap.md:36 "Phase 12.6 / Faz dual-axis note" → "Phase 12.6 task / sub-task dual-axis note"; "(Faz 1-38)" → "(Tasks 1-38)" - CHANGELOG.md (v0.5.5 [0.5.5] section) "**`forgelm/_http.safe_post`** (Faz 7 carry-over)" → "**`forgelm/_http.safe_post`**" (drop the internal-only Wave/Faz parenthetical; the surrounding paragraph already explains the carry-over) Skipped: - *gemini-code-assist on docs/roadmap.md TR mirror "fazlar"* — N/A, docs/roadmap-tr.md is the Turkish mirror and "fazlar" is the correct Turkish term there. - *CHANGELOG.md:1118 v0.5.0 section "(Faz 12)"* — historical entry, out of scope for the v0.5.5 cleanup the user authorized. Verified: all 6 doc CI guards green (anchor 266 files, parity 45 pairs, cli-help 439 invocations, wizard-defaults 3/8, no-analysis-refs, dry-run). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Fixes a runtime TypeError that prevented every SFT training run from starting on modern trl (0.13+ and the 1.x line). The max_seq_length parameter was renamed to max_length on SFTConfig in trl 0.13 and the old name removed; v0.5.6 still passed max_seq_length unconditionally, so forgelm --config <yaml> crashed with: TypeError: SFTConfig.__init__() got an unexpected keyword argument 'max_seq_length' on any environment that pulled a current trl wheel — notably the Colab default `pip install forgelm` path. Fix: forgelm/trainer.py::_get_training_args_for_type's SFT branch now inspects SFTConfig.__init__'s signature at runtime and picks the correct parameter name. trl 0.13+ receives max_length; trl 0.12.x (still within pyproject's `trl>=0.12.0,<2.0.0` floor) keeps receiving max_seq_length. No code change for DPO / SimPO / KTO / ORPO / GRPO trainers — those *Config parameters were not affected by the rename. Three new regression tests in tests/test_trainer_sft_config.py pin the modern-trl path, the legacy-trl path, and that packing / dataset_text_field continue to be propagated. Other release-prep updates: - pyproject.toml: 0.5.6 → 0.5.7 - forgelm/_version.py: rationale comment updated - CHANGELOG.md: [0.5.7] entry inserted above [0.5.6] - docs/roadmap/releases.md: v0.5.7 entry inserted above v0.5.6 - notebooks/*.ipynb: install pins bumped 0.5.6 → 0.5.7 (10 files) Verified: ruff format/check clean, all 3 doc CI guards green (anchor 266 files, parity 45 pairs, no-analysis-refs). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
docs/roadmap/completed-phases.md (1)
653-764: ⚡ Quick winNormalize subsection heading levels and labels to avoid lint churn.
Several sections use
#### Tasks/Requirements/Deliverydirectly under## Phase ..., which triggers heading-jump and duplicate-heading warnings repeatedly. Promote these to###and make labels phase-specific (or otherwise unique) to keep anchors/lint stable.Suggested pattern
-#### Tasks: +### Tasks (Phase 10): -#### Requirements: +### Requirements (Phase 10): -#### Delivery: +### Delivery (Phase 10):🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/roadmap/completed-phases.md` around lines 653 - 764, The document uses repeated "#### Tasks/Requirements/Delivery" headings under "## Phase ..." causing lint heading-jump and duplicate-heading warnings; rename those subsections to unique third-level headings (e.g., "### Phase 10.5 — Tasks", "### Phase 10.5 — Requirements", "### Phase 10.5 — Delivery") and update their labels/anchors accordingly so they are unique per phase and preserve nesting; ensure any internal cross-references or TOC entries that referenced the old "####" labels are updated to the new "### Phase 10.5 ..." titles to avoid broken anchors and lint churn.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/roadmap-tr.md`:
- Line 13: Update the Turkish roadmap line that currently reads "Faz 12.6 —
Kapanış Döngüsü (5 wave boyunca 38 faz)" to match the EN mirror's semantic
phrasing; replace the ambiguous "38 faz" with "37 içerik fazı + 1 sürüm etiketi
= 38 kayıt" in the string that contains "✅ Tamam | [Faz 12.6 — Kapanış Döngüsü
(5 wave boyunca 38 faz)]" (the same line that links to
roadmap/completed-phases.md) so the Turkish user-facing text mirrors the English
wording and removes the counting ambiguity.
In `@docs/roadmap.md`:
- Line 36: Add a Turkish mirror of the “Phase 12.6 task / sub-task dual-axis
note” that was added to the English roadmap: copy the user-facing clarification
text about Phase 12.6 being a 38-task closure cycle (Tasks 1–38) and the
per-wave PR delta note into the Turkish roadmap page (roadmap-tr), translating
the content into Turkish, and place it in the same Phase 12.6 section so both
roadmap pages remain bilingual and in sync.
In `@docs/roadmap/completed-phases.md`:
- Around line 719-720: Update the stale note at the top that reads “This file
details a single phase” to accurately state that this document is a multi-phase
archive; replace that sentence with something like “This file is a multi-phase
archive of completed roadmap phases; for a summary of all phases see
../roadmap.md” and remove or update any surrounding phrasing referencing a
single phase so the header and the following “Filename history” paragraph remain
correct (find and edit the paragraph containing the single-phase note shown in
the diff).
In `@docs/roadmap/phase-14-pipeline-chains.md`:
- Line 73: Update the visible link text that currently reads
"phase-12-6-closure-cycle.md" so it matches the actual target file
"completed-phases.md" and add an anchor to the Phase 12.6 section (e.g.,
completed-phases.md#phase-12-6 or similar) to avoid confusion; locate the link
in the sentence mentioning Target release `v0.6.0` in
docs/roadmap/phase-14-pipeline-chains.md and replace the label while preserving
the existing URL target (or replace the target with
completed-phases.md#phase-12-6 if appropriate) so the displayed text and href
are consistent.
In `@docs/roadmap/releases.md`:
- Around line 131-149: Update the "Latest release" lines in docs/roadmap.md and
docs/roadmap-tr.md to match the new release v0.5.7 to avoid contradictory docs;
locate the textual lines that currently mention v0.5.5 (look for "Latest
release" or that exact version string) and replace them with "v0.5.7 — Released
to PyPI 2026-05-10" (or the same short wording used in docs/roadmap/releases.md)
and ensure any accompanying date or link text is updated consistently.
---
Nitpick comments:
In `@docs/roadmap/completed-phases.md`:
- Around line 653-764: The document uses repeated "####
Tasks/Requirements/Delivery" headings under "## Phase ..." causing lint
heading-jump and duplicate-heading warnings; rename those subsections to unique
third-level headings (e.g., "### Phase 10.5 — Tasks", "### Phase 10.5 —
Requirements", "### Phase 10.5 — Delivery") and update their labels/anchors
accordingly so they are unique per phase and preserve nesting; ensure any
internal cross-references or TOC entries that referenced the old "####" labels
are updated to the new "### Phase 10.5 ..." titles to avoid broken anchors and
lint churn.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 39dc7d46-a6c3-4dd5-b25b-8fec2bd05cf4
📒 Files selected for processing (33)
.claude/skills/add-trainer-feature/SKILL.mdCHANGELOG.mddocs/roadmap-tr.mddocs/roadmap.mddocs/roadmap/completed-phases.mddocs/roadmap/phase-10-5-quickstart.mddocs/roadmap/phase-10-post-training.mddocs/roadmap/phase-11-5-backlog.mddocs/roadmap/phase-11-data-ingestion.mddocs/roadmap/phase-12-5-backlog.mddocs/roadmap/phase-12-6-closure-cycle.mddocs/roadmap/phase-12-data-curation-maturity.mddocs/roadmap/phase-14-pipeline-chains.mddocs/roadmap/releases.mddocs/standards/documentation.mddocs/standards/release.mdforgelm/_version.pyforgelm/ingestion.pyforgelm/trainer.pynotebooks/data_curation.ipynbnotebooks/dpo_alignment.ipynbnotebooks/galore_memory_optimization.ipynbnotebooks/grpo_reasoning.ipynbnotebooks/kto_binary_feedback.ipynbnotebooks/multi_dataset.ipynbnotebooks/post_training_workflow.ipynbnotebooks/quickstart_sft.ipynbnotebooks/safety_evaluation.ipynbnotebooks/synthetic_data_training.ipynbpyproject.tomltests/test_phase12_5.pytests/test_trainer_sft_config.pytools/bench_simhash.py
💤 Files with no reviewable changes (7)
- docs/roadmap/phase-11-5-backlog.md
- docs/roadmap/phase-11-data-ingestion.md
- docs/roadmap/phase-12-5-backlog.md
- docs/roadmap/phase-10-5-quickstart.md
- docs/roadmap/phase-12-data-curation-maturity.md
- docs/roadmap/phase-10-post-training.md
- docs/roadmap/phase-12-6-closure-cycle.md
…bi doctor probe
The v0.5.6 torch revert (>=2.3 → >=2.2) restored Intel Mac (x86_64)
installability but exposed a transitive ABI mismatch: torch 2.2 was
compiled against NumPy 1.x, so any host that pip-resolves numpy>=2
silently degrades torch's numpy bridge with `_ARRAY_API not found`.
Intel Mac is the canonical victim — PyTorch Foundation no longer
publishes torch >= 2.3 wheels for x86_64 Darwin, so pip caps at
2.2.x and any modern numpy on the box mismatches.
- pyproject.toml: add `numpy<2; sys_platform == 'darwin' and
platform_machine == 'x86_64'` so pip caps numpy at 1.x on the
only platform that is wheel-locked to torch 2.2. Other platforms
keep numpy unconstrained — torch 2.3+ on Linux / Apple Silicon /
Windows is binary-compatible with NumPy 2.x.
- forgelm/cli/subcommands/_doctor.py: new `numpy.torch_abi` probe
detects the (torch < 2.3) × (numpy >= 2) combination and emits a
structured `fail` with a `pip install 'numpy<2'` remediation hint
instead of leaving operators chasing torch's stderr UserWarning.
Probe imports torch / numpy with warnings suppressed and tolerates
prerelease + local-version suffixes via a regex MAJOR.MINOR parse.
- tests/test_doctor.py: 6 new TestNumpyTorchAbiCheck cases — fail
path, two pass paths, no-numpy / no-torch graceful skips, and a
prerelease/local-version regression case.
- CHANGELOG.md: v0.5.7 entry extended with the new Added (probe) +
Fixed (pyproject pin) bullets; v0.5.7 release notes preface
expanded to call out both the SFT trl fix and the NumPy ABI fix.
- docs/usermanuals/{en,tr}/reference/json-output.md: probe-name
example list updated to include `numpy.torch_abi`; H2/H3/H4 spine
unchanged so bilingual parity guard stays clean.
Verified live on Intel Mac: `python3 -m forgelm.cli doctor` now
reports `[x fail] numpy.torch_abi torch 2.2.2 (compiled against
NumPy 1.x) is paired with numpy 2.4.4. ...` — exit code 1, ready
to gate CI consumers via --output-format json.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Verified each finding against current code. Five valid; one nitpick skipped with reason. Fixed (5): - docs/roadmap-tr.md:13 TR mirror "5 wave boyunca 38 faz" → "5 wave boyunca 37 içerik fazı + 1 sürüm etiketi = 38 kayıt" (semantic alignment with EN line 13's explicit task count). - docs/roadmap-tr.md:36 added Turkish mirror of the EN "Phase 12.6 task / sub-task dual-axis note" — the TR roadmap was missing the line entirely after EN line 36's addition in commit e080da6. - docs/roadmap/completed-phases.md:719 removed the orphaned "This file details a single phase" note inside the Phase 10.5 section. The note is a leftover from when phase-10-5-quickstart.md was a standalone file; after consolidation, the archive is multi-phase, so the note was actively misleading. The Filename history paragraph below it is preserved (still accurate) and gains a "absorbed into this multi-phase archive in the v0.5.7 cycle" clarification. - docs/roadmap/phase-14-pipeline-chains.md:73 link text "phase-12-6-closure-cycle.md" (stale filename) → "Phase 12.6 closure cycle archive"; href now resolves to the in-document anchor "completed-phases.md#phase-126-closure-cycle-38-tasks-across-5-waves". - docs/roadmap.md + docs/roadmap-tr.md "Latest release on PyPI" / "PyPI'deki son sürüm" lines updated from v0.5.5 to chain v0.5.7 → v0.5.6 → v0.5.5 with the "what each patch fixed" framing (Intel Mac install fix, SFT trainer trl-modernisation fix). The EN and TR copy stay in sync. Skipped (1): - *Nitpick on completed-phases.md:653-764 duplicate "#### Tasks/Requirements/Delivery" headings.* Deferred — fixing would require renaming ~25 H4s across the absorbed Phase 10/10.5/11 sections to "### Phase X — Tasks/Requirements/Delivery" patterns (and updating any internal cross-references). The repository's anchor-resolution gate already passes at strict mode and GitHub renders the duplicates fine; this is pure markdownlint cosmetic noise. Worth a separate refactor PR if/when a markdownlint-strict policy is adopted. Verified: all 3 doc CI guards green (anchor 266 files, parity 45 pairs, no-analysis-refs). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three concerns from the round-3 cross-review of v0.5.7's review-cycle
follow-ups: a hard-fail upgrade to the SFT signature-detection branch,
a training-pipeline preflight that turns the NumPy 2 / torch 2.2 ABI
mismatch into a single-line actionable error instead of a cryptic
mid-training `NameError: name '_C' is not defined`, and a pre-existing
test-pollution cascade that hid 35 unrelated failures behind a
half-loaded `torch._C` whenever the full suite ran.
- `forgelm/trainer.py::_get_training_args_for_type` SFT branch now
raises `ValueError` when neither `max_length` nor `max_seq_length`
is on `SFTConfig`'s signature; warn-and-continue was a silent-loss
hole against an explicit YAML setting.
- New `forgelm/cli/_abi_check.py` carries the shared verdict +
remediation string; the doctor probe and the new
`_preflight_numpy_torch_abi` in `forgelm/cli/_training.py` both
consume it. Preflight is a no-op on healthy platforms.
- `tests/test_doctor.py` + `tests/test_quickstart_compat.py` swap
bare `sys.modules.pop("torch"/"numpy")` for `monkeypatch.delitem`
so the stranded modules no longer poison every later
`from trl import SFTConfig` in the same pytest session.
- Roadmap stale-ref cleanup: completed-phases.md and
risks-and-decisions.md no longer cite the deleted phase-11-5-backlog.md
and phase-12-data-curation-maturity.md files; three leftover "faz"
strings in the Phase 12.6 section now read "task".
Verified: 1648 passed, 14 skipped, 0 failed (project floor 40% met
at 73.81% coverage); ruff format + ruff check + all five doc guards
(anchor-resolution, no-analysis-refs, bilingual-parity, CLI help,
wizard-defaults) green.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Around line 75-87: In the 0.5.7 changelog block there are duplicate level-3
headings ("### Added" and "### Fixed") causing markdownlint MD024; edit
CHANGELOG.md so headings are unique—either merge the two "### Added" entries
into a single "### Added" section, rename one of the duplicates (e.g., use "###
Changed" or demote to "####") or restructure the "### Fixed" content under the
existing headings; ensure the new forgelm doctor numpy.torch_abi entry remains
under a single unique heading to satisfy linting.
In `@docs/roadmap-tr.md`:
- Line 30: The document currently declares EXIT_WIZARD_CANCELLED = 5 which
violates the repository exit-code contract (allowed values 0–4); update the text
to use an approved exit code (e.g., set EXIT_WIZARD_CANCELLED = 4 or another
value in 0..4) or explicitly point readers to the canonical error-handling table
(docs/standards/error-handling.md) and note that the contract was changed before
keeping 5—ensure the symbol EXIT_WIZARD_CANCELLED and the surrounding sentence
reflect the chosen compliant value or link to the canonical contract.
In `@docs/roadmap.md`:
- Line 30: The docs line that reads "Latest release on PyPI: `v0.5.7` — ...
(2026-05-10)" incorrectly claims the release is live; update the sentence
referencing the literal token `v0.5.7` (and the date) to indicate the release is
pending (e.g. "v0.5.7 (pending publish)" or revert to the currently published
version like "Latest release on PyPI: `v0.5.6`") and remove or change the
release date so the roadmap does not state a published date until the
tag/publish completes.
In `@docs/roadmap/completed-phases.md`:
- Around line 926-927: The Phase 12 Tier 1 summary still claims an extra
"[ingestion-secrets]" and use of "detect-secrets" which has been removed from
the package surface; update the sentence in the Phase 12 status paragraph so it
no longer advertises that install option — either rephrase it as historical
context (e.g., "previously used detect-secrets as an extra") or change it to
state that the secret detection now uses the regex fallback only, and update
references to modules like forgelm/ingestion.py and forgelm/data_audit.py
accordingly so readers aren't misled about an available "[ingestion-secrets]"
extra.
In `@forgelm/cli/_training.py`:
- Around line 72-76: The ABI preflight call _preflight_numpy_torch_abi runs
outside the module's guarded CLI error path so unexpected failures bypass the
JSON error envelope; wrap the call (or move it) into the same guarded error
handling used for the rest of the CLI so any exception from
compute_numpy_torch_abi_status or _preflight_numpy_torch_abi is caught (catch
Exception, not just ImportError) and forwarded to the CLI error handler/exit
path (use the existing CLI error handler and named exit-code constants) so the
failure emits the structured JSON envelope and correct exit code.
In `@forgelm/cli/subcommands/_doctor.py`:
- Around line 273-279: Replace the assert with explicit error handling: check if
status != ABI_OK and return a failing _CheckResult with
name=_PROBE_NUMPY_TORCH_ABI, status=_STATUS_FAIL, an informative detail message
including the unexpected status and the torch/numpy versions, and include
extras; otherwise return the existing passing _CheckResult. Use the same pattern
as other probes (_check_python_version, _check_torch_cuda) and reference
_CheckResult, _STATUS_FAIL, _STATUS_PASS, and ABI_OK to implement the
conditional branch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 871e2ee8-dcb7-4385-abce-fc8bed54341f
📒 Files selected for processing (18)
CHANGELOG.mddocs/roadmap-tr.mddocs/roadmap.mddocs/roadmap/completed-phases.mddocs/roadmap/phase-14-pipeline-chains.mddocs/roadmap/risks-and-decisions.mddocs/usermanuals/en/reference/json-output.mddocs/usermanuals/tr/reference/json-output.mdforgelm/_version.pyforgelm/cli/_abi_check.pyforgelm/cli/_training.pyforgelm/cli/subcommands/_doctor.pyforgelm/trainer.pypyproject.tomltests/test_abi_check.pytests/test_doctor.pytests/test_quickstart_compat.pytests/test_trainer_sft_config.py
✅ Files skipped from review due to trivial changes (5)
- docs/roadmap/risks-and-decisions.md
- docs/usermanuals/tr/reference/json-output.md
- docs/usermanuals/en/reference/json-output.md
- docs/roadmap/phase-14-pipeline-chains.md
- forgelm/_version.py
🚧 Files skipped from review as they are similar to previous changes (1)
- forgelm/trainer.py
| Başlangıçta dört ardışık PyPI tag'i (`v0.5.0` / `v0.5.1` / `v0.5.2` / `v0.5.3`) olarak planlandı, dört faz tek tutarlı yüzey (yut → cila → olgunlaş → cila) oluşturduğu için tek kapsamlı `v0.5.0` release'inde birleştirildi. | ||
|
|
||
| **PyPI'deki son sürüm:** `v0.5.5` — "Kapanış Döngüsü Paketi + Faz 22 Sihirbaz + Site Dokümantasyon Taraması" (2026-05-10). Şunları paketler: **Faz 12.6** (Library API + GDPR purge / reverse-pii + ISO 27001 / SOC 2 alignment + doctor / cache / safety-eval / verify-* subcommand'leri + cross-OS release matrix + supply-chain security baseline; [#14 webhook SSRF hardening](https://github.com/cemililik/ForgeLM/issues/14) follow-up'ını kapsar), **Faz 22** (CLI sihirbazı tarayıcı yüzeyiyle eşdeğerlik + `--wizard-start-from` ile idempotent yeniden çalıştırma + şema-güdümlü varsayılanlar SOT + ayrı `EXIT_WIZARD_CANCELLED = 5` exit kodu) ve `site/*.html` üzerindeki tüm görünür YAML / artefakt-yolu / CLI / şema iddialarını live kod yüzeyine hizalayan ve altı-dil i18n paritesini eski haline getiren bir **site dokümantasyon düzeltme taraması**. | ||
| **PyPI'deki son sürüm:** `v0.5.7` — "SFT trainer trl-modernizasyon düzeltmesi" (2026-05-10), `v0.5.6` (Intel Mac kurulum düzeltmesi) ve `v0.5.5` "Kapanış Döngüsü Paketi + Faz 22 Sihirbaz + Site Dokümantasyon Taraması" (2026-05-10) üzerine gelen bir takip patch'i. v0.5.5 şunları paketler: **Faz 12.6** (Library API + GDPR purge / reverse-pii + ISO 27001 / SOC 2 alignment + doctor / cache / safety-eval / verify-* subcommand'leri + cross-OS release matrix + supply-chain security baseline; [#14 webhook SSRF hardening](https://github.com/cemililik/ForgeLM/issues/14) follow-up'ını kapsar), **Faz 22** (CLI sihirbazı tarayıcı yüzeyiyle eşdeğerlik + `--wizard-start-from` ile idempotent yeniden çalıştırma + şema-güdümlü varsayılanlar SOT + ayrı `EXIT_WIZARD_CANCELLED = 5` exit kodu) ve `site/*.html` üzerindeki tüm görünür YAML / artefakt-yolu / CLI / şema iddialarını live kod yüzeyine hizalayan ve altı-dil i18n paritesini eski haline getiren bir **site dokümantasyon düzeltme taraması**. v0.5.6, v0.5.5'teki `torch>=2.3` tabanını `torch>=2.2`'ye düşürerek Intel Mac (x86_64) kurulabilirliğini geri kazandırır. v0.5.7, trl 0.13'ün `SFTConfig.max_seq_length` → `SFTConfig.max_length` yeniden adlandırmasından kaynaklanan SFT trainer runtime `TypeError` hatasını düzeltir. |
There was a problem hiding this comment.
Exit-code contract is documented inconsistently
Line 30 states EXIT_WIZARD_CANCELLED = 5, but the public contract in this PR’s coding guidelines constrains exit codes to 0/1/2/3/4. Please align this line with the contract (or explicitly point to the canonical error-handling table if the contract changed).
As per coding guidelines, “Error paths must follow [docs/standards/error-handling.md] with exit codes constrained to 0/1/2/3/4 as a public CI/CD contract”.
🧰 Tools
🪛 LanguageTool
[misspelling] ~30-~30: Söz ve sayı arasında defis yoqtur: "v-0"
Context: ...irleştirildi. PyPI'deki son sürüm: v0.5.7 — "SFT trainer trl-modernizasyon d...
(NUMBER_BEFORE_DEFIS_MISSING)
[misspelling] ~30-~30: Söz ve sayı arasında defis yoqtur: "v-0"
Context: ...modernizasyon düzeltmesi" (2026-05-10), v0.5.6 (Intel Mac kurulum düzeltmesi) ve ...
(NUMBER_BEFORE_DEFIS_MISSING)
[misspelling] ~30-~30: Söz ve sayı arasında defis yoqtur: "v-0"
Context: ....5.6(Intel Mac kurulum düzeltmesi) vev0.5.5` "Kapanış Döngüsü Paketi + Faz 22 S...
(NUMBER_BEFORE_DEFIS_MISSING)
[misspelling] ~30-~30: Söz ve sayı arasında defis yoqtur: "v-0"
Context: ...5-10) üzerine gelen bir takip patch'i. v0.5.5 şunları paketler: Faz 12.6 (Lib...
(NUMBER_BEFORE_DEFIS_MISSING)
[misspelling] ~30-~30: Söz ve sayı arasında defis yoqtur: "v-0"
Context: ...ite dokümantasyon düzeltme taraması**. v0.5.6, v0.5.5'teki torch>=2.3 tabanını ...
(NUMBER_BEFORE_DEFIS_MISSING)
[misspelling] ~30-~30: Söz ve sayı arasında defis yoqtur: "v-0"
Context: ...mantasyon düzeltme taraması**. v0.5.6, v0.5.5'teki torch>=2.3 tabanını `torch>=...
(NUMBER_BEFORE_DEFIS_MISSING)
[misspelling] ~30-~30: Söz ve sayı arasında defis yoqtur: "v-0"
Context: ...64) kurulabilirliğini geri kazandırır. v0.5.7, trl 0.13'ün `SFTConfig.max_seq_len...
(NUMBER_BEFORE_DEFIS_MISSING)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/roadmap-tr.md` at line 30, The document currently declares
EXIT_WIZARD_CANCELLED = 5 which violates the repository exit-code contract
(allowed values 0–4); update the text to use an approved exit code (e.g., set
EXIT_WIZARD_CANCELLED = 4 or another value in 0..4) or explicitly point readers
to the canonical error-handling table (docs/standards/error-handling.md) and
note that the contract was changed before keeping 5—ensure the symbol
EXIT_WIZARD_CANCELLED and the surrounding sentence reflect the chosen compliant
value or link to the canonical contract.
| Originally planned as four sequential PyPI tags (`v0.5.0` / `v0.5.1` / `v0.5.2` / `v0.5.3`), consolidated into one comprehensive `v0.5.0` release because the four phases form one coherent surface (ingest → polish → mature → polish) hard to use in parts. | ||
|
|
||
| **Latest release on PyPI:** `v0.5.5` — "Closure Cycle Bundle + Phase 22 Wizard + Site Documentation Sweep" (2026-05-10). Bundles **Phase 12.6** (Library API + GDPR purge / reverse-pii + ISO 27001 / SOC 2 alignment + doctor / cache / safety-eval / verify-* subcommands + cross-OS release matrix + supply-chain security baseline; folds in [#14 webhook SSRF hardening](https://github.com/cemililik/ForgeLM/issues/14)), **Phase 22** (CLI wizard parity with the in-browser surface + idempotent `--wizard-start-from` re-run + schema-driven defaults SOT + distinct `EXIT_WIZARD_CANCELLED = 5` exit code), and a **site documentation correction sweep** that brings every visible YAML / artefact-path / CLI / schema claim on `site/*.html` in line with the live code surface and restores six-language i18n parity. | ||
| **Latest release on PyPI:** `v0.5.7` — "SFT trainer trl-modernisation fix" (2026-05-10), a follow-up patch on top of `v0.5.6` (Intel Mac install fix) and the headline `v0.5.5` "Closure Cycle Bundle + Phase 22 Wizard + Site Documentation Sweep" (2026-05-10). v0.5.5 bundled **Phase 12.6** (Library API + GDPR purge / reverse-pii + ISO 27001 / SOC 2 alignment + doctor / cache / safety-eval / verify-* subcommands + cross-OS release matrix + supply-chain security baseline; folds in [#14 webhook SSRF hardening](https://github.com/cemililik/ForgeLM/issues/14)), **Phase 22** (CLI wizard parity with the in-browser surface + idempotent `--wizard-start-from` re-run + schema-driven defaults SOT + distinct `EXIT_WIZARD_CANCELLED = 5` exit code), and a **site documentation correction sweep** that brings every visible YAML / artefact-path / CLI / schema claim on `site/*.html` in line with the live code surface and restores six-language i18n parity. v0.5.6 reverted the v0.5.5 `torch>=2.3` floor back to `torch>=2.2` to restore Intel Mac (x86_64) installability. v0.5.7 fixes a runtime `TypeError` in the SFT trainer caused by trl 0.13 renaming `SFTConfig.max_seq_length` → `SFTConfig.max_length`. |
There was a problem hiding this comment.
Don’t label v0.5.7 as “latest on PyPI” before publish is complete.
Line 30 states v0.5.7 is already the latest PyPI release (dated May 10, 2026). The PR objective says tag/publish are still pending post-merge, so this should be phrased as pending (or kept at the currently published version) until release is actually live.
🧰 Tools
🪛 LanguageTool
[style] ~30-~30: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ore Intel Mac (x86_64) installability. v0.5.7 fixes a runtime TypeError in the ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/roadmap.md` at line 30, The docs line that reads "Latest release on
PyPI: `v0.5.7` — ... (2026-05-10)" incorrectly claims the release is live;
update the sentence referencing the literal token `v0.5.7` (and the date) to
indicate the release is pending (e.g. "v0.5.7 (pending publish)" or revert to
the currently published version like "Latest release on PyPI: `v0.5.6`") and
remove or change the release date so the roadmap does not state a published date
until the tag/publish completes.
| > **Status:** ✅ **Tier 1 DONE** — landed on `development` for the (originally) `v0.5.2` cycle. All five must-have tasks shipped: MinHash LSH dedup option (`[ingestion-scale]` extra via `datasketch`), markdown-aware splitter (`--strategy markdown`), code/secrets leakage tagger (`[ingestion-secrets]` extra via `detect-secrets` with regex fallback), heuristic quality filter (`--quality-filter`), DOCX/Markdown table preservation. Tier 2 (Presidio adapter, Croissant metadata) and Tier 3 (`--all-mask` composite, wizard "audit first" hook) are deferred to a follow-up **Phase 12.5** backlog file (analogous to [Phase 11.5 section above](#phase-115-ingestion-audit-polish)). Modules: [`forgelm/data_audit.py`](../../forgelm/data_audit/), [`forgelm/ingestion.py`](../../forgelm/ingestion.py), [`forgelm/cli.py`](../../forgelm/cli/); tests: [`tests/test_data_audit_phase12.py`](../../tests/test_data_audit_phase12.py), [`tests/test_ingestion_phase12.py`](../../tests/test_ingestion_phase12.py); CLI tests added in [`tests/test_cli_subcommands.py`](../../tests/test_cli_subcommands.py). | ||
|
|
There was a problem hiding this comment.
Phase 12 status summary still advertises a removed extra.
Line 926 states Tier 1 shipped with [ingestion-secrets] + detect-secrets fallback, but the current package surface no longer exposes that extra. This should be reworded as historical context (or corrected to regex-only) to avoid misleading install guidance.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 927-927: Blank line inside blockquote
(MD028, no-blanks-blockquote)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/roadmap/completed-phases.md` around lines 926 - 927, The Phase 12 Tier 1
summary still claims an extra "[ingestion-secrets]" and use of "detect-secrets"
which has been removed from the package surface; update the sentence in the
Phase 12 status paragraph so it no longer advertises that install option —
either rephrase it as historical context (e.g., "previously used detect-secrets
as an extra") or change it to state that the secret detection now uses the regex
fallback only, and update references to modules like forgelm/ingestion.py and
forgelm/data_audit.py accordingly so readers aren't misled about an available
"[ingestion-secrets]" extra.
| # Preflight: detect Intel-Mac-style torch/NumPy ABI mismatch BEFORE | ||
| # the heavy imports below would surface it as a cryptic NameError | ||
| # mid-pipeline. No-op on healthy platforms. | ||
| _preflight_numpy_torch_abi(json_output) | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's read the file at the lines in question and surrounding context
cat -n forgelm/cli/_training.py | sed -n '50,100p'Repository: cemililik/ForgeLM
Length of output: 2500
🏁 Script executed:
# Also check the error-handling.md standard
cat -n docs/standards/error-handling.md | head -100Repository: cemililik/ForgeLM
Length of output: 4841
🏁 Script executed:
# Find _report_training_error and _preflight_numpy_torch_abi implementations
rg -n "_report_training_error|_preflight_numpy_torch_abi" forgelm/cli/_training.pyRepository: cemililik/ForgeLM
Length of output: 303
🏁 Script executed:
# Get the full context of _run_training_pipeline function
ast-grep --pattern $'func _run_training_pipeline($_) {
$$$
}'Repository: cemililik/ForgeLM
Length of output: 43
🏁 Script executed:
cat -n forgelm/cli/_training.py | sed -n '1,30p'Repository: cemililik/ForgeLM
Length of output: 1200
🏁 Script executed:
cat -n forgelm/cli/_abi_check.pyRepository: cemililik/ForgeLM
Length of output: 5210
🏁 Script executed:
# Check if there are other similar preflight patterns in the codebase
rg -n "preflight\|_preflight" forgelm/ -A 5 -B 2Repository: cemililik/ForgeLM
Length of output: 43
🏁 Script executed:
# Verify what exceptions could escape from _preflight_numpy_torch_abi
# by checking the full chain of what it calls
rg -n "def compute_numpy_torch_abi_status|def format_abi_remediation|def _major_minor" forgelm/cli/_abi_check.py -A 10Repository: cemililik/ForgeLM
Length of output: 1747
🏁 Script executed:
# Check error handling patterns in similar CLI functions
rg -n "_report_training_error" forgelm/cli/_training.py -B 3 -A 1Repository: cemililik/ForgeLM
Length of output: 1253
🏁 Script executed:
cat -n forgelm/cli/_training.py | sed -n '70,125p'Repository: cemililik/ForgeLM
Length of output: 3295
Route ABI preflight failures through the structured error path.
Line 75 executes the ABI preflight before the function's guarded error handling (lines 79–90 and 92–116). While compute_numpy_torch_abi_status() handles expected ImportError cases gracefully, any unexpected runtime failure (e.g., corrupt torch binary, missing version attribute) will escape unhandled, breaking the JSON error contract and producing an unstructured traceback instead of a CLI error envelope with the documented exit-code contract (0/1/2/3/4).
Suggested fix
- _preflight_numpy_torch_abi(json_output)
+ try:
+ _preflight_numpy_torch_abi(json_output)
+ except Exception as exc: # noqa: BLE001
+ _report_training_error(
+ json_output,
+ payload={
+ "success": False,
+ "error": "numpy_torch_abi_preflight_failed",
+ "detail": f"{exc.__class__.__name__}: {exc}",
+ },
+ log_msg="NumPy/torch ABI preflight failed unexpectedly.",
+ exit_code=EXIT_TRAINING_ERROR,
+ with_traceback=True,
+ )Per docs/standards/error-handling.md: error paths must exit with named constants and route through the CLI error handler to preserve the JSON envelope and exit-code contract.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Preflight: detect Intel-Mac-style torch/NumPy ABI mismatch BEFORE | |
| # the heavy imports below would surface it as a cryptic NameError | |
| # mid-pipeline. No-op on healthy platforms. | |
| _preflight_numpy_torch_abi(json_output) | |
| # Preflight: detect Intel-Mac-style torch/NumPy ABI mismatch BEFORE | |
| # the heavy imports below would surface it as a cryptic NameError | |
| # mid-pipeline. No-op on healthy platforms. | |
| try: | |
| _preflight_numpy_torch_abi(json_output) | |
| except Exception as exc: # noqa: BLE001 | |
| _report_training_error( | |
| json_output, | |
| payload={ | |
| "success": False, | |
| "error": "numpy_torch_abi_preflight_failed", | |
| "detail": f"{exc.__class__.__name__}: {exc}", | |
| }, | |
| log_msg="NumPy/torch ABI preflight failed unexpectedly.", | |
| exit_code=EXIT_TRAINING_ERROR, | |
| with_traceback=True, | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@forgelm/cli/_training.py` around lines 72 - 76, The ABI preflight call
_preflight_numpy_torch_abi runs outside the module's guarded CLI error path so
unexpected failures bypass the JSON error envelope; wrap the call (or move it)
into the same guarded error handling used for the rest of the CLI so any
exception from compute_numpy_torch_abi_status or _preflight_numpy_torch_abi is
caught (catch Exception, not just ImportError) and forwarded to the CLI error
handler/exit path (use the existing CLI error handler and named exit-code
constants) so the failure emits the structured JSON envelope and correct exit
code.
| assert status == ABI_OK, f"unexpected ABI status {status!r}" | ||
| return _CheckResult( | ||
| name=_PROBE_NUMPY_TORCH_ABI, | ||
| status=_STATUS_PASS, | ||
| detail=f"torch {torch_version} + numpy {numpy_version} are ABI-compatible.", | ||
| extras=extras, | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Read the actual file at the specified lines
head -n 280 forgelm/cli/subcommands/_doctor.py | tail -n 20Repository: cemililik/ForgeLM
Length of output: 765
🏁 Script executed:
# Read the error-handling standard referenced
cat docs/standards/error-handling.mdRepository: cemililik/ForgeLM
Length of output: 12731
🏁 Script executed:
# Search for assert usage in the CLI module to understand patterns
rg "^\s*assert\s+" forgelm/cli/ -A 1 | head -50Repository: cemililik/ForgeLM
Length of output: 221
🏁 Script executed:
# Check if -O optimization is documented in the project
rg "\-O|optimize|PYTHONOPTIMIZE" . -t md -t txt --max-count=10Repository: cemililik/ForgeLM
Length of output: 17373
🏁 Script executed:
# Check for any Python optimization flags in CI/test configuration
rg "PYTHONOPTIMIZE|python\s+-O|optimize.*python" .github/ tests/ --type yaml --type py --type txtRepository: cemililik/ForgeLM
Length of output: 43
🏁 Script executed:
# Check if the subcommand module returns _CheckResult which is then handled by caller
rg "_CheckResult|status.*PASS|status.*FAIL" forgelm/cli/subcommands/_doctor.py -B 2 -A 2 | head -60Repository: cemililik/ForgeLM
Length of output: 1861
🏁 Script executed:
# Verify the pattern used in other probe functions to confirm consistency requirement
sed -n '140,220p' forgelm/cli/subcommands/_doctor.py | head -80Repository: cemililik/ForgeLM
Length of output: 3360
🏁 Script executed:
# Check how the probe functions are called/aggregated to understand error flow
rg "_build_check_plan|_check_" forgelm/cli/subcommands/_doctor.py -A 3 -B 1 | head -40Repository: cemililik/ForgeLM
Length of output: 1324
🏁 Script executed:
# Check how probe results are aggregated and reported
rg "def _build_check_plan|def doctor" forgelm/cli/subcommands/_doctor.py -A 30 | head -60Repository: cemililik/ForgeLM
Length of output: 1464
🏁 Script executed:
# Check the main entry point to see error handling for the doctor command
rg "def .*doctor.*\(" forgelm/cli/subcommands/_doctor.py -A 20 | head -50Repository: cemililik/ForgeLM
Length of output: 1176
Use explicit error handling to match other probes in this file.
All other probe functions (_check_python_version, _check_torch_cuda, etc.) use explicit if/return patterns to return _CheckResult with appropriate status codes. Line 273 breaks this pattern by using assert status == ABI_OK, which diverges from the consistent error-handling approach used throughout this module. Replace the assert with an explicit condition to return _STATUS_FAIL when status is unexpected, keeping behavior uniform with other probes.
Suggested fix
- assert status == ABI_OK, f"unexpected ABI status {status!r}"
- return _CheckResult(
- name=_PROBE_NUMPY_TORCH_ABI,
- status=_STATUS_PASS,
- detail=f"torch {torch_version} + numpy {numpy_version} are ABI-compatible.",
- extras=extras,
- )
+ if status != ABI_OK:
+ return _CheckResult(
+ name=_PROBE_NUMPY_TORCH_ABI,
+ status=_STATUS_FAIL,
+ detail=f"Unexpected ABI probe status {status!r}. Please re-run with --output-format json and report this.",
+ extras={**extras, "unexpected_status": status},
+ )
+ return _CheckResult(
+ name=_PROBE_NUMPY_TORCH_ABI,
+ status=_STATUS_PASS,
+ detail=f"torch {torch_version} + numpy {numpy_version} are ABI-compatible.",
+ extras=extras,
+ )🧰 Tools
🪛 GitHub Check: Codacy Static Code Analysis
[warning] 273-273: forgelm/cli/subcommands/_doctor.py#L273
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@forgelm/cli/subcommands/_doctor.py` around lines 273 - 279, Replace the
assert with explicit error handling: check if status != ABI_OK and return a
failing _CheckResult with name=_PROBE_NUMPY_TORCH_ABI, status=_STATUS_FAIL, an
informative detail message including the unexpected status and the torch/numpy
versions, and include extras; otherwise return the existing passing
_CheckResult. Use the same pattern as other probes (_check_python_version,
_check_torch_cuda) and reference _CheckResult, _STATUS_FAIL, _STATUS_PASS, and
ABI_OK to implement the conditional branch.
Round-4 cross-review of f205f42 turned up one MAJOR (marker spelling drift in a test docstring) and a handful of MINOR/NIT items. No public surface change; all changes are honesty / robustness / CI guard additions plus the documentation gap the previous round left open. MAJOR - `tests/test_abi_check.py` docstring now quotes the PEP 508 marker text actually shipped in `pyproject.toml` (`sys_platform == 'darwin'`) rather than the equivalent-but- different `platform_system == "Darwin"` form. A future reviewer chasing the marker chain via the test docstring would otherwise conclude the marker had drifted. MINOR / NIT - `forgelm/trainer.py` SFT else-raise reads the trl version off `SFTConfig.__module__` rather than a duplicate `import trl as _trl`; `sys` promoted to module-top. - `forgelm/cli/_abi_check.py::compute_numpy_torch_abi_status` short- circuits to ABI_OK when either `_major_minor` returns (0, 0). Corporate forks with non-semver tags (e.g. `foo-bar` torch) would otherwise hit `< (2, 3)` against any healthy numpy ≥ 2 and trigger a false-positive ABI_BROKEN. Two new tests cover both sides. - `forgelm/cli/_abi_check.py::format_abi_remediation` raises ValueError on a None version (precondition guard against future call-site drift that would emit "torch None ..." strings). - `forgelm/cli/subcommands/_doctor.py` swaps the bare `assert status == ABI_OK` for a `RuntimeError` (asserts strip under `python -O`). The six-symbol `_abi_check` import promoted from function-local to module-top. - New CI guard `tools/check_no_unguarded_sys_modules_pop.py` fails on any `sys.modules.pop("torch"|"numpy"|"trl"|"transformers"| "peft"|"datasets")` or `del sys.modules[...]` without `monkeypatch.delitem` — the round-3 fix pattern. Wired into both the self-review gauntlet (`CLAUDE.md`, now 10 checks) and `.github/workflows/ci.yml`. - `pytest-randomly>=3.15.0,<4.0.0` added to the `[dev]` extra so order-dependent regressions surface in CI rather than waiting for a Python micro-bump to shuffle collection. Suite re-verified under shuffled order (1651 passed, 14 skipped, 0 failed). - Training preflight's 5-key JSON envelope (`success`, `error`, `torch_version`, `numpy_version`, `remediation`) now documented in `docs/usermanuals/{en,tr}/reference/json-output.md` so CI consumers can branch on the stable `"error": "numpy_torch_abi_mismatch"` token. - One leftover `(Faz 12)` in the v0.5.2 CHANGELOG entry rewritten to `(Phase 12)` (English-only-in-CHANGELOG cleanup, pre-existing drift). Verified: 1651 passed, 14 skipped, 0 failed under pytest-randomly shuffled order; ruff format + ruff check + all ten doc guards (anchor-resolution, no-analysis-refs, bilingual-parity, CLI help, wizard-defaults, no-unguarded-sys-modules-pop, …) green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
CodeRabbit posted 11 inline comments across PR #44 (two batches, one per round-3/round-4 cycle). Seven were already resolved by earlier rounds; this round closes the three residuals plus one stale-doc finding CodeRabbit raised but the project's own `error-handling.md` already documented correctly. - `forgelm/cli/_training.py::_preflight_numpy_torch_abi` now wraps the call to `compute_numpy_torch_abi_status` in a `try/except` so any unexpected exception (e.g. corrupted torch install where `torch.__version__` raises `AttributeError`) converts into a structured `abi_preflight_crashed` JSON envelope rather than emitting a raw Python traceback that pre-empts the `--output-format json` contract. Two new regression tests in `tests/test_abi_check.py::TestPreflightHandlesProbeCrash` pin the exit code (`EXIT_TRAINING_ERROR` = 2, same class as the broken-ABI verdict so CI/CD doesn't need to distinguish "ABI bad" from "ABI probe died") and the envelope shape. - `CLAUDE.md` exit-code line corrected from `0/1/2/3/4` to `0/1/2/3/4/5` to match the canonical table in `docs/standards/error-handling.md` (Phase 22 / v0.5.5 added `EXIT_WIZARD_CANCELLED = 5`; CLAUDE.md was stale, not the code). - `docs/roadmap/completed-phases.md` Phase 12 Tier 1 status line no longer advertises a `[ingestion-secrets]` extra that was never published — only `[ingestion-pii-ml]` exists in `pyproject.toml`. Reworded as historical context ("originally planned as `[ingestion-secrets]` but deferred"). Verified: 1653 passed, 14 skipped, 0 failed under pytest-randomly shuffled order; ruff format + ruff check + all ten gauntlet doc guards green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CodeRabbit posted 11 inline comments across PR #44 (two batches, one per round-3/round-4 cycle). Seven were already resolved by earlier rounds; this round closes the three residuals plus one stale-doc finding CodeRabbit raised but the project's own `error-handling.md` already documented correctly. - `forgelm/cli/_training.py::_preflight_numpy_torch_abi` now wraps the call to `compute_numpy_torch_abi_status` in a `try/except` so any unexpected exception (e.g. corrupted torch install where `torch.__version__` raises `AttributeError`) converts into a structured `abi_preflight_crashed` JSON envelope rather than emitting a raw Python traceback that pre-empts the `--output-format json` contract. Two new regression tests in `tests/test_abi_check.py::TestPreflightHandlesProbeCrash` pin the exit code (`EXIT_TRAINING_ERROR` = 2, same class as the broken-ABI verdict so CI/CD doesn't need to distinguish "ABI bad" from "ABI probe died") and the envelope shape. - `CLAUDE.md` exit-code line corrected from `0/1/2/3/4` to `0/1/2/3/4/5` to match the canonical table in `docs/standards/error-handling.md` (Phase 22 / v0.5.5 added `EXIT_WIZARD_CANCELLED = 5`; CLAUDE.md was stale, not the code). - `docs/roadmap/completed-phases.md` Phase 12 Tier 1 status line no longer advertises a `[ingestion-secrets]` extra that was never published — only `[ingestion-pii-ml]` exists in `pyproject.toml`. Reworded as historical context ("originally planned as `[ingestion-secrets]` but deferred"). Verified: 1653 passed, 14 skipped, 0 failed under pytest-randomly shuffled order; ruff format + ruff check + all ten gauntlet doc guards green. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Consolidate the [Unreleased] block (rounds 3+4+5 of PR #44 review absorption) into the v0.5.7 release block. v0.5.7 was previously landed but never tagged or published; consolidating into a single coherent release section keeps the CHANGELOG narrative honest about what ships under this tag. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>



Two-part PR:
Critical fix (v0.5.7 patch release) — SFT trainer crashed with
TypeError: SFTConfig.__init__() got an unexpected keyword argument 'max_seq_length'on every environment with modern trl (0.13+ / 1.x), notably the Colab defaultpip install forgelmpath.Roadmap directory consolidation (doc-only) — 7 completed phase files absorbed into
completed-phases.md.Commits
4225b5b—docs(roadmap): consolidate 7 completed phase files into completed-phases.mde080da6—fix(roadmap): PR #44 review absorption — placeholder + Turkish drift cleanup5fbbbd4—fix(release): v0.5.7 — SFT trainer trl-modernisation TypeErrorPart 1 — v0.5.7 SFT fix (critical)
trl 0.13 renamed the sequence-length cap from
max_seq_lengthtomax_lengthonSFTConfigand removed the old name. v0.5.6 still passed the old kwarg unconditionally, so any modern trl install (Colab default, fresh installs after 2026-04) crashed at trainer-args build time.Fix:
forgelm/trainer.py::_get_training_args_for_type's SFT branch now inspectsSFTConfig.__init__'s signature at runtime and picks the correct parameter name. trl 0.13+ receivesmax_length; trl 0.12.x — still withinpyproject.toml'strl>=0.12.0,<2.0.0floor — keeps receivingmax_seq_length. No code change for DPO / SimPO / KTO / ORPO / GRPO trainers (their*Configparameters were not affected by the rename).Tests: 3 new regression tests in
tests/test_trainer_sft_config.pypin the modern-trl path, the legacy-trl path, and thatpacking/dataset_text_fieldcontinue to be propagated.Release-prep:
pyproject.toml:0.5.6→0.5.7forgelm/_version.py: rationale comment updated;__api_version__stays1.0.0CHANGELOG.md:[0.5.7]entry inserted above[0.5.6]docs/roadmap/releases.md:v0.5.7entry inserted abovev0.5.6notebooks/*.ipynb: install pins bumped 0.5.6 → 0.5.7 (10 files)Part 2 — roadmap consolidation (doc-only refactor)
Tidy-up of the
docs/roadmap/directory: every completed phase file (Phase 10 / 10.5 / 11 / 11.5 / 12 / 12.5 / 12.6) is now absorbed into the canonicalcompleted-phases.mdarchive with full content (heading shift +1 level), and the standalone files are removed. The roadmap directory now carries only the planning surface.Before: 12 files (7 completed-phase docs + 2 planned-phase docs + 3 cross-cutting)
After: 5 files (1 archive + 2 planned + 2 cross-cutting)
Round-1 review absorption (commit
e080da6) addressed all gemini-code-assist + sourcery-ai findings on the consolidation: placeholder cleanup (3 medium), Turkish-term drift cleanup ("fazlar" / "Faz N" → "tasks" / "Task N" — the 38 numbered items in Phase 12.6's closure cycle were originally tracked in Turkish; the canonical English archive uses "tasks").Test plan
ruff format --check . && ruff check .cleanforgelm --config config_template.yaml --dry-rungreentools/check_anchor_resolution.py --strict(266 markdown files OK)tools/check_bilingual_parity.py --strict(45 pairs OK)tools/check_cli_help_consistency.py --strict(439 forgelm invocations OK)tools/check_wizard_defaults_sync.py(3 sections / 8 fields OK)tools/check_no_analysis_refs.py(no working-memory leakage)v0.5.7from main, push tag, publish.yml runs 12 cross-OS combos before PyPI publishpip install -U forgelmreaches v0.5.7; SFT training no longer crashes on modern trl🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
New Features
Chores
Tests
Documentation