Skip to content

release: v0.5.7 — SFT trainer trl-modernisation fix + roadmap consolidation - #44

Merged
cemililik merged 7 commits into
mainfrom
development
May 11, 2026
Merged

cemililik merged 7 commits into
mainfrom
development

Conversation

@cemililik

@cemililik cemililik commented May 10, 2026 •

Copy link
Copy Markdown
Collaborator

Two-part PR:

  1. 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 default pip install forgelm path.

  2. 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.md
  • e080da6 — fix(roadmap): PR #44 review absorption — placeholder + Turkish drift cleanup
  • 5fbbbd4 — fix(release): v0.5.7 — SFT trainer trl-modernisation TypeError

Part 1 — v0.5.7 SFT fix (critical)

trl 0.13 renamed the sequence-length cap from max_seq_length to max_length on SFTConfig and 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 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.toml's trl>=0.12.0,<2.0.0 floor — keeps receiving max_seq_length. No code change for DPO / SimPO / KTO / ORPO / GRPO trainers (their *Config parameters were not affected by the rename).

Tests: 3 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.

Release-prep:

  • pyproject.toml: 0.5.6 → 0.5.7
  • forgelm/_version.py: rationale comment updated; __api_version__ stays 1.0.0
  • 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)

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 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.

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 . clean
  • forgelm --config config_template.yaml --dry-run green
  • tools/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)
  • After merge: tag v0.5.7 from main, push tag, publish.yml runs 12 cross-OS combos before PyPI publish
  • After publish: pip install -U forgelm reaches v0.5.7; SFT training no longer crashes on modern trl

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes

    • Fixed SFT trainer runtime compatibility with modern TRL (avoids TypeError during training).
  • New Features

    • Added a PyTorch↔NumPy ABI diagnostic probe and a training preflight that aborts with a structured JSON remediation on known ABI mismatches.
  • Chores

    • Package bumped to v0.5.7; added Intel‑Mac NumPy pin and a CI guard script to prevent unguarded sys.modules evictions; added test-order tooling.
  • Tests

    • Broadened regression and probe tests covering trainer compatibility, ABI checks, and preflight behavior.
  • Documentation

    • Consolidated roadmap content into a single completed-phases archive and updated release notes.

…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>
@qodo-code-review

Copy link
Copy Markdown
ⓘ 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.

@sourcery-ai

sourcery-ai Bot commented May 10, 2026 •

Copy link
Copy Markdown

Reviewer's Guide

Consolidates 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 consolidation

flowchart 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
Loading

File-Level Changes

Change Details Files
Inline all completed phase documents into the canonical completed-phases.md archive.
  • Replace the short Phase 10 stub with the full phase-10-post-training content, adjusting heading levels and adding detailed goals, tasks, requirements, and delivery sections.
  • Append new sections for Phases 10.5, 11, 11.5, 12, 12.5, and 12.6 in chronological order as top-level headings, preserving their original structure and explanatory context.
  • Update any intra-phase cross-references and anchors in the imported content so they resolve as in-document headings within completed-phases.md rather than separate files.
docs/roadmap/completed-phases.md
Remove obsolete standalone completed-phase roadmap files now that they are archived inline.
  • Delete the seven per-phase markdown documents for Phases 10, 10.5, 11, 11.5, 12, 12.5, and 12.6 from docs/roadmap/.
  • Rely on completed-phases.md as the single source of truth for completed phase details going forward.
docs/roadmap/phase-10-post-training.md
docs/roadmap/phase-10-5-quickstart.md
docs/roadmap/phase-11-data-ingestion.md
docs/roadmap/phase-11-5-backlog.md
docs/roadmap/phase-12-data-curation-maturity.md
docs/roadmap/phase-12-5-backlog.md
docs/roadmap/phase-12-6-closure-cycle.md
Update roadmap index and release docs to reference the consolidated archive instead of per-phase files.
  • Change roadmap.md and roadmap-tr.md tables and directory overviews so completed phases link to roadmap/completed-phases.md rather than to individual phase-X files, and simplify the roadmap/ directory listing accordingly.
  • Adjust references in docs/roadmap/releases.md to describe Phase 10 and Phase 10.5 as pointing at completed-phases.md.
  • Clarify comments about Phase 12.6 and the closure cycle to mention completed-phases.md as the tracking location.
docs/roadmap.md
docs/roadmap-tr.md
docs/roadmap/releases.md
Retarget internal documentation and code comments that pointed at specific phase files to the unified completed-phases.md.
  • Update CHANGELOG.md links that previously pointed at phase-12-5-backlog.md or phase-10-5-quickstart.md to instead reference completed-phases.md.
  • Update standards docs (documentation and release standards) to mention completed-phases.md as the canonical place to track phase work and closure-cycle inventory.
  • Update a skill doc under .claude to use completed-phases.md as the example phase file reference.
CHANGELOG.md
docs/standards/documentation.md
docs/standards/release.md
.claude/skills/add-trainer-feature/SKILL.md
Adjust code and tooling references to phase docs to preserve comments and benchmark guidance after consolidation.
  • Update the comment in forgelm/ingestion.py that explains deferred semantic chunking to reference completed-phases.md instead of the old Phase 12 file.
  • Update tests/test_phase12_5.py docstring to describe the Phase 12.5 backlog as living in completed-phases.md.
  • Update tools/bench_simhash.py instructions so benchmark results are copied back into completed-phases.md rather than the deleted Phase 11.5 backlog file.
forgelm/ingestion.py
tests/test_phase12_5.py
tools/bench_simhash.py

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@coderabbitai

coderabbitai Bot commented May 10, 2026 •

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: c5a0b2fb-9ba0-4760-9777-d0e0aac1986a

📥 Commits

Reviewing files that changed from the base of the PR and between f205f42 and baf5b55.

📒 Files selected for processing (11)
  • .github/workflows/ci.yml
  • CHANGELOG.md
  • CLAUDE.md
  • docs/usermanuals/en/reference/json-output.md
  • docs/usermanuals/tr/reference/json-output.md
  • forgelm/cli/_abi_check.py
  • forgelm/cli/subcommands/_doctor.py
  • forgelm/trainer.py
  • pyproject.toml
  • tests/test_abi_check.py
  • tools/check_no_unguarded_sys_modules_pop.py
✅ Files skipped from review due to trivial changes (2)
  • tests/test_abi_check.py
  • docs/usermanuals/tr/reference/json-output.md
🚧 Files skipped from review as they are similar to previous changes (3)
  • pyproject.toml
  • forgelm/cli/subcommands/_doctor.py
  • forgelm/trainer.py

📝 Walkthrough

Walkthrough

Consolidates 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.

Changes

v0.5.7 Release & Documentation Consolidation

Layer / File(s) Summary
ABI helper & parser
forgelm/cli/_abi_check.py
New shared ABI checker with status constants, _major_minor, compute_numpy_torch_abi_status(), and format_abi_remediation() detecting torch<2.3 + numpy>=2 incompatibility.
Training preflight
forgelm/cli/_training.py
Adds _preflight_numpy_torch_abi(json_output) and calls it at _run_training_pipeline start; aborts with structured JSON and exit code on broken ABI.
Doctor probe
forgelm/cli/subcommands/_doctor.py
Adds _PROBE_NUMPY_TORCH_ABI, _check_numpy_torch_abi() delegating to shared checker, inserts probe into plan, and re-exports for tests.
ABI & Doctor Tests
tests/test_abi_check.py, tests/test_doctor.py
New tests for version parsing, ABI verdicts, remediation formatting, training preflight JSON/log behavior, and doctor probe pass/fail/skip cases.
SFT Trainer Runtime Detection & Tests
forgelm/trainer.py, tests/test_trainer_sft_config.py
ForgeTrainer inspects trl.SFTConfig constructor to choose max_length vs max_seq_length from self.config.model.max_length; raises ValueError if neither is detected. New regression tests cover modern/legacy/transitional/error cases and passthrough of packing/dataset_text_field.
Version bump & packaging
pyproject.toml, forgelm/_version.py, CHANGELOG.md
Project version → 0.5.7; added Intel Mac numpy<2 environment marker; changelog updated with Unreleased and v0.5.7 entries; internal version comment updated.
Consolidated roadmap archive
docs/roadmap/completed-phases.md
Adds full Phase 10–12.6 archive sections (task lists, delivery notes, 12.6 closure-cycle inventory), replacing per-phase files.
Remove individual phase files
docs/roadmap/phase-10-*.md, phase-11-*.md, phase-12-*.md
Deleted per-phase docs now subsumed by completed-phases.md.
Docs cross-references & standards
docs/roadmap.md, docs/roadmap-tr.md, docs/roadmap/releases.md, docs/standards/*, docs/usermanuals/*, docs/roadmap/phase-14-pipeline-chains.md, docs/roadmap/risks-and-decisions.md
Updated links and narrative to point to completed-phases.md; added v0.5.7 release notes and translation updates.
In-code doc refs & tooling
forgelm/ingestion.py, tools/bench_simhash.py, .claude/skills/*/SKILL.md
Updated comments, NotImplementedError messages, and script instructions to reference consolidated roadmap.
Notebooks
notebooks/*.ipynb
Updated install pins from 0.5.6 → 0.5.7 across multiple notebooks and extras.
Tools & CI guard
tools/check_no_unguarded_sys_modules_pop.py, .github/workflows/ci.yml
New guard script to detect unguarded sys.modules evictions and CI validate step to run it.
Tests: small refactors
tests/test_quickstart_compat.py, tests/test_phase12_5.py
Minor test updates: use monkeypatch for torch-missing vram probe; docstring link updates to consolidated roadmap.

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
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Poem

🐰 Phase pages tucked in one neat tome,

Trainer peeks to learn which param name,
ABI helper hums a cautious tone,
Tests stand watch so fixes make their home,
Notebooks pin—v0.5.7 hops on.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 39.66% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The PR title clearly summarizes both main changes: the v0.5.7 SFT trainer fix for trl modernization and the roadmap consolidation. It is specific, concise, and highlights the primary deliverables.
Description check ✅ Passed The PR description comprehensively covers both parts, includes commit references, test plan checkmarks, and clear before/after structure for the consolidation. All template sections are addressed with meaningful content.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch development

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codacy-production

codacy-production Bot commented May 10, 2026 •

Copy link
Copy Markdown

Not up to standards ⛔

🔴 Issues 1 medium · 3 minor

Alerts:
⚠ 4 issues (≤ 0 issues of at least minor severity)

Results:
4 new issues

Category Results
BestPractice 1 medium
2 minor
CodeStyle 1 minor

View in Codacy

🟢 Metrics 75 complexity · 2 duplication

Metric Results
Complexity 75
Duplication 2

View in Codacy

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.

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment thread docs/roadmap/completed-phases.md Outdated

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread docs/roadmap/completed-phases.md Outdated
> **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).

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
> **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).

Comment thread docs/roadmap/completed-phases.md Outdated
> 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).

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
> **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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The link text here is awkward and contains a parenthetical placeholder. It should be cleaned up to use natural phrasing.

Suggested change
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.

Comment thread docs/roadmap/completed-phases.md Outdated

---

## Phase 12.6 — Closure Cycle (38 fazlar across 5 waves)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
## Phase 12.6 — Closure Cycle (38 fazlar across 5 waves)
## Phase 12.6 — Closure Cycle (38 phases across 5 waves)

Comment thread docs/roadmap.md Outdated
| ✅ 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) |

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The term "fazlar" is Turkish and should be translated to "phases" to maintain language consistency in this English roadmap file.

Suggested change
| ✅ 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) |

cemililik and others added 2 commits May 10, 2026 23:16
…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>
@cemililik cemililik changed the title docs(roadmap): consolidate 7 completed phase files into completed-phases.md release: v0.5.7 — SFT trainer trl-modernisation fix + roadmap consolidation May 10, 2026
@cemililik

Copy link
Copy Markdown
Collaborator Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented May 10, 2026

Copy link
Copy Markdown
Contributor
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🧹 Nitpick comments (1)
docs/roadmap/completed-phases.md (1)

653-764: ⚡ Quick win

Normalize subsection heading levels and labels to avoid lint churn.

Several sections use #### Tasks/Requirements/Delivery directly 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

📥 Commits

Reviewing files that changed from the base of the PR and between f492579 and 5fbbbd4.

📒 Files selected for processing (33)
  • .claude/skills/add-trainer-feature/SKILL.md
  • CHANGELOG.md
  • docs/roadmap-tr.md
  • docs/roadmap.md
  • docs/roadmap/completed-phases.md
  • docs/roadmap/phase-10-5-quickstart.md
  • docs/roadmap/phase-10-post-training.md
  • docs/roadmap/phase-11-5-backlog.md
  • docs/roadmap/phase-11-data-ingestion.md
  • docs/roadmap/phase-12-5-backlog.md
  • docs/roadmap/phase-12-6-closure-cycle.md
  • docs/roadmap/phase-12-data-curation-maturity.md
  • docs/roadmap/phase-14-pipeline-chains.md
  • docs/roadmap/releases.md
  • docs/standards/documentation.md
  • docs/standards/release.md
  • forgelm/_version.py
  • forgelm/ingestion.py
  • forgelm/trainer.py
  • notebooks/data_curation.ipynb
  • notebooks/dpo_alignment.ipynb
  • notebooks/galore_memory_optimization.ipynb
  • notebooks/grpo_reasoning.ipynb
  • notebooks/kto_binary_feedback.ipynb
  • notebooks/multi_dataset.ipynb
  • notebooks/post_training_workflow.ipynb
  • notebooks/quickstart_sft.ipynb
  • notebooks/safety_evaluation.ipynb
  • notebooks/synthetic_data_training.ipynb
  • pyproject.toml
  • tests/test_phase12_5.py
  • tests/test_trainer_sft_config.py
  • tools/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

Comment thread docs/roadmap-tr.md Outdated
Comment thread docs/roadmap.md
Comment thread docs/roadmap/completed-phases.md Outdated
Comment thread docs/roadmap/phase-14-pipeline-chains.md Outdated
Comment thread docs/roadmap/releases.md
cemililik and others added 3 commits May 11, 2026 01:26
…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>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 5fbbbd4 and f205f42.

📒 Files selected for processing (18)
  • CHANGELOG.md
  • docs/roadmap-tr.md
  • docs/roadmap.md
  • docs/roadmap/completed-phases.md
  • docs/roadmap/phase-14-pipeline-chains.md
  • docs/roadmap/risks-and-decisions.md
  • docs/usermanuals/en/reference/json-output.md
  • docs/usermanuals/tr/reference/json-output.md
  • forgelm/_version.py
  • forgelm/cli/_abi_check.py
  • forgelm/cli/_training.py
  • forgelm/cli/subcommands/_doctor.py
  • forgelm/trainer.py
  • pyproject.toml
  • tests/test_abi_check.py
  • tests/test_doctor.py
  • tests/test_quickstart_compat.py
  • tests/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

Comment thread CHANGELOG.md
Comment thread docs/roadmap-tr.md
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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

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.

Comment thread docs/roadmap.md
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`.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

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.

Comment on lines +926 to +927
> **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).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

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.

Comment thread forgelm/cli/_training.py
Comment on lines +72 to +76
# 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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 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 -100

Repository: 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.py

Repository: 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.py

Repository: 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 2

Repository: 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 10

Repository: 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 1

Repository: 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.

Suggested change
# 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.

Comment thread forgelm/cli/subcommands/_doctor.py Outdated
Comment on lines +273 to +279
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,
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

# Read the actual file at the specified lines
head -n 280 forgelm/cli/subcommands/_doctor.py | tail -n 20

Repository: cemililik/ForgeLM

Length of output: 765


🏁 Script executed:

# Read the error-handling standard referenced
cat docs/standards/error-handling.md

Repository: 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 -50

Repository: 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=10

Repository: 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 txt

Repository: 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 -60

Repository: 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 -80

Repository: 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 -40

Repository: 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 -60

Repository: 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 -50

Repository: 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>
@sonarqubecloud

Copy link
Copy Markdown

@cemililik
cemililik merged commit 2be90a5 into main May 11, 2026
11 of 12 checks passed
cemililik added a commit that referenced this pull request May 11, 2026
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>
cemililik added a commit that referenced this pull request May 11, 2026
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>
cemililik added a commit that referenced this pull request May 11, 2026
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant