Repository navigation
Conversation
Closure plan §18 deliverable. Design-only PR; no code changes. The document pins down what has been a *de facto* library API for several minor releases — `from forgelm import ForgeTrainer` works, but until now nothing told a downstream consumer which symbols are stable, which are experimental, and which are internal. Phase 19 implements the design without re-deriving any decisions. 12 sections + 16-row implementation plan covering: - Stable/experimental/internal tier split + per-symbol mapping - `py.typed` + `mypy --strict` contract for the public surface - Lazy-import discipline (`import forgelm` MUST NOT pull torch) + the regression test that pins it - Wave 1 round-5 carry-over resolved: `forgelm/cli/__init__.py` lazy `__getattr__` + `_SYMBOL_TO_MODULE` pattern, monkeypatch-safe - Two-segment `__api_version__` separated from `__version__` so CLI patch releases do not inflate the library contract version - Integration test cases Phase 19 ships (`tests/test_library_api.py` + `tests/test_cli_lazy_imports.py`) - Documentation deliverables (reference, guide, notebook, README) - Cross-cutting: logging hygiene, config-from-dict, error propagation at the library boundary, thread/process safety - Five open questions resolved Closes: closure-plan-202604300906.md §7 Phase 18 Unblocks: Phase 19 (Library API implementation) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Reviewer's GuideDesign-only PR that adds a detailed 480+ line ForgeLM Library API analysis/design document and a matching CHANGELOG entry, defining the future public library surface, stability tiers, lazy-import rules, type-checking contract, API versioning, integration tests, and documentation/tasks for the upcoming implementation phase, without touching any runtime code. Sequence diagram for planned lazy import of ForgeTrainer via forgelm.getattrsequenceDiagram
actor User
participant Py as PythonImporter
participant FM as forgelm
participant TR as forgelm.trainer
participant TC as torch
User->>Py: import forgelm
Py->>FM: load module forgelm
note over FM: Execute lightweight imports only<br/>Do not import torch or trainer
Py-->>User: forgelm module object
User->>FM: access ForgeTrainer
FM->>FM: __getattr__("ForgeTrainer")
FM->>TR: import forgelm.trainer
TR->>TC: import torch (and other heavy deps)
TR-->>FM: ForgeTrainer class
FM-->>User: ForgeTrainer reference
User->>FM: dir(forgelm)
FM->>FM: __dir__ returns stable and experimental symbols
FM-->>User: list including ForgeTrainer
Class diagram for planned ForgeLM library API surfaceclassDiagram
direction LR
class forgelm {
<<package>>
}
class ForgeConfig {
<<stable>>
+fields...
+validate()
}
class ForgeTrainer {
<<stable>>
+ForgeTrainer(config: ForgeConfig)
+train() TrainResult
}
class TrainResult {
<<stable>>
+success bool
+metrics dict
+artifacts_path str
}
class BenchmarkResult {
<<stable>>
+task_name str
+scores dict
}
class AuditReport {
<<stable>>
+total_samples int
+issues dict
+croissant dict
}
class VerifyResult {
<<stable>>
+valid bool
+reason str
}
class AuditLogger {
<<stable>>
+AuditLogger(output_dir: str, run_id: str)
+log_event(event: str, fields: dict)
}
class WebhookNotifier {
<<experimental>>
+WebhookNotifier(config: object)
+notify_event(event: str, payload: dict)
}
class ConfigError {
<<stable>>
+message str
}
class LibraryFunctions {
<<facade>>
+load_config(path: str) ForgeConfig
+prepare_dataset(path: str) object
+get_model_and_tokenizer(config: ForgeConfig) tuple
+run_benchmark(config: ForgeConfig) BenchmarkResult
+audit_dataset(source: str, options: dict) AuditReport
+verify_audit_log(path: str, require_hmac: bool) VerifyResult
+detect_pii(text: str) list
+mask_pii(text: str) str
+detect_secrets(text: str) list
+mask_secrets(text: str) str
+compute_simhash(text: str) int
}
class Versioning {
<<meta>>
+__version__ str
+__api_version__ str
}
forgelm o-- ForgeConfig
forgelm o-- ForgeTrainer
forgelm o-- TrainResult
forgelm o-- BenchmarkResult
forgelm o-- AuditReport
forgelm o-- VerifyResult
forgelm o-- AuditLogger
forgelm o-- WebhookNotifier
forgelm o-- LibraryFunctions
forgelm o-- Versioning
forgelm o-- ConfigError
ForgeTrainer --> ForgeConfig
ForgeTrainer --> TrainResult
LibraryFunctions --> ForgeConfig
LibraryFunctions --> TrainResult
LibraryFunctions --> BenchmarkResult
LibraryFunctions --> AuditReport
LibraryFunctions --> VerifyResult
AuditLogger --> AuditReport
Versioning --> forgelm
Flow diagram for Phase 19 ForgeLM library API implementation tasksflowchart TD
A["Start Phase 19<br/>Implement library API"] --> B["Add forgelm/py.typed<br/>(PEP 561 marker)"]
B --> C["Create forgelm/api.py<br/>with __api_version__"]
C --> D["Extend forgelm.__init__<br/>__getattr__ and __all__<br/>for new symbols"]
D --> E["Implement __dir__ for forgelm"]
E --> F["Port forgelm.cli.__init__<br/>to lazy __getattr__ facade"]
F --> G["Add tests/test_library_api.py<br/>(stable journeys + lazy imports)"]
G --> H["Add tests/test_cli_lazy_imports.py"]
H --> I["Wire mypy --strict CI step<br/>for public surface"]
I --> J["Write docs/reference/library_api.md"]
J --> K["Write docs/guides/library_api.md<br/>and example notebook"]
K --> L["Update README, CHANGELOG,<br/>release standards"]
L --> M["Run full test + lint matrix<br/>and complete Phase 19"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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 |
Review Summary by QodoPhase 18: Library API analysis & design document (design-only)
WalkthroughsDescription• Delivers Phase 18 design document pinning ForgeLM's library API surface • Establishes three stability tiers (stable/experimental/internal) with per-symbol mapping • Documents lazy-import invariant ensuring import forgelm does not load torch • Specifies type-checking contract (py.typed + mypy --strict on public surface) • Defines 16-row Phase 19 implementation task plan with acceptance criteria Diagramflowchart LR
A["Phase 18<br/>Design Document"] -->|"Pins API surface"| B["Stability Tiers<br/>Stable/Experimental/Internal"]
A -->|"Enforces lazy imports"| C["Lazy-Import<br/>Invariant"]
A -->|"Type safety"| D["py.typed +<br/>mypy --strict"]
A -->|"Versioning"| E["__api_version__<br/>vs __version__"]
A -->|"Implementation spec"| F["Phase 19<br/>Task Plan"]
B -->|"9 test cases"| G["Integration Tests"]
C -->|"Regression test"| G
D -->|"CI enforcement"| G
File Changes1. CHANGELOG.md
|
Code Review by Qodo
1.
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="CHANGELOG.md" line_range="27-29" />
<code_context>
+ `AuditReport`, `WebhookNotifier`, `detect_pii` / `mask_pii` / `detect_secrets` /
+ `mask_secrets` / `compute_simhash`); spells out the `py.typed` + `mypy --strict`
+ contract for the public surface; resolves the Wave 1 round-5 carry-over
+ (lazy `__getattr__` migration for `forgelm/cli/__init__.py`); spec'es the
+ `tests/test_library_api.py` integration suite Phase 19 must ship. No code
+ changes in this PR — design only; Phase 19 implements.
</code_context>
<issue_to_address>
**issue (typo):** The word "spec'es" looks like a typo; consider using "specs" or "specifies" instead.
Using a standard term here (e.g., “specifies” or “specs”) will read more clearly than the nonstandard contraction “spec’es,” which comes across as a typo.
```suggestion
contract for the public surface; resolves the Wave 1 round-5 carry-over
(lazy `__getattr__` migration for `forgelm/cli/__init__.py`); specifies the
`tests/test_library_api.py` integration suite Phase 19 must ship. No code
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Up to standards ✅🟢 Issues
|
![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 a comprehensive design document for the ForgeLM Library API, establishing stability tiers, a strict type contract, and a lazy-import discipline to ensure heavy ML dependencies are only loaded when needed. Feedback focuses on improving the implementation of the lazy-import mechanism by suggesting dictionary-based lookups and caching to enhance performance and maintainability. Additionally, it was noted that explicit type declarations within TYPE_CHECKING blocks are necessary to ensure downstream mypy --strict compatibility for lazy-loaded symbols.
|
|
||
| | Surface | Type-hint coverage required | | ||
| |---|---| | ||
| | Stable symbols (§2.1) | 100% — every parameter + return. `mypy --strict forgelm/__init__.py` clean. | |
There was a problem hiding this comment.
To achieve the goal of mypy --strict cleanliness for downstream consumers, the design should explicitly require that all lazy-loaded symbols are declared in forgelm/__init__.py using a if TYPE_CHECKING: block. Without these declarations, mypy will not be able to resolve the types of attributes accessed via __getattr__, leading to Any types or errors in consumer code.
Comment on lines
+162
to
+197
| def __getattr__(name: str): | ||
| if name == "audit_dataset": | ||
| from .data_audit import audit_dataset as _v | ||
| return _v | ||
| if name == "verify_audit_log": | ||
| from .compliance import verify_audit_log as _v | ||
| return _v | ||
| if name == "AuditLogger": | ||
| from .compliance import AuditLogger as _v | ||
| return _v | ||
| if name == "VerifyResult": | ||
| from .compliance import VerifyResult as _v | ||
| return _v | ||
| if name == "AuditReport": | ||
| from .data_audit import AuditReport as _v | ||
| return _v | ||
| if name == "WebhookNotifier": | ||
| from .webhook import WebhookNotifier as _v | ||
| return _v | ||
| if name == "detect_pii": | ||
| from .data_audit import detect_pii as _v | ||
| return _v | ||
| if name == "mask_pii": | ||
| from .data_audit import mask_pii as _v | ||
| return _v | ||
| if name == "detect_secrets": | ||
| from .data_audit import detect_secrets as _v | ||
| return _v | ||
| if name == "mask_secrets": | ||
| from .data_audit import mask_secrets as _v | ||
| return _v | ||
| if name == "compute_simhash": | ||
| from .data_audit import compute_simhash as _v | ||
| return _v | ||
| # ... existing entries (ForgeTrainer, get_model_and_tokenizer, ...) ... | ||
| raise AttributeError(name) |
There was a problem hiding this comment.
This if/elif chain is repetitive and less maintainable than the dictionary-based lookup proposed for the CLI in Section 4.3. Additionally, the implementation lacks caching; every access will re-trigger the import logic. It is recommended to refactor this to use a mapping dictionary and ensure that resolved symbols are set on the module (e.g., via globals()[name] = value) to optimize subsequent accesses.
Comment on lines
+224
to
+230
| def __getattr__(name: str): | ||
| target_module = _SYMBOL_TO_MODULE.get(name) | ||
| if target_module is None: | ||
| raise AttributeError(f"module 'forgelm.cli' has no attribute {name!r}") | ||
| import importlib | ||
| submodule = importlib.import_module(target_module, package=__name__) | ||
| return getattr(submodule, name) |
There was a problem hiding this comment.
The implementation of __getattr__ should cache the resolved attribute in the module's namespace. As noted in the "monkeypatch resolution" section (line 236), __getattr__ is only bypassed once the name is set on the module. The current sketch is missing the assignment step that would enable this bypass and improve performance for subsequent accesses.
Suggested change
| def __getattr__(name: str): | |
| target_module = _SYMBOL_TO_MODULE.get(name) | |
| if target_module is None: | |
| raise AttributeError(f"module 'forgelm.cli' has no attribute {name!r}") | |
| import importlib | |
| submodule = importlib.import_module(target_module, package=__name__) | |
| return getattr(submodule, name) | |
| def __getattr__(name: str): | |
| target_module = _SYMBOL_TO_MODULE.get(name) | |
| if target_module is None: | |
| raise AttributeError(f"module 'forgelm.cli' has no attribute {name!r}") | |
| import importlib | |
| submodule = importlib.import_module(target_module, package=__name__) | |
| value = getattr(submodule, name) | |
| globals()[name] = value | |
| return value |
3 tasks done
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Closes the three HIGH findings from the multi-agent review and the six MEDIUMs that have a clean doc-only fix: - F-23-01 (HIGH): rewrote §4.1 ¶2 to acknowledge the existing test_lazy_imports.py only covers forgelm.trainer / forgelm.model; bare `import forgelm` is verified manually today and Phase 19 task #7 closes the gap with a real subprocess regression test. - F-23-02 (HIGH): added a TYPE_CHECKING import block to the §4.2 __getattr__ sketch so mypy --strict sees real signatures (not Any), and replaced the if/elif chain with a _LAZY_SYMBOLS dict + caching via globals()[name] = value (gemini-code-assist comments addressed in the same edit: maintainability + per-access caching). - F-23-03 (HIGH): replaced the "raise AttributeError on forgelm._http" test with a stronger and import-side-effect-immune assertion ("_http" not in dir(forgelm) and not in __all__). Documented in §2.3 that internal = absent from public surface, not import-protected. - F-23-04 (MEDIUM): §12 sign-off note honestly acknowledges that no existing test imports the new stable names and Phase 19 task #7 ships the coverage; the table claim was aspirational. - F-23-05 (MEDIUM): §4.3 reframes the cli/__init__.py lazy migration as hygiene, not perf; explicitly notes _dispatch eagerly imports six submodules so a real CLI invocation defeats the laziness anyway. - F-23-06 (MEDIUM): §5.2 names the CI trigger (release-* branch push) and explicitly documents the script as a no-op for v0.5.5 because there is no prior __api_version__-bearing baseline. - F-23-07 (MEDIUM): added a tier-completeness audit table covering every top-level module + sub-package (Stable / Experimental / Internal) so reach-ins to forgelm.safety / forgelm.judge / forgelm.ingestion are not silently load-bearing. - F-23-08 (MEDIUM): replaced the §2.1 audit_dataset signature (`**kwargs` placeholder) and verify_audit_log signature (missing hmac_secret) with the real signatures audited from forgelm/data_audit/_orchestrator.py and forgelm/compliance.py. - BOT (sourcery): "spec'es" typo in CHANGELOG.md replaced with "specifies". Reviewer recommendation said "pre-Phase-19 must land before Phase 19 starts"; that's exactly what this commit does — design + Phase 19 implementation plan are now consistent with the audited live code. Closes (review IDs): F-23-01 .. F-23-08 + sourcery-typo Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
cemililik
added a commit
that referenced
this pull request
May 2, 2026
Closes the three CRITICAL findings + four HIGH findings from the multi-agent review and the bot comments that have a clean doc-only fix. CRITICAL fixes: - F-24-01 (CRITICAL): added §3.1 "Prior-state acknowledgement" that resolves the staging_ttl_days collision with the already-shipped EvaluationConfig.staging_ttl_days. Phase 21 deprecates the evaluation field via DeprecationWarning + alias-forward to the new retention.staging_ttl_days; removal targeted v0.7.0 per docs/standards/release.md cadence. - F-24-02 (CRITICAL): same §3.1 entry resolves the closure-plan §15.5 GH-23 conflict between top-level retention.* (this design) and ingestion.retention.raw_documents.ttl_days (legacy GDPR docs). Top-level retention.* wins; Phase 21 updates the user-manual gdpr.md page in the same PR; closure-plan GH-23 entry will be amended when Phase 21 ships. - F-24-03 (CRITICAL): added §5.4 "Personal-data minimisation across every audit-event field" — per-field classification + hash policy for category-3 (personal-data-bearing) fields. target_id in row mode is hashed via SHA-256(salt + value); error_message is routed through the existing _http._mask_secrets_in_text + PII regex mask helpers; everything else is genuinely operational. Phase 21 ships test_target_id_hashed_in_audit_event regression. HIGH fixes: - F-24-04 (HIGH): §4.2 flag table revised — line-number fallback for --row-id REFUSED; directory mode REFUSED; multi-row matches in a single file aborted unless --row-matches=all opt-in; new --yes flag required for unattended use (interactive without --yes prompts on TTY, exits EXIT_CONFIG_ERROR on non-TTY). - F-24-05 (HIGH): §4.3 exit codes aligned with the project-wide 0/1/2/3/4 contract; --check-policy returns code 3 (EXIT_EVAL_FAILURE) when violations exist (was: always 0); code 4 (EXIT_AWAITING_APPROVAL) for the interactive-confirm path. - F-24-06 (HIGH): §4.4 atomicity rewritten with strict event ordering (data.erasure_requested FIRST before any deletion, then rewrite, then data.erasure_completed). Step-5 and step-6 failure recovery paths spelled out with named regression test test_partial_commit_emits_failed_event. - F-24-08 (HIGH): §6 marketing-claim revision now carries an explicit scope-limitation paragraph naming what the operator remains responsible for (replicas, downstream consumers, webhook receivers, legal-hold copies). New data.erasure_warning_external_copies event fires when a webhook target is configured. MEDIUM fixes (selected high-value): - F-24-09 (MEDIUM): §3.2 audit_log_retention_days docstring rewritten with accurate Article 12(2) (6-month floor) + Article 11 (10-year high-risk) + GDPR Article 30 (≥3y) reasoning; default 1825d justified as middle-of-the-road horizon. - F-24-11 (MEDIUM): §5.1 audit-event catalog grew from 3 to 6 events — data.erasure_warning_memorisation, _warning_synthetic_data_present, _warning_external_copies are now properly specified (previously referenced 3× without spec). BOT fixes (gemini, qodo, sourcery): - gemini-mtime: §3.3 new section "mtime caveat (and the in-band age field)" — retention check prefers the audit-log timestamp where available (HMAC-signed, can't be touch'd), falls back to mtime with age_source notation in the JSON output. - qodo-line-count: §12 sign-off updated with actual line count (445 vs claimed ≈460); v2 additions enumerated. - sourcery-typo: "spec'es" → "specifies" in design intro + CHANGELOG. DEFERRED (not in this PR): - F-24-07 (HIGH) full §8 file-map enumeration (16 → 20 files): folded into Phase 21 implementation PR description per its closer scope. - F-24-10 (MEDIUM) acquire_run_lock spec + Windows fallback: still Phase 22 ISO/SOC 2 design scope per closure-plan §15.5 Round-5 carry-over; Phase 20 references it but does not specify it. - F-24-12 (MEDIUM) TR mirror file map: Phase 30 final doc sweep scope; Phase 21 ships EN side first. - F-24-13 (MEDIUM) distributed-corpus deletion: explicitly out of scope per §11; already documented. - F-24-14 (MEDIUM): --yes flag now in §4.2 table; no-tty behaviour spelled out. Closes (review IDs): F-24-01..06, 08, 09, 11 + gemini-mtime, qodo-line-count, sourcery-typo Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Collaborator
Author
Wave 2a Round-1 multi-agent review — fixes pushedAll CRITICAL + HIGH findings from the multi-agent review (and the relevant inline bot comments) have been addressed in the latest commit on this branch. See the commit message for the per-finding mapping (review IDs F-23-*, gemini bot IDs, qodo bot IDs, sourcery bot IDs). Per-PR test counts:
ruff format/check clean across all five PRs. Cross-PR XPR-01 concern (audit-log parser proliferation) closed in PR #25 via the new Items deferred to follow-up PRs (with rationale) are listed under DEFERRED in each commit message. @coderabbitai review |
This was referenced May 2, 2026
Contributor
|
Tip For best results, initiate chat on the files or code changes.
[review] |
8 tasks
Collaborator
Author
|
Superseded by #28 (consolidated Wave 2a integration). All commits from this branch are merged into |
cemililik
added a commit
that referenced
this pull request
Jun 15, 2026
…PR#63-review) verify_integrity iterated manifest['artifacts'] unchecked: null/int raised an uncaught TypeError (traceback, not exit 1) and str/dict false-passed as 'All 0 artifacts present' (exit 0). Add an explicit container guard returning valid=False (exit 1) rather than CodeRabbit's silent coercion. CodeRabbit #23. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
18 tasks
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
Design-only PR. Ships the Phase 18 deliverable from the closure plan: a 470-line analysis document (`docs/analysis/code_reviews/library-api-design-202605021414.md`) that pins down ForgeLM's library API surface so Phase 19 can implement it without re-deriving decisions.
What this resolves
ForgeLM today is a de facto library — `from forgelm import ForgeTrainer` works because of an existing `getattr` shim — but nothing has documented:
The design answers all of these. Five open questions resolved with explicit decisions (see §10).
Document structure
Test plan
Risk
None. Pure design document. No runtime / API / dependency surface touched.
Closes
Unblocks
🤖 Generated with Claude Code
Summary by Sourcery
Documentation: