Repository navigation
feat(phase-14): multi-stage training pipeline chains (SFT → DPO → GRPO) - #53
Merged
Merged
Conversation
Implements every Phase 14 task documented in
``docs/roadmap/phase-14-pipeline-chains.md``. Single-stage runs reach
``forgelm/trainer.py`` byte-identically to v0.6.0 — the orchestrator
module is only imported when a ``pipeline:`` block is present.
Foundation (Wave 1)
- ``forgelm/config.py``: ``PipelineStage`` + ``PipelineConfig`` Pydantic
models enforce ``^[a-z0-9_]{1,32}$`` stage names, ≥ 1-stage
pipelines, unique-name validator, and explicit-``trainer_type``
audit-clarity rule per stage. ``extra="forbid"`` rejects per-stage
``distributed`` / ``webhook`` / ``compliance`` / ``risk_assessment``
/ ``monitoring`` / ``retention`` / ``synthetic`` / ``merge`` /
``auth`` with the offending key in the Pydantic error.
- ``merge_pipeline_stage_config()`` helper materialises a flat
``ForgeConfig`` per stage with section-wholesale inheritance + the
4-priority auto-chain resolution rule (``--input-model`` > explicit
stage.model > prev.output_dir/final_model > root.model.name_or_path).
- ``ForgeConfig.pipeline: Optional[PipelineConfig]`` + ``model_rebuild()``
to resolve the forward reference.
Orchestrator (Wave 2)
- ``forgelm/cli/_pipeline.py`` (new module): ``PipelineOrchestrator``
drives the chain. ``PipelineStageState`` + ``PipelineState``
dataclasses round-trip to ``pipeline_state.json`` via atomic write
(tmp + ``os.replace``). ``pipeline.started`` /
``pipeline.stage_started`` / ``pipeline.stage_completed`` /
``pipeline.stage_reverted`` / ``pipeline.completed`` audit events
fire alongside the existing per-stage ``training.*`` vocabulary so
pre-existing dashboards keep working.
- ``--stage <name>`` partial-run requires the previous stage's
on-disk output (auto-resolved) or ``--input-model <path>``
(escape hatch); silent fallback to root model is hard-failed.
- ``--resume-from <name>`` skips ``completed`` stages whose
output_model path exists. Stale-state guard refuses to resume
when ``pipeline_config_hash`` differs from the current YAML
bytes; ``--force-resume`` bypasses with a WARNING + audit record.
- Human-approval gate inside a pipeline exits
``EXIT_AWAITING_APPROVAL (4)`` and leaves downstream stages
``pending`` (not ``skipped_due_to_prior_revert``) so resume
picks them up post-approval.
Compliance (Wave 3)
- ``forgelm/compliance.py``: ``generate_pipeline_manifest()`` builds
the Annex IV chain index documented in Phase 14 Task 3 schema.
``export_pipeline_manifest()`` writes it atomically.
``verify_pipeline_manifest()`` walks a pipeline run directory and
returns a ``VerifyPipelineResult`` (chain integrity + per-stage
manifest existence). ``_verify_manifest_payload()`` is the
in-memory helper for unit tests.
- ``forgelm verify-annex-iv --pipeline <run_dir>`` CLI mode validates
a complete chain and exits non-zero on any violation.
CLI + webhook (Wave 4)
- ``forgelm/cli/_parser.py``: new ``--stage``, ``--resume-from``,
``--force-resume``, ``--input-model`` top-level flags.
- ``forgelm/cli/_dispatch.py``: routes to the orchestrator when
``config.pipeline`` is set; passes the raw YAML bytes for the
config-hash guard.
- ``forgelm/webhook.py``: ``notify_pipeline_started`` /
``notify_pipeline_completed`` / ``notify_pipeline_reverted``
methods; gated by the existing ``notify_on_start`` /
``notify_on_success`` / ``notify_on_failure`` config flags.
Tests + fixtures (Waves 2-5)
- ``tests/test_pipeline_config.py`` — 53 schema + inheritance-matrix
+ auto-chain priority tests.
- ``tests/test_pipeline_orchestrator.py`` — 27 state-machine tests
covering happy-path / auto-revert / human-approval / resume /
stale-config / partial-filter / webhook-failure-tolerance.
- ``tests/test_pipeline_compliance.py`` — 14 manifest schema +
chain-integrity verifier tests (including partial-filter and
auto-revert manifest acceptance).
- ``tests/test_pipeline_cli.py`` — 7 flag-interaction +
dispatch-routing tests.
- 6 fixture YAMLs under ``tests/fixtures/pipeline/`` exercising
every documented scenario.
Operator surface (Waves 6-7)
- ``docs/guides/pipeline.md`` + ``docs/guides/pipeline-tr.md`` —
bilingual operator guide (when to use, inheritance matrix, CLI
semantics, human-approval flow, Annex IV verifier, Limitations).
- ``config_template.yaml`` carries a commented-out ``pipeline:``
example block.
- ``CHANGELOG.md`` ``[Unreleased]`` section ``Added (Phase 14 — …)``
+ ``Changed (Phase 14)`` entries.
- ``docs/roadmap/phase-14-pipeline-chains.md`` Status header
updated to "Implementation complete on development; awaiting
v0.7.0 PyPI tag".
- ``tools/check_bilingual_parity.py`` ``_PAIRS`` registers the new
guide pair.
Wizard / notebooks / CLI help
- ``forgelm wizard`` is **not** extended with a pipeline path
(operator-grade workflow; documented in the guide's Limitations).
- ``notebooks/`` carries no new pipeline notebook (fixture suite
serves the same purpose without 3× boilerplate duplication).
- 451 ``forgelm`` invocations across docs / README pass the
CLI-help-consistency guard against the live parser surface.
Verification
- ruff format + check: clean across 174 files.
- 11 / 11 self-review gauntlet guards green (dry-run, bilingual
parity, anchor resolution, CLI help consistency 451
invocations, wizard defaults, no-analysis-refs, no-unguarded-
sys.modules-pop, site-version-literal sync).
- 1879 passed / 13 skipped / 0 failed across the full pytest
suite (28 min 35 sec; ``tests/test_phase12_review_fixes.py``
perf-flake remains documented out-of-scope).
- All 4 new Phase 14 test modules: 101 / 101 pass in 11 sec.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @cemililik, your pull request is larger than the review limit of 150000 diff characters
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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 |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 433 |
| Duplication | 22 |
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.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces multi-stage training pipeline chains, allowing sequential execution of stages like SFT, DPO, and GRPO via a new orchestrator. It includes updated configuration models, CLI support for stage resumption, and Annex IV manifest generation for compliance. Review feedback identified the need to prevent output directory collisions during dry runs, harden state file deserialization against corruption, and fix a path validation bug for auto-chained models.
Comment on lines
+539
to
+561
| def dry_run(self) -> int: | ||
| """Validate every stage without allocating any GPU resources. | ||
|
|
||
| Collects all per-stage Pydantic / merge errors before exiting, | ||
| mirroring ``pytest --collectonly``. Returns | ||
| :data:`EXIT_CONFIG_ERROR (1)` if any stage fails validation, | ||
| :data:`EXIT_SUCCESS (0)` otherwise. | ||
| """ | ||
| errors: List[str] = [] | ||
| prev_output: Optional[str] = None | ||
| for stage in self.pipeline.stages: | ||
| try: | ||
| merged = merge_pipeline_stage_config( | ||
| self.root_cfg, | ||
| stage, | ||
| prev_output_model=prev_output, | ||
| ) | ||
| except Exception as exc: # noqa: BLE001 — best-effort: collect every per-stage validation error for a single operator report instead of stopping at the first. | ||
| errors.append(f"Stage {stage.name!r}: merge failed: {exc}") | ||
| continue | ||
|
|
||
| # Project the output path the next stage would auto-chain to. | ||
| prev_output = os.path.join(merged.training.output_dir, "final_model") |
There was a problem hiding this comment.
The dry_run method should validate that each stage has a unique training.output_dir. Since stages inherit the root config's output_dir by default, a multi-stage pipeline where stages omit this field will cause per-stage manifests and checkpoints to overwrite each other, breaking the Annex IV chain of custody.
def dry_run(self) -> int:
"""Validate every stage without allocating any GPU resources.
Collects all per-stage Pydantic / merge errors before exiting,
mirroring ``pytest --collectonly``. Returns
:data:`EXIT_CONFIG_ERROR (1)` if any stage fails validation,
:data:`EXIT_SUCCESS (0)` otherwise.
"""
errors: List[str] = []
prev_output: Optional[str] = None
seen_dirs: set[str] = set()
for stage in self.pipeline.stages:
try:
merged = merge_pipeline_stage_config(
self.root_cfg,
stage,
prev_output_model=prev_output,
)
# Check for output_dir collisions to prevent manifest/checkpoint overwrites.
out_dir = os.path.abspath(merged.training.output_dir)
if out_dir in seen_dirs:
errors.append(f"Stage {stage.name!r}: output_dir collision: {merged.training.output_dir}")
seen_dirs.add(out_dir)
except Exception as exc: # noqa: BLE001 — best-effort: collect every per-stage validation error for a single operator report instead of stopping at the first.
errors.append(f"Stage {stage.name!r}: merge failed: {exc}")
continue
# Project the output path the next stage would auto-chain to.
prev_output = os.path.join(merged.training.output_dir, "final_model")
Comment on lines
+628
to
+638
| def _load_state_file(self) -> Optional[PipelineState]: | ||
| path = self.paths["state_file"] | ||
| if not os.path.isfile(path): | ||
| return None | ||
| try: | ||
| with open(path, "r", encoding="utf-8") as f: | ||
| payload = json.load(f) | ||
| except (OSError, json.JSONDecodeError) as e: | ||
| logger.warning("Failed to read pipeline_state.json at %s: %s. Treating as missing.", path, e) | ||
| return None | ||
| return _deserialise_state(payload) |
There was a problem hiding this comment.
The _load_state_file method should catch potential TypeError, KeyError, or AttributeError that might be raised during _deserialise_state if the state file is corrupted or tampered with. This ensures the orchestrator handles malformed state files gracefully by treating them as missing.
Suggested change
| def _load_state_file(self) -> Optional[PipelineState]: | |
| path = self.paths["state_file"] | |
| if not os.path.isfile(path): | |
| return None | |
| try: | |
| with open(path, "r", encoding="utf-8") as f: | |
| payload = json.load(f) | |
| except (OSError, json.JSONDecodeError) as e: | |
| logger.warning("Failed to read pipeline_state.json at %s: %s. Treating as missing.", path, e) | |
| return None | |
| return _deserialise_state(payload) | |
| def _load_state_file(self) -> Optional[PipelineState]: | |
| path = self.paths["state_file"] | |
| if not os.path.isfile(path): | |
| return None | |
| try: | |
| with open(path, "r", encoding="utf-8") as f: | |
| payload = json.load(f) | |
| return _deserialise_state(payload) | |
| except (OSError, json.JSONDecodeError, AttributeError, TypeError, KeyError) as e: | |
| logger.warning("Failed to read pipeline_state.json at %s: %s. Treating as missing.", path, e) | |
| return None |
Comment on lines
+755
to
+763
| expected_dir = os.path.dirname(stage_cfg.model.name_or_path) | ||
| if not os.path.isdir(expected_dir): | ||
| stage_state.error = ( | ||
| f"Stage {stage.name!r} requires the previous stage's output at " | ||
| f"{stage_cfg.model.name_or_path}; pass --input-model <path> to " | ||
| f"override or run the full pipeline first." | ||
| ) | ||
| logger.error(stage_state.error) | ||
| return EXIT_CONFIG_ERROR |
There was a problem hiding this comment.
The existence check for auto-chained models uses os.path.dirname on the model path, which checks the parent directory instead of the model directory itself. If the parent directory exists but the final_model directory is missing (e.g., due to a previous stage crash), the check will pass, leading to a failure later in the trainer. The check should be performed on the full path directly.
Suggested change
| expected_dir = os.path.dirname(stage_cfg.model.name_or_path) | |
| if not os.path.isdir(expected_dir): | |
| stage_state.error = ( | |
| f"Stage {stage.name!r} requires the previous stage's output at " | |
| f"{stage_cfg.model.name_or_path}; pass --input-model <path> to " | |
| f"override or run the full pipeline first." | |
| ) | |
| logger.error(stage_state.error) | |
| return EXIT_CONFIG_ERROR | |
| if not os.path.isdir(stage_cfg.model.name_or_path): | |
| stage_state.error = ( | |
| f"Stage {stage.name!r} requires the previous stage's output at " | |
| f"{stage_cfg.model.name_or_path}; pass --input-model <path> to " | |
| f"override or run the full pipeline first." | |
| ) | |
| logger.error(stage_state.error) | |
| return EXIT_CONFIG_ERROR |
Addresses two review cycles on the Phase 14 pipeline-chains branch. Blocking (Review 1): - B-1 dispatch order: pipeline branch now runs before _maybe_run_no_train_mode so --dry-run reaches the orchestrator instead of the single-stage validator. - B-2 strong-reference revert: stale state-file paths from prior runs no longer leak into the resume flow; hash mismatch now hard-aborts with a precise error unless --force-resume is given. - B-3 chain integrity: _verify_manifest_payload now compares each stage against its immediate predecessor rather than the most-recent-with-output, so a hash mismatch on stage N cannot be masked by stage N-2's output. Significant: - F-1 leaf-vs-parent dir: replaced os.path.dirname(...) with os.path.isdir(stage_cfg.model.name_or_path) — the parent check was always true for any non-root path. - F-2 empty --input-model: '' is normalised to None at dispatch time so the orchestrator never sees a falsy override that the truthy guard would silently treat as 'no override'. - F-3 --force-resume audit trail: force path now emits a dedicated pipeline.force_resume event with both old + new pipeline_config_hash before mutating state. Mismatch path without --force returns a precise diagnostic string rather than raising. - S-2 finalised-manifest verifier: flags a manifest that still has running stages as a structural integrity failure. - S-4 atomic state writes: pipeline_state.json writes go through a tmp + os.replace round-trip to survive crashes mid-write. - R1-S4 tamper guard: _load_state_file catches KeyError/TypeError/ ValueError from _deserialise_state and treats a structurally invalid state file as 'no resume state' instead of crashing. Nitpicks: - N-1 docstring drift on PipelineStage.training (clarified that trainer_type is required when training: is supplied). - N-2 verifier flags running stages on finalised manifest. - N-3 audit-write failures are non-fatal — pipeline continues, warning is logged, regression test added. - N-5 _emit_summary JSON is round-trippable (regression test added). - N-6 gated stages emit a dedicated pipeline.stage_gated audit event (separate from .stage_completed). - N-7 inheritance-matrix 'data' row marked 'strongly discouraged' in both EN + TR guides and the roadmap doc. - N-9 release-time follow-up callout added to roadmap doc. - N-10 force-resume test now asserts both old + new hashes in the audit payload, not just one. N-8 (test helper deduplication into tests/_helpers/) deferred to a follow-up PR — review explicitly labeled it 'Skip — follow-up PR'. Verification: - pytest tests/test_pipeline_*.py → 115 passed - ruff format + ruff check → clean - 11-guard gauntlet → all green (dry-run, bilingual parity, anchors, wizard defaults, no-analysis-refs, sys.modules guard, site version, CLI-help consistency, field descriptions, pip-audit, bandit) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
Author
Review-response — Phase 14 pipeline chainsCombined response to both review cycles (Review 1: 3 blocking + 3 significant + 1 nitpick; Review 2: 3 significant + 10 nitpicks). All findings addressed in Blocking (Review 1) — all fixed
Significant — all fixed
Nitpicks (Review 2) — addressed
Deferred
VerificationReady for re-review. |
Three new findings from gemini-code-assist on top of the 0d58f1b batch + a small hardening tweak. All three appear in the PR as inline review comments on forgelm/cli/_pipeline.py. F-G-1 — output_dir collision check in dry-run (medium): Two stages resolving to the same training.output_dir silently overwrite each other's per-stage manifests + checkpoints. The footgun is real for stages that inherit the root training block wholesale. dry_run() now flags the collision (both explicit-override and inherited cases) via EXIT_CONFIG_ERROR before any GPU work. F-G-2 — AttributeError on tampered state (medium): Round out the _load_state_file shape-tamper guard with AttributeError alongside KeyError / TypeError / ValueError. Covers the case where a hand-crafted stages: [null, "foo"] state file makes s.items() raise AttributeError on the non-dict item. F-G-3 — chain-existence leaf check: Already addressed in 0d58f1b (F-1); re-confirmed here. Tests added: - test_dry_run_flags_output_dir_collision — explicit collision via two stage training blocks with the same output_dir. - test_dry_run_flags_inherited_output_dir_collision — two stages that both inherit the root training.output_dir (no per-stage override). CHANGELOG: extended the review-response section with the F-N-1 pipeline.stage_gated event (also new in this round) and the F-G-1 collision guard. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
Author
Phase 14 PR #53 — Gemini-inline review response (commit b8512f0)Addressed all 3 inline comments from 1. 2. 3. Verification
|
Addresses the Codacy "2 new issues" gate and the SonarCloud "Quality Gate
failed (E Reliability Rating on New Code)" gate on the Phase 14 PR.
BLOCKER bugs (SonarCloud python:S930 — root cause of the Reliability
Rating regression):
- forgelm/webhook.py _send: pipeline notifiers (notify_pipeline_started/
completed/reverted) were passing stage_count / final_status /
stopped_at / stage_name as keyword arguments to a fixed-signature
_send method. TypeError raised at runtime, swallowed by the pipeline
orchestrator's best-effort try/except → pipeline webhooks silently
never fired in production.
_send now accepts **extra and merges them into the payload (with a
base-field-collision guard so the contract stays stable).
MAJOR bugs:
- tests/test_pipeline_compliance.py _three_stage_state: removed dead
`"completed" if all_completed else "completed"` ternary that always
returned the same value (SonarCloud python:S3923). Dropped the unused
`all_completed` kwarg too.
- tests/test_pipeline_config.py: switched a float == 1.5 assertion to
pytest.approx() (SonarCloud python:S1244).
MAJOR code smells:
- forgelm/cli/_pipeline.py _run_single_stage merge-failure branch:
swapped logger.error(... exc) for logger.exception(...) (Sonar S8572)
so the traceback lands in the log alongside the error message.
- Trainer-crash catch: pulled the long inline noqa rationale out into a
preceding comment block so the # noqa: BLE001 marker has nothing
trailing it (Sonar python:S7632 disliked the trailing — em-dash
prose).
Codacy + minor smells:
- tests/test_pipeline_orchestrator.py: round = _deserialise_state(...)
shadowed the builtin; renamed to round_tripped.
- tests/test_pipeline_compliance.py: renamed test_*is_OK to test_*is_ok
so the method name matches the standard snake_case regex.
- tests/test_pipeline_cli.py _ns: dict(...) constructor → {} literal.
Verification:
- ruff format + ruff check clean.
- 149/149 pipeline + webhook tests pass.
- All 11 self-review guards green.
Cognitive-complexity CRITICAL code smells (run() at 53, _verify_*
at 24, etc.) are deliberately not addressed in this commit — they are
Maintainability-rating items and do not affect the failing
Reliability Rating gate, and refactoring them safely without
behavioural drift would warrant its own PR.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
Author
CI Quality Gate response — commit b3c6a45Codacy (2 issues) + SonarCloud (16 issues, E Reliability Rating) gates addressed: BLOCKER bugs fixed (SonarCloud python:S930 — root cause of the Reliability regression)
MAJOR bugs fixed
MAJOR code smells fixed
Codacy + minor smells
Out of scopeCRITICAL cognitive-complexity smells ( Verification
|
Addresses the 5 remaining CRITICAL code-smell findings on the Phase 14 PR by extracting helper methods. Behaviour is preserved end-to-end — the 227-test sweep (pipeline + CLI + verify-annex-iv + webhook + library API) passes byte-identical to b3c6a45. forgelm/cli/_dispatch.py:166 _main_inner (17 → ≤15): Extracted _dispatch_legacy_data_audit (the --data-audit alias path with its DeprecationWarning + audit-log breadcrumb + delegation to _run_data_audit) and _dispatch_pipeline_mode (the YAML re-read + run_pipeline_from_args dispatch). _main_inner is now a thin argparse-then-route entry point. forgelm/cli/_pipeline.py:289 run() (53 → ≤15): This was the biggest one. Split into six helpers, each with one cohesive responsibility: - _prepare_resume_or_init_state — loads existing state for resume or builds a fresh one. Returns (state, None) on success or (None, EXIT_CONFIG_ERROR) for invalid resume paths. - _resolve_resume_skiplist — walks state.stages before the resume point and builds the "already-completed on disk" skip list. - _resolve_stage_filter_setup — validates --stage <name> + seeds the auto-chain prev_output when filtering to a non-first stage. - _execute_one_stage — per-stage setup (started_at / status / trainer_type / save) + delegates to _run_single_stage + records duration. - _handle_stage_outcome — translates the per-stage exit code into the next-state transition (gated / completed / failed) + audit/webhook event + chain_broken flag. - _finalise_pipeline — phase 5+6: terminal state + pipeline.completed. The retained loop is just dispatch + skip/done bookkeeping; the branching that drove the cognitive-complexity score lives in the helpers, each at single-digit complexity. forgelm/cli/_pipeline.py:821 _run_single_stage (18 → ≤15): Extracted: - _resolve_input_source — pure function returning the InputSourceLiteral via the documented priority ladder (CLI override > stage explicit > chain > root). - _merge_and_validate_stage — merge_pipeline_stage_config wrap + chain-presence guard. Returns the merged ForgeConfig or None on a config-error path (caller maps to EXIT_CONFIG_ERROR). - _invoke_trainer — heavy-import-guarded ForgeTrainer lifecycle. Returns the TrainResult or None on a runtime/import failure. _run_single_stage is now linear: resolve source → merge → audit start → invoke trainer → record results. forgelm/cli/subcommands/_verify_annex_iv.py:188 (27 → ≤15): Extracted: - _run_pipeline_mode — --pipeline flag branch (manifest + chain- integrity violations). - _verify_artefact_and_handle_io_errors — the FileNotFoundError / IsADirectoryError / JSONDecodeError / OSError → exit-code mapping that handles the single-artefact path's I/O failures. - _print_artefact_result — text + JSON output formatting. _run_verify_annex_iv_cmd is now: validate path → branch on mode → delegate. forgelm/compliance.py:1057 _verify_manifest_payload (24 → ≤15): Lifted the per-stage chain-integrity check into _check_chain_link (pure function, idx-based for "compare against immediate predecessor") and the stopped_at + running-stage finalised checks into _check_status_consistency. Top-level keys list lifted to module-level constant _PIPELINE_MANIFEST_REQUIRED_KEYS. All 11 self-review guards remain green; ruff format + check clean; 227 / 227 tests pass on the broader pipeline + CLI sweep. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
Author
SonarCloud cognitive complexity — commit a65d3c9All 5 CRITICAL python:S3776 findings addressed by extracting helper methods. Behaviour preserved end-to-end (227/227 pipeline + CLI + verify-annex-iv + webhook + library API tests pass byte-identical).
Verification:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 11-16: The changelog currently contradicts itself about required
stage count; pick the true contract (e.g., minimum 2 stages) and make the
wording consistent across the bullet for
PipelineConfig/PipelineStage/ForgeConfig: replace both “2+ stages” and “≥1-stage
minimum” with a single clear phrase such as “minimum of 2 stages” (or “at least
1 stage” if that is the intended rule), and ensure the text references the same
constraint enforced by PipelineConfig and PipelineStage validation.
In `@docs/guides/pipeline-tr.md`:
- Line 3: Update the phrasing that currently states "Faz 14 — `v0.7.0` ile
geldi." to a pre-release form indicating planned or targeted status (e.g., "Faz
14 — `v0.7.0` için eklendi/hedefleniyor" or equivalent) so it does not read as
already shipped; replace the exact string "Faz 14 — `v0.7.0` ile geldi." with
the chosen pre-release wording.
In `@docs/guides/pipeline.md`:
- Line 3: Update the release-tense wording in the document where the phrase
"Phase 14 — added in `v0.7.0`." appears: replace "added in `v0.7.0`" with a
non-final wording such as "targeted for `v0.7.0`" (or "planned for `v0.7.0`") so
the line reads e.g. "Phase 14 — targeted for `v0.7.0`."
In `@forgelm/cli/_pipeline.py`:
- Around line 747-803: The method _validate_resume_state currently accepts a
stale config when force is true but doesn't verify stage topology; add a
pre-accept guard that compares existing.stages to the current
self.pipeline.stages (e.g., count and stable identifiers like stage names/order)
before emitting the warning/audit and updating existing.pipeline_config_hash; if
they differ, log and return an error string (don’t update the stored hash or
call _audit_event), otherwise proceed with the existing warning, call to
self._audit_event("pipeline.force_resume", ...), and update
existing.pipeline_config_hash as now implemented.
In `@forgelm/cli/subcommands/_verify_annex_iv.py`:
- Around line 188-218: The code in _run_pipeline_mode currently treats any
violations from verify_pipeline_manifest_at_path as EXIT_CONFIG_ERROR; instead,
catch runtime I/O errors separately and map them to EXIT_TRAINING_ERROR. Wrap
the call to verify_pipeline_manifest_at_path(path) in a try/except that catches
OSError/FileNotFoundError/PermissionError (or the specific exception
verify_pipeline_manifest_at_path raises for unreadable manifests) and on those
exceptions print a descriptive FAIL message and sys.exit(EXIT_TRAINING_ERROR);
keep the existing handling for returned violations (non-I/O validation errors)
using EXIT_CONFIG_ERROR. Also, if verify_pipeline_manifest_at_path encodes
unreadable-manifest as a special violation string, detect that string in
violations and treat it as EXIT_TRAINING_ERROR instead of EXIT_CONFIG_ERROR.
In `@forgelm/compliance.py`:
- Around line 1792-1802: The loop assumes manifest.get("stages") is a list of
dicts and uses s.get(...), which raises on malformed payloads; update the code
around _verify_manifest_payload(manifest) and the loop to first validate that
stages = manifest.get("stages") is a list, appending a violation if it is not,
and inside the for loop ensure each item s is a dict before calling s.get
(append a violation like "stage at index X is not an object" when not), then
only check per_stage_manifest and os.path.isfile for dict stages so violations
is returned instead of raising.
🪄 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: fdab5bfd-a8e1-4954-94d6-dbca3edc315b
📒 Files selected for processing (24)
CHANGELOG.mdconfig_template.yamldocs/guides/pipeline-tr.mddocs/guides/pipeline.mddocs/roadmap/phase-14-pipeline-chains.mdforgelm/cli/_dispatch.pyforgelm/cli/_parser.pyforgelm/cli/_pipeline.pyforgelm/cli/subcommands/_verify_annex_iv.pyforgelm/compliance.pyforgelm/config.pyforgelm/webhook.pytests/fixtures/pipeline/auto_revert_at_stage_2.yamltests/fixtures/pipeline/gated_pending_approval.yamltests/fixtures/pipeline/inheritance_matrix.yamltests/fixtures/pipeline/invalid_distributed_per_stage.yamltests/fixtures/pipeline/minimal_3_stage.yamltests/fixtures/pipeline/stale_state_resume_v1.yamltests/fixtures/pipeline/stale_state_resume_v2.yamltests/test_pipeline_cli.pytests/test_pipeline_compliance.pytests/test_pipeline_config.pytests/test_pipeline_orchestrator.pytools/check_bilingual_parity.py
Each finding was first verified against current code; fixes are
minimal and validated. All 232 broader-sweep tests pass; 11/11
self-review guards green; ruff format + check clean.
1) CHANGELOG.md — contradictory stage-count wording (valid):
"2+ training stages" + "≥1-stage minimum" said two different
things. Code enforces ``min_length=1``. Unified the bullet to
"one or more training stages" + "at least 1 stage (`min_length=1`
— …)" with the schema's nuance preserved (single-stage runs are
better served by omitting the `pipeline:` block, but the schema
accepts a 1-element pipeline for incremental authoring).
2) docs/guides/pipeline.md — release-tense wording (valid):
v0.7.0 is still ``[Unreleased]`` in CHANGELOG; the guide read
as already-shipped. "added in `v0.7.0`" → "targeted for `v0.7.0`
(currently on the `Unreleased` line of the CHANGELOG)".
3) docs/guides/pipeline-tr.md — release-tense wording (valid):
Same fix in Turkish. "`v0.7.0` ile geldi" → "`v0.7.0` için
hedefleniyor (şu an CHANGELOG'un `Unreleased` satırında)".
4) forgelm/cli/_pipeline.py:_validate_resume_state — topology
guard for `--force-resume` (valid):
Even under --force-resume the old code accepted any stale state,
so a renamed/inserted/deleted stage between runs would make
``state.stages[i]`` address a different stage than
``self.pipeline.stages[i]`` while the orchestrator iterates
positionally. Added a pre-emit guard that compares the ordered
stage-name list (count + identifier + order); on mismatch we log
+ return an error string *before* the audit hook fires and
before the stored hash is overwritten, so the JSONL audit trail
doesn't carry a force_resume event for a refused resume.
Regression test: test_force_resume_refuses_when_stage_topology_changed.
5) forgelm/cli/subcommands/_verify_annex_iv.py:_run_pipeline_mode —
I/O-vs-config exit-code mapping (valid):
``verify_pipeline_manifest_at_path`` already encodes I/O failures
as ``"… unreadable: …"`` sentinel strings, but the CLI mapped
every non-empty violation list to ``EXIT_CONFIG_ERROR (1)``,
contradicting the single-artefact path's policy
(FileNotFoundError → 1, OSError → 2 via
_verify_artefact_and_handle_io_errors). Now:
- try/except wraps the verifier call (defence-in-depth against
a future change that lets exceptions bubble) and maps OSError
→ ``EXIT_TRAINING_ERROR (2)``.
- The "unreadable" sentinel in the returned violations also maps
to exit 2; everything else (including the "not found" sentinel
and chain-integrity violations) stays at exit 1.
Regression tests: TestVerifyAnnexIvPipelineModeExitCodes — three
tests covering missing → 1, unreadable → 2, chain-violation → 1.
6) forgelm/compliance.py:_verify_manifest_payload +
verify_pipeline_manifest_at_path — non-dict stage type-guard
(valid):
Both the in-memory verifier *and* the disk wrapper iterated
manifest["stages"] and called ``s.get(...)`` without checking each
``s`` is a dict — a tampered manifest with ``stages: [null,
"string"]`` would crash with AttributeError instead of producing
a structured violation. Added per-item ``isinstance(s, dict)``
check in both code paths; the in-memory verifier filters the
well-formed subset out and runs the rest of the checks on that
filtered list so downstream helpers (which assume dicts) stay
stable. Regression test:
test_disk_wrapper_type_guards_non_dict_stage_items pins both
surfaces.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The first node of the flowchart in docs/usermanuals/{en,tr}/training/lora.md
embedded a bare ``<`` inside the rhombus label:
Q1{Need to fit on a<br/>consumer GPU<br/>(<24 GB)?}
Mermaid 10.9.4 parses the ``<`` as the start of an HTML tag inside a
``{...}`` rhombus node, fails to find a matching ``>``, and aborts the
whole diagram with "Syntax error in text". The site's user-manual
renderer surfaces this verbatim where the diagram should be — the
exact error the user reported.
Fix: wrap the affected label in double quotes so Mermaid treats the
contents literally:
Q1{"Need to fit on a<br/>consumer GPU<br/>(<24 GB)?"}
Same fix in the Turkish mirror. No other mermaid label in the user
manuals uses an unquoted ``<`` — verified by grep across
docs/usermanuals/.
The site/js/usermanuals/*.js bundles are gitignored (regenerated at
CI build via ``tools/build_usermanuals.py``); ``build_usermanuals.py
--check`` is green so the next deploy picks the fix up.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Following the LoRA decision-diagram fix (6a6ca60), a scan of every mermaid block under docs/usermanuals/, docs/guides/, docs/reference/, docs/qms/, docs/standards/, and docs/roadmap*/ surfaced 5 more node labels with unquoted special characters that could trip Mermaid's parser depending on version. Fix in all of them: wrap the label in ``"…"`` so Mermaid takes the contents literally — same approach as the LoRA fix. Identified by a regex scanner that walks each mermaid fence, parses out the shape brackets, strips safe ``<br/>`` tokens, and flags any remaining bare ``<`` / ``>`` / ``&`` in the label. Scanner re-runs clean after these fixes. Files: - docs/usermanuals/en/getting-started/introduction.md:35 ``I[Export &<br/>deploy]`` — bare ``&`` immediately before ``<br/>`` reads as a partial HTML entity start. TR mirror uses "ve" instead of "&" and was already safe. - docs/usermanuals/{en,tr}/concepts/alignment-overview.md:13 ``SFT[SFT<br/>format & content]`` (EN) and ``SFT[SFT<br/>format & içerik]`` (TR) — same bare-``&`` risk. - docs/reference/data_ingestion_architecture{,_tr}.md ~L210 ``CHECK{>= 70 percent of pages?}`` (EN) and ``CHECK{>= yüzde 70 sayfa?}`` (TR) — bare ``>=`` at the start of a rhombus label can confuse Mermaid because ``>`` overlaps with the asymmetric-shape syntax. - docs/roadmap/completed-phases.md:425 ``F{> threshold?}`` — same bare-``>`` issue. User-manual bundles regenerated via ``tools/build_usermanuals.py`` (the site/js/usermanuals/*.js bundles are gitignored — CI rebuilds on deploy). Bilingual parity + anchor-resolution guards green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… SystemExit Two findings from the latest Sonar pass on a65d3c9: forgelm/cli/_pipeline.py:550 — run() cognitive complexity 22 → ≤15 After the previous refactor (a65d3c9, 53 → ≤15) the for-loop body still contributed ~7 to the score: three nested skip checks, two post-execution conditional updates, and one ``should_break`` guard all sat inside a for inside the method. Lifted the entire stage loop into ``_run_stage_loop`` and the skip-decision into ``_stage_skip_reason`` (returns a discriminated string for "filtered" / "already_completed" / "chain_broken" / None). Inline the two ``if x: y = z`` post-execution updates as ternary / boolean-or expressions: prev_output = new_prev_output if new_prev_output is not None else prev_output chain_broken = chain_broken or chain_broken_now ``run()`` is now linear: prepare state → resume skiplist → filter setup → started event → loop → finalise → summary. tests/test_pipeline_compliance.py:496 — try/except SystemExit (S5754) The ``_run`` helper in TestVerifyAnnexIvPipelineModeExitCodes wraps ``_run_verify_annex_iv_cmd`` in a bare ``try/except SystemExit`` to capture the CLI's exit code. Sonar flagged the catch as the "swallow without reraise" anti-pattern (BaseException subclass — ``SystemExit`` inherits from BaseException, not Exception). Replaced with ``pytest.raises(SystemExit)`` — pytest's idiomatic context manager for CLI tests. The contract is now positive ("we *expect* SystemExit every invocation") rather than speculative-catch, and a future code path that fails to ``sys.exit`` would surface as a test failure rather than being silently masked. Verification: - ruff format + check clean - 154 / 154 pipeline + webhook tests pass Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two independent final-review reviewers spotted 6 findings; both were
verified against HEAD before any change. All 6 are addressed.
🔴 F-B-1 (Blocking) — Pipeline audit events got different run_ids
Verified: AuditLogger.__init__ accepts an optional run_id; without
one it auto-generates ``fg-<uuid>``. ``_audit_event`` constructed
a fresh AuditLogger per call, so every entry in the pipeline-level
``audit_log.jsonl`` carried a different top-level ``run_id`` field
even though the events shared the same ``pipeline_run_id`` payload.
That breaks SIEM-style grouping and the Article 12 record-keeping
correlation contract — auditors join on ``run_id``.
Fix: cache an AuditLogger on the orchestrator, pinned to the
pipeline run id, lazily initialised on the first event.
``_audit_event`` now requires ``pipeline_run_id`` as a keyword-only
argument (all 8 existing call sites already pass it, so the new
signature is back-compat-safe and prevents silent regression).
Regression test: test_audit_events_share_one_pipeline_run_id.
🔴 F-B-2 (Blocking) — --stage <non-first> manifests fail the chain verifier
Verified: ``--stage dpo_stage`` with the predecessor's
``final_model`` on disk runs correctly (auto-chains from the
on-disk seed) but the predecessor's manifest entry has
``status: skipped_by_filter`` and ``output_model: null``. The
executed stage records ``input_source: "chain"``, so the strict
chain verifier (F-B-3 from an earlier cycle) compares against
``stages[X-1].output_model = null`` and reports
``chain_integrity_violation: previous stage has no output_model``
on every legitimate ``--stage <non-first>`` run.
Fix: after ``_resolve_stage_filter_setup`` returns the resolved
disk seed, surface it on the predecessor's
``state.stages[filter_idx - 1].output_model`` so the manifest
accurately represents "stage X-1 produced this path (in a prior
run); stage X chained from it now". Strict chain verifier is
happy; stage X-1's status stays ``skipped_by_filter`` to convey
"we didn't run it in this invocation". Regression test:
test_filter_middle_stage_with_disk_seed_passes_chain_verifier.
🟡 F-S-1 (Significant) — Pipeline state writes raced on shared .tmp
Verified: ``_atomic_write_json`` used ``f"{path}.tmp"`` —
shared filename per target. Two writers truncate each other's
tmp and lose the race to ``os.replace`` with FileNotFoundError.
Phase 14 explicitly doesn't support concurrent orchestrators on
the same ``pipeline.output_dir`` (documented now under
Limitations), but the FileNotFoundError traceback was a worse
failure mode than last-writer-wins.
Fix: switch to ``f"{path}.{pid}.{rand}.tmp"`` for the temp name.
Each writer gets its own tmp file, atomically replaces. Added a
defensive cleanup on write failure (orphan tmp removed; original
exception re-raised). The temp lives in the same directory as
the target so ``os.replace`` stays a same-filesystem atomic
rename.
🟢 F-N-1 (Nitpick) — CHANGELOG audit-event count was stale
The Added section said "5 new audit events" enumerating the
original five; HEAD emits 7 (added stage_gated in F-N-1 and
force_resume in F-B-2 from earlier cycles). Updated the count
+ list; also called out the F-B-1 run_id-pinning fix.
🟢 F-N-2 (Nitpick) — Limitations doc gaps
docs/guides/pipeline.md + pipeline-tr.md gained three new bullets:
- No multi-process locking on pipeline.output_dir (concurrent
orchestrators race; pick distinct dirs).
- Stage-vs-pipeline output_dir overlap is allowed but interleaves
the audit log (matches the inheritance_matrix.yaml fixture's
layout — documented, not a bug).
- --force-resume topology guard catches positional breaks only;
value-level edits (trainer_type, output_dir, hyperparameters
under identical names) are accepted by design.
🟢 F-N-3 (Nitpick) — _validate_resume_state docstring clarity
The topology-guard comment now spells out that value-level edits
are accepted by design — the operator's explicit signal under
``--force-resume`` that they understand the divergence. Aligns
the code's contract with the guide's Limitations bullet.
Verification:
- ruff format + check clean
- 159 / 159 pipeline + webhook tests pass (new audit-run_id +
--stage-disk-seed tests added)
- All 11 self-review guards green
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
Author
Final review round — commit cd9f416Two independent final-round reviewers (one Approve with nitpicks, one Block with 2 blockers + 1 significant) both verified against HEAD. 6 findings; all addressed. Blocking — Reviewer 2🔴 F-B-1 — Pipeline audit events had different top-level Fix: cache 🔴 F-B-2 — Fix: after Significant — Reviewer 2🟡 F-S-1 — Fix: Nitpicks — both reviewers🟢 F-N-1 — CHANGELOG audit-event count stale (5 → 7). Lists 🟢 F-N-2 — 🟢 F-N-3 — Out of scope (acknowledged by Reviewer 1 as nitpicks for v0.7.0)
Verification
|
A single CRITICAL python:S5754 finding on cd9f416's new atomic-write race regression test: ``except BaseException`` is the documented anti-pattern (SystemExit / KeyboardInterrupt should propagate). Narrowed to ``except Exception`` — still wide enough to capture any unexpected escape from ``_atomic_write_json`` for the race-probe assertion, but doesn't shadow SystemExit / KeyboardInterrupt the way ``BaseException`` does. The race the test actually targets is an OSError (FileNotFoundError from ``os.replace`` losing to a concurrent unlink), well within ``Exception``. Test still passes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
cemililik
added a commit
that referenced
this pull request
May 14, 2026
…ng site Phase 14 multi-stage pipelines (PR #53) merged into development without the promotional / discoverability surface around it. This commit lands that surface so operators looking for "chain my trainers together" actually find the feature. Scope (single content drop — no behavioural changes): 1. README.md — added "Multi-Stage Pipelines" row to the Guides table between Alignment and CI/CD Pipeline Integration, with EN + TR guide links. 2. site/features.html — added a "Multi-Stage Pipelines" feature card in the Training & Alignment group (between GRPO and QLoRA), with a chain-icon SVG and short copy: "Chain SFT → DPO → GRPO in one config-driven run. Auto-chained inputs, per-stage gates, crash-safe resume, one Annex IV manifest covering the whole chain." 3. site/js/translations.js — new `features.train.pipeline.{title,body}` i18n keys in all 6 languages (EN/TR/DE/FR/ES/ZH) for the card. 4. site/index.html — new 4th hero slide (after compliance) using new `home.hero.pipeline.*` keys. Title "SFT → DPO → GRPO, one config.", CTAs to the pipeline guide + YAML example. Slider auto-detects the new slide (DOM-driven in main.js); new pagination dot added with localised aria-label. 5. site/js/translations.js — new `home.hero.pipeline.{title,lead, cta1,cta2}` + `home.hero.dot.pipeline` i18n keys in all 6 languages. Note: pre-existing orphan `home.pipeline.*` keys (trainer-stack pitch from a prior section) are left intact; they describe a different concept (six paradigms) and a separate marketing pass can decide whether to wire or remove them. 6. docs/usermanuals/{en,tr}/training/pipelines.md — new sidebar page under Training & Alignment, mirroring the structure of dpo.md / grpo.md (mermaid decision tree, minimal config, CLI surface, what-the-orchestrator-gives-you, inheritance matrix, limitations, see-also). Sources of truth remain `docs/guides/pipeline{,-tr}.md`; the manual page is the discovery surface inside the sidebar. 7. docs/usermanuals/_meta.yaml — registered the new page (`id: pipelines`) between `grpo` and `lora` in the Training & Alignment section, with localised titles in all 6 languages. 8. docs/usermanuals/{en,tr}/concepts/choosing-trainer.md — added a short note under "Common starting points" pointing every "→"-shaped trainer sequence at the new pipelines page, plus a "See also" entry. 9. site/js/usermanuals/{en,tr,_index}.js — regenerated via tools/build_usermanuals.py (66 pages per language, up from 65). Validation (all green): - ruff format + check - 11-guard gauntlet (dry-run, bilingual parity, anchor resolution, CLI help consistency, wizard defaults, no-analysis-refs, unguarded-sys-modules-pop, site-version, build_usermanuals --check) - 180 / 180 tests across pipeline + bilingual-parity + anchor-resolution Out of scope (deferred): - CHANGELOG move from [Unreleased] → [0.7.0] — release will be cut after this surface refresh is approved. - Wizard (CLI + web) pipeline support — Phase 14 explicit out-of-scope. - Old `home.pipeline.*` orphan i18n keys — separate marketing pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
5 of 7 tasks
cemililik
added a commit
that referenced
this pull request
May 15, 2026
… 2 MEDIUM) Each finding was verified against current code; 10 were applied as minimal edits, 4 documented as out-of-scope for the release-cut PR. 🔴 BLOCKER 1 — Pipeline dispatch silently overrode no-train flags File: forgelm/cli/_pipeline.py Verified: ``_dispatch_pipeline_mode`` routes BEFORE ``_maybe_run_no_train_mode`` (the F-B-1 fix from PR #53 was explicit about this), so ``--fit-check`` / ``--merge`` / ``--generate-data`` / ``--compliance-export`` / ``--benchmark-only`` on a pipeline config would silently trigger a full pipeline training run instead of the single-stage operation requested. Fix: ``run_pipeline_from_args`` now rejects each of the five unsupported single-stage flags with EXIT_CONFIG_ERROR + an actionable error pointing operators at ``--dry-run`` for multi-stage validation. ``--dry-run`` itself still routes to ``orchestrator.dry_run()``. Regression test: ``test_no_train_single_stage_flags_rejected_on_pipeline_config``. 🔴 BLOCKER 2 — --resume-from did not skip gated_pending_approval File: forgelm/cli/_pipeline.py:376 Verified: ``_resolve_resume_skiplist`` only skipped stages with ``status == "completed"``. A stage that exited with EXIT_AWAITING_APPROVAL is recorded as ``gated_pending_approval``. After ``forgelm approve`` promotes the staging path and the operator resumes from a later stage, the gated SFT would re-train, DPO would auto-chain from the freshly-trained output instead of the operator-approved one, and the audit log would carry two ``pipeline.stage_started`` events for the same stage_index. Fix: skip set now contains both ``completed`` and ``gated_pending_approval`` — presence of an ``output_model`` directory on disk is what proves the stage produced a real artefact, regardless of which status label it carries. Regression test: ``test_resume_skips_gated_pending_approval_with_on_disk_output``. 🔴 BLOCKER 3 — Topology guard skipped on hash match File: forgelm/cli/_pipeline.py:909 Verified: ``_validate_resume_state`` returned ``None`` immediately when ``existing.pipeline_config_hash == self.config_hash``, before the topology check ran. A tampered / truncated / hand-edited ``pipeline_state.json`` could carry the same config_hash (operator never edited the YAML) but a damaged ``stages[]`` array, and the resume would silently overwrite the wrong stage's history because ``state.stages[i]`` is positionally addressed. Fix: topology check (ordered ``[s.name for s in stages]`` lists) now runs UNCONDITIONALLY — before the hash shortcut. Regression test: ``test_topology_guard_runs_even_on_hash_match``. 🟠 HIGH 4 — output_dir collision check only in dry_run() File: forgelm/cli/_pipeline.py:770 Verified: the per-stage ``training.output_dir`` collision guard only fired during ``--dry-run``; an operator who skipped dry-run could silently overwrite per-stage manifests + checkpoints across stages. Fix: extracted the collision check into ``_check_output_dir_collisions`` and now call it as a pre-flight in ``run()`` too. Regression test: ``test_output_dir_collision_rejected_in_run_not_only_dry_run``. 🟠 HIGH 5 — Pipeline-only flags silently ignored on non-pipeline configs File: forgelm/cli/_dispatch.py:300 Verified: ``--stage`` / ``--resume-from`` / ``--force-resume`` / ``--input-model`` against a config with no ``pipeline:`` block was a no-op — operator surprise. Fix: ``_main_inner`` now rejects each pipeline-only flag with EXIT_CONFIG_ERROR + an actionable error. Regression test: ``test_pipeline_only_flags_rejected_on_non_pipeline_config``. 🟠 HIGH 8 — Reference docs missing the pipeline: block + new CLI flags Files: docs/reference/configuration.md, configuration-tr.md, usage.md, usage-tr.md Verified: docs/standards/documentation.md mandates that config / CLI surface changes are mirrored in the canonical reference docs (EN + TR) before merge. Phase 14's ``pipeline:`` block and the four new CLI flags were not yet documented. Fix: appended a "pipeline (Optional — Multi-Stage Training Chains, Phase 14)" section to ``configuration.md`` + TR mirror covering the schema (root + PipelineStage fields, inheritance rules, root-only sections, YAML example, CLI surface, verifier command). Added a "Multi-Stage Pipelines (Phase 14, v0.7.0)" subsection to ``usage.md`` + TR mirror with the 7 CLI invocations and the single-stage-flag-rejection contract. Cross-linked both directions. 🟠 HIGH 9 — Module-size grandfather list policy was a frozen v0.6.x set File: tools/check_module_size.py Verified: the file-level docstring claimed the grandfather list was specifically "the seven modules that already sit over the ceiling at PR #29 HEAD"; adding ``forgelm/cli/_pipeline.py`` to that list without acknowledging the policy change weakens the guard's discipline. Fix: rewrote the grandfather-policy section of the docstring to make the admission criteria explicit (inline-comment requirement + follow-up tracking artefact) and called out the post-PR-#29 addition (``_pipeline.py``). The discipline IS preserved — every entry has a tracking pointer. 🟡 MEDIUM 12 — JSONDecodeError conflated with OSError in pipeline verifier Files: forgelm/compliance.py, forgelm/cli/subcommands/_verify_annex_iv.py Verified: ``verify_pipeline_manifest_at_path`` emitted a shared "unreadable" sentinel for both ``OSError`` and ``JSONDecodeError``, so the CLI mapped malformed JSON to EXIT_TRAINING_ERROR (exit 2) even though parse failures are operator-actionable input errors (the single-artefact path maps them to exit 1). Fix: verifier now emits a distinct ``invalid JSON`` sentinel for ``JSONDecodeError`` and keeps ``unreadable`` for ``OSError`` only. CLI maps ``invalid JSON`` → EXIT_CONFIG_ERROR (1), ``unreadable`` → EXIT_TRAINING_ERROR (2). Existing test that asserted exit 2 on malformed JSON was a stale carry-forward; updated to match the corrected contract. 🟡 MEDIUM 13 — Roadmap state inconsistent with the cut release Files: docs/roadmap.md, docs/roadmap-tr.md, docs/guides/pipeline.md, docs/guides/pipeline-tr.md Verified: Phase 14 row still said "📋 Planned" and the ``Current state:`` line still said "Phase 14 planned post-release" while CHANGELOG.md is dated [0.7.0] — 2026-05-14 and pyproject.toml reports version 0.7.0. The pipeline guide's header said "currently on the Unreleased line of the CHANGELOG" which is no longer true. Fix: Phase 14 row → "🟡 Merged — implementation complete on `development`; `v0.7.0` PyPI tag is the release-pending step". Current-state and tree-comment lines updated to match. Guide header → "landing in `v0.7.0` (CHANGELOG entry dated 2026-05-14; PyPI tag pending)". TR mirrors updated in lockstep. The full "Done" promotion to ``completed-phases.md`` lands as part of the post-tag dev-cycle reset commit (per cut-release skill step 4). 🟡 MEDIUM 14 — New pipelines.md pair not in bilingual parity registry File: tools/check_bilingual_parity.py Verified: docs/usermanuals/{en,tr}/training/pipelines.md added in the docs+site surface-refresh commit (d404092) but the heading-spine parity check (``_PAIRS`` registry) wasn't extended, leaving the pair open to structural drift. Fix: registered the pair in ``_PAIRS``. The check now reports "48 bilingual doc pair(s)" (was 47). ⏭️ Skipped for v0.7.0 cut (documented for v0.7.x follow-up) --- - HIGH 6 — canonical pipeline manifest hash + non-chain-field tamper detection: real but material design change (needs spec work + verifier overhaul on top of the existing Annex IV scheme). Not a release blocker because (a) the structural verifier already catches the most common tamper shapes (chain integrity, stage indexing, status consistency), (b) the per-stage manifests retain their existing canonical hashes, and (c) the pipeline manifest is written atomically per transition and the surrounding audit log is hash-chained — a non-chain-field tamper window is narrow. Tracked for v0.7.x. - HIGH 7 — disk verifier deep-parses each per-stage training_manifest: same complexity class as HIGH 6. Currently the disk-wrapper checks ``os.path.isfile(per_stage_manifest)``; deep parsing would embed the full Annex IV single-artefact verifier per stage. Filed as v0.7.x; pairs with HIGH 6 to land together as a "pipeline manifest verification rewrite" sub-phase. - MEDIUM 10 — webhook ``pipeline.*`` events extend the 5-event vocabulary: events were intentionally added with a documented rollout (CHANGELOG enumerates 7 new ``pipeline.*`` events; the webhook contract's ``event`` field is an open-ended string, not an enum; receivers that hard-validate against a frozen enum need to be updated, but the schema explicitly leaves room). Defer unless a downstream consumer reports the issue. - MEDIUM 11 — ``_send(**extra)`` allowlist: ``**extra`` is caller-controlled (orchestrator methods that already control the call site), not from external input, and the base-field-collision guard prevents the obvious attack. Tightening to an explicit allowlist is good hygiene but not a release blocker. Tracked for v0.7.x. Verification --- - ruff format + check clean - 222 / 222 tests pass (pipeline + compliance + CLI + dispatch + bilingual parity + anchor resolution + module size + cli subcommands) - All 11 doc guards green in the gauntlet: dry-run, site-version --check, bilingual parity (48 pairs), anchor resolution (274 markdown files), CLI help consistency (457 invocations), wizard defaults, no-analysis-refs, no-unguarded-sys-modules-pop, build_usermanuals --check, ruff, module-size (--strict, 7 grandfathered) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
cemililik
added a commit
that referenced
this pull request
May 15, 2026
publish.yml run 25906380797 (v0.7.0 tag push) failed on all four
Windows matrix cells (3.10/3.11/3.12/3.13) of cross-os-tests with:
AssertionError: Concurrent writers produced 12 exception(s):
[PermissionError(13, 'Access is denied'), ...]
tests/test_pipeline_orchestrator.py:246
The test
(``TestAtomicWriteJson.test_per_writer_temp_path_does_not_race_on_shared_tmp``)
pins the POSIX ``FileNotFoundError`` race that the F-S-1 fix on PR #53
addressed: pre-fix the helper used a shared ``<target>.tmp`` and
``os.replace`` from writer B could fail because writer A had just
unlinked the same tmp via its own rename. Per-writer-unique tmp
filenames eliminate that bug on POSIX.
On Windows, ``os.replace`` to the same destination path raises a
DIFFERENT error (``PermissionError: Access is denied``) when
concurrent writers briefly hold a handle on the destination — that's
a file-lock race intrinsic to the Win32 rename API, not the
tmp-rename race the F-S-1 fix is about. The pipeline orchestrator's
atomic-write contract is POSIX-shaped (sequential writers; the
test's 16-thread stress is purely a regression probe against the
original race shape), and Windows's concurrent-replace semantics
are not in scope.
Fix: marker ``@pytest.mark.skipif(sys.platform == "win32", ...)``
with a docstring explaining the platform-specific reason. The
POSIX-side regression coverage is preserved on the ubuntu-latest and
macos-latest matrix cells.
PyPI was NOT published in run 25906380797 — the ``pypi`` job is
gated on ``needs: cross-os-tests`` which never went green. The
v0.7.0 GitHub tag was deleted (local + remote) so this commit can be
re-tagged onto main HEAD and publish.yml re-fired cleanly.
Verification:
- ruff format --check + ruff check clean
- TestAtomicWriteJson passes locally (2/2; macOS)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
cemililik
added a commit
that referenced
this pull request
May 15, 2026
publish.yml run 25906380797 (v0.7.0 tag push) failed on all four
Windows matrix cells (3.10/3.11/3.12/3.13) of cross-os-tests with:
AssertionError: Concurrent writers produced 12 exception(s):
[PermissionError(13, 'Access is denied'), ...]
tests/test_pipeline_orchestrator.py:246
The test
(``TestAtomicWriteJson.test_per_writer_temp_path_does_not_race_on_shared_tmp``)
pins the POSIX ``FileNotFoundError`` race that the F-S-1 fix on PR #53
addressed: pre-fix the helper used a shared ``<target>.tmp`` and
``os.replace`` from writer B could fail because writer A had just
unlinked the same tmp via its own rename. Per-writer-unique tmp
filenames eliminate that bug on POSIX.
On Windows, ``os.replace`` to the same destination path raises a
DIFFERENT error (``PermissionError: Access is denied``) when
concurrent writers briefly hold a handle on the destination — that's
a file-lock race intrinsic to the Win32 rename API, not the
tmp-rename race the F-S-1 fix is about. The pipeline orchestrator's
atomic-write contract is POSIX-shaped (sequential writers; the
test's 16-thread stress is purely a regression probe against the
original race shape), and Windows's concurrent-replace semantics
are not in scope.
Fix: marker ``@pytest.mark.skipif(sys.platform == "win32", ...)``
with a docstring explaining the platform-specific reason. The
POSIX-side regression coverage is preserved on the ubuntu-latest and
macos-latest matrix cells.
PyPI was NOT published in run 25906380797 — the ``pypi`` job is
gated on ``needs: cross-os-tests`` which never went green. The
v0.7.0 GitHub tag was deleted (local + remote) so this commit can be
re-tagged onto main HEAD and publish.yml re-fired cleanly.
Verification:
- ruff format --check + ruff check clean
- TestAtomicWriteJson passes locally (2/2; macOS)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
cemililik
added a commit
that referenced
this pull request
May 15, 2026
v0.7.0 published to PyPI (publish.yml run 25906894834, all 12 cross-OS
matrix cells green, OIDC trusted publish success). Smoke test:
``pip install forgelm==0.7.0`` in a fresh venv reports
``forgelm --version → ForgeLM 0.7.0``. Post-release housekeeping:
1. ``pyproject.toml`` → ``0.7.1rc1`` (next dev cycle marker)
2. ``docs/roadmap.md`` + ``-tr.md``:
- Phase 14 row: "🟡 Merged" → "✅ Done" with v0.7.0 release
date + completed-phases anchor
- "Latest release on PyPI" / "PyPI'deki son sürüm" paragraph:
v0.6.0 → v0.7.0 with the Phase 14 + SSRF hardening summary;
v0.6.0 demoted to "Previous release" / "Önceki sürüm"
- "Released:" / "Yayınlandı:" line: v0.6.0 → v0.7.0
- "Current state" line: 20 phases → 21 phases (Phase 14 added),
PyPI through v0.7.0
- Mermaid: ``v0.7.0`` node → ``v0.7.0 ✅ Released`` /
``✅ Yayınlandı``
- Tree-comment for ``phase-14-pipeline-chains.md``: "Merged …
v0.7.0 tag pending" → "Done — shipped v0.7.0 (PyPI 2026-05-15)"
- ``releases.md`` comment: ``v0.3.0 → v0.6.0`` → ``v0.3.0 →
v0.7.0``
3. ``docs/roadmap/completed-phases.md``: appended a comprehensive
Phase 14 entry (driver, what shipped, security fold-in,
backward-compat invariant, full review-absorption history with
PR #53 + #54 round summaries, cross-references to operator
guides + schema + user manual pages). Slug
``#phase-14-multi-stage-pipeline-chains-v070`` matches the
target anchor used from ``roadmap.md`` and ``roadmap-tr.md``.
Verification:
- ruff format --check + ruff check clean
- 48 / 48 bilingual doc pair parity green
- 274 markdown files: all anchors + relative links resolve
(including the new ``completed-phases.md#phase-14-...`` anchors)
- site/ version literals still pin v0.7.0 (no change needed; the
bump to 0.7.1rc1 is dev-cycle only)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Phase 14 — chain 2+ training stages (SFT → DPO → GRPO) into one config-driven, dry-run-validatable, Annex-IV-traceable run. Implements every Wave 1 task from docs/roadmap/phase-14-pipeline-chains.md.
Backward compatibility: a config without a
pipeline:block reachesforgelm/trainer.pybyte-identically to v0.6.0 — the orchestrator module is never imported on single-stage runs.Surface delta
forgelm/config.pyPipelineStage,PipelineConfig,merge_pipeline_stage_config()forgelm/cli/_pipeline.py(new)forgelm/compliance.pygenerate_pipeline_manifest(),verify_pipeline_manifest_at_path(), structural+chain integrity helperforgelm/cli/_parser.py,_dispatch.py,_verify_annex_iv.py--stage,--resume-from,--force-resume,--input-model,verify-annex-iv --pipelineforgelm/webhook.pynotify_pipeline_started/completed/revertedtests/test_pipeline_{config,orchestrator,compliance,cli}.pytests/fixtures/pipeline/*.yamldocs/guides/pipeline.md+-tr.mdconfig_template.yamlpipeline:example blockCHANGELOG.md::[Unreleased]::AddedVerification
pytest tests/(with--ignore=test_phase12_review_fixes.py, the documented perf-flake from prior reviews): 1879 passed, 13 skipped, 0 failed in 28:30Release-time follow-up (deferred to
v0.7.0cut-releasecycle)Phase 14 doc carries an explicit "Release-time follow-up" callout for
docs/usermanuals/,site/features.html,site/index.html,site/js/translations.js, andtools/update_site_version.py. These are release-product surfaces and are intentionally not edited in this PR — they ship under thev0.7.0tag viacut-release.Test plan
forgelm --config pipeline.yaml --dry-runcollects every per-stage errorforgelm --stage dpo_stagewith missing prev-stage output → EXIT_CONFIG_ERROR=1 with the documented message shape--force-resumeallows with WARNINGrequire_human_approval: true→ EXIT_AWAITING_APPROVAL=4, state preserved for resumeforgelm verify-annex-iv --pipeline <run_dir>returns 0 on a clean manifest, non-zero with violations on a tampered one🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
--stage(run single stage),--resume-from(resume pipeline execution),--force-resume(override config validation), and--input-model(override stage input model).verify-annex-iv --pipeline.Documentation