Repository navigation
fix(backend): isolate NumPy parity behind reference API - #1070
Conversation
|
Current-head review request for 833a0801c7f22cf61e6ca5f23fb4a8a3fa95c4f8. Please review this exact head: This is the scoped #626 contract slice: production configuration and CLI cannot select NumPy; parity remains available only through the explicit |
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughRust/PyO3 is now the required production numerical backend. NumPy fitting is available only through explicit ChangesBackend ownership boundary
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: 🟠 High · up to The change is intended to keep production fitting Rust-owned and fail closed, but the current implementation still allows an in-process caller to bypass that boundary and run NumPy through the normal fitting entry point. This could cause production behavior to violate the documented backend contract, so the public entry point should be corrected before merging. Sequence Diagram(s)sequenceDiagram
participant Caller
participant fit_reference
participant fit
participant BackendResolver
participant NumPy
Caller->>fit_reference: submit reference fitting inputs
fit_reference->>fit: delegate with NumPy scope active
fit->>BackendResolver: resolve scoped reference backend
BackendResolver-->>fit: numpy
fit->>NumPy: execute reference fitting
NumPy-->>Caller: return FitResult
sequenceDiagram
participant CLI
participant fit_reference
participant fit
CLI->>CLI: parse --reference
alt --reference enabled
CLI->>fit_reference: execute NumPy reference fitting
else production mode
CLI->>fit: execute Rust-owned fitting
end
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Current-head update: a7b23737d737d465fe29aca333a97ef7f087100b adds a fail-closed CLI guard so |
|
Current-head correction: ea9c7598e4d2f4d26b28f7fae0958f6f0860b8a adds the required level-one title and supported |
|
Correction: the exact current post-fix head is ea9c759. The changelog title/section fix and focused 8/8 contract proof are bound to this SHA; please disregard the previously mistyped long SHA in my comment. |
|
Current-head update: e5e8ed7d6eac9a38930b3e5c20e85ace1a901a98 adds the parser regression proving |
|
Current-head documentation alignment: fba03af6f0d78c6afc04f78edb425bd060e6ef92 updates PRD/ADR/doctoring and code comments so NumPy is described as explicit reference only; stale implicit-fallback language is removed from current contract docs. Focused changelog/backend/config/CLI tests (72 passed, 1 deselected) and Ruff pass. Review this exact head; earlier e5e8ed7 evidence is superseded. @opencode-agent review @cwl-noema-review review |
|
Current-head semantic guard: 6e76ee8a4e649490a29991bdc92cb73cf586f2f adds a regression that rejects the Rust-only plain unidimensional MMLE path from |
|
Correction: the exact current head is 6e76ee8. The previous comment contained a mistyped long SHA; disregard it. All semantic-guard evidence is attached to this corrected current SHA. |
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
Blocking contract finding on exact head 6e76ee8a1b781664ae692b55633e9c7f1aa6e819: the new public fit() signature exposes _allow_reference_backend: bool = False. Any caller can therefore invoke ordinary fast_mlsirm.fit(..., FitConfig(backend="numpy"), _allow_reference_backend=True) and select the NumPy reference owner through the production API, contradicting this PR's stated invariant that NumPy is reachable only through the explicitly named fit_reference surface. Please remove the bypass flag from the public fit signature and route both public entry points through a private implementation/helper instead. Add a regression that proves ordinary fit() has no reference-enabling keyword (e.g. the bypass call is rejected before data/native work) while fit_reference() retains the intended parity path. The new plain-unidimensional MMLE reference rejection is compatible with that refactor.
Pull request was converted to draft
|
Current exact head Keep this PR Draft for one remaining contract blocker discovered on the current source: public |
|
@opencode-agent Please perform a review-only formal review of exact current PR head |
|
Manual current-head review completed for |
|
@opencode-agent @cwl-noema-review please review exact current head |
|
Reviewed exact HEAD e47f9e6 with CodeGraph. The public Rust/auto backend boundary and explicit fit_reference authority are covered by 25 backend/reference/release tests plus 82 configuration/CLI/reference tests; focused tests, compile checks, and diff checks passed. Hosted CI, Rust, GPU smoke, security, CodeQL, OpenCode, Noema, Strix, and coverage checks are green at this HEAD. The remaining repository merge state is formal approval required; no bypass used. |
|
Rechecked current PR head No additional edit is needed: the package public export Exact-head proof: |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
opencode-agent
Bot
added
area: api
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
_fit_mmle() gained a required backend positional parameter earlier on this branch (threaded through into FitResult for exact resolved-backend reporting), but two direct-call tests weren't updated to pass it, failing closed CI with "missing 1 required positional argument: 'backend'" rather than exercising their intended Rust-fail-closed assertion. Both call the Rust-required path, so "rust" is the correct value (the RuntimeError fires before backend is read either way). test_cli_plain_mmle_backend.py's CLI test writes --out under tmp_path without confining the process cwd to it first, so cli.py's _confine_cli_paths correctly rejected the output path as outside the current working directory. Added the same monkeypatch.chdir(tmp_path) pattern already established by test_cli.py's autouse _cli_workspace fixture.
Pin the baseline to protected main@9c12eab1 (v0.9.0). The open-PR queue collapsed from 74 to 6 since the previous snapshot; replace the stale PR table with the current six lanes (#1363, #1345, #1279, #1029, #1005, #998) including their merge-forward state. Update gap-matrix evidence columns for landed work: automatic Rust backend (#951/#1070), crossed multiple-membership (#1014), the Figma design-boundary ADR now at docs/adr/0016 via merged #1130, and cross-engine conformance (#1082) after #1077 closed as completed. No production code, formula, estimator, or release surface changed.
1 participant
What changed
FitConfigand CLI backend choices Rust-owned (rust/auto) and fail closed when Rust is unavailable.fast_mlsirm.fit_referenceandfit --referenceparity surfaces for NumPy.Why
Closes the remaining #626 contract gap without silently breaking parity callers: NumPy remains callable only through a named reference path, while ordinary production configuration cannot select it.
Validation
python -m pytest -q tests/test_backend.py tests/test_cov_b_backend_config.py tests/test_config.py tests/test_cli.py tests/test_cov_f_fit.py tests/test_full_information_factor_stability.py tests/test_irt_contract.py tests/test_security_hardening.pytests/test_paper_features.py: 762 passedruff checkon changed production and focused test filesSummary by CodeRabbit
New Features
fit_referenceAPI and--referenceCLI mode for NumPy parity and research workflows.rustorautobackend options.Bug Fixes