fix(fitstats): fail closed without Rust for chi2_sf and BH - #723
Conversation
Require the compiled core for public chi2_sf and benjamini_hochberg so production numerical ownership cannot silently fall back to pure Python. Update coverage stubs that only need fit-stat entrypoints for unrelated numpy-fallback paths.
|
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:
📝 WalkthroughWalkthroughThe change makes public ChangesCompiled Rust fit-statistics requirement
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 296-299: Update the “Fit-statistics tail ownership” changelog
entry to state that public chi2_sf and benjamini_hochberg require the compiled
Rust core and fail closed when it is unavailable; remove the claim about a
pure-Python fallback.
In `@docs/doctoring/fitstats_rust_required.md`:
- Around line 9-11: Add stable links and concise source summaries for the cited
AERA/APA/NCME Standards and Benjamini–Hochberg literature in
docs/doctoring/fitstats_rust_required.md; where redistribution is permitted,
include the relevant paper PDFs instead. Preserve the full citations and explain
how each source supports the contract.
In `@python/fast_mlsirm/fitstats.py`:
- Around line 214-217: Update the fit-statistics entrypoint checks in
python/fast_mlsirm/fitstats.py lines 214-217 and 226-231 to retrieve chi2_sf and
benjamini_hochberg respectively, requiring each retrieved entrypoint to be
callable before invoking it; otherwise raise the existing capability
RuntimeError. In tests/test_fitstats_rust_required.py lines 27-38, add a core
sentinel whose entrypoints are non-callable and assert the same capability
error.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4060039e-819b-4a0c-a044-73ac9636e494
📒 Files selected for processing (6)
CHANGELOG.mddocs/changelog.d/720-fitstats-rust-required.mddocs/doctoring/fitstats_rust_required.mdpython/fast_mlsirm/fitstats.pytests/test_cov_c_fitstats.pytests/test_fitstats_rust_required.py
| #### Fit-statistics tail ownership | ||
|
|
||
| - Public `chi2_sf` and `benjamini_hochberg` now prefer the Rust core for ranking and tail arithmetic, with a pure-Python fallback only when the compiled core methods are unavailable. | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Correct the fit-statistics fallback description.
Line 298 still documents a pure-Python fallback. python/fast_mlsirm/fitstats.py:208-229 now raises RuntimeError("fit statistics require the compiled Rust core") when the Rust core or entrypoint is unavailable. Update this entry to state that both APIs require the compiled Rust core and fail closed.
Proposed wording
- Public `chi2_sf` and `benjamini_hochberg` now prefer the Rust core for ranking and tail arithmetic, with a pure-Python fallback only when the compiled core methods are unavailable.
+ Public `chi2_sf` and `benjamini_hochberg` require the compiled Rust core for ranking and tail arithmetic and fail closed when the core methods are unavailable.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #### Fit-statistics tail ownership | |
| - Public `chi2_sf` and `benjamini_hochberg` now prefer the Rust core for ranking and tail arithmetic, with a pure-Python fallback only when the compiled core methods are unavailable. | |
| #### Fit-statistics tail ownership | |
| - Public `chi2_sf` and `benjamini_hochberg` require the compiled Rust core for ranking and tail arithmetic and fail closed when the core methods are unavailable. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CHANGELOG.md` around lines 296 - 299, Update the “Fit-statistics tail
ownership” changelog entry to state that public chi2_sf and benjamini_hochberg
require the compiled Rust core and fail closed when it is unavailable; remove
the claim about a pure-Python fallback.
| American Educational Research Association, American Psychological Association, & National Council on Measurement in Education. (2014). *Standards for educational and psychological testing*. American Educational Research Association. | ||
|
|
||
| Benjamini, Y., & Hochberg, Y. (1995). Controlling the false discovery rate: A practical and powerful approach to multiple testing. *Journal of the Royal Statistical Society: Series B, 57*(1), 289–300. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add links and source summaries for the cited literature.
This document has citations only. Add permitted paper PDFs, or add a stable link and a short summary of the support each source provides for this contract.
As per coding guidelines, “Substantive feature or process PRs should include relevant academic paper PDFs with full citations when redistribution is permissible; otherwise include a citation, link, and summary.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/doctoring/fitstats_rust_required.md` around lines 9 - 11, Add stable
links and concise source summaries for the cited AERA/APA/NCME Standards and
Benjamini–Hochberg literature in docs/doctoring/fitstats_rust_required.md; where
redistribution is permitted, include the relevant paper PDFs instead. Preserve
the full citations and explain how each source supports the contract.
Source: Coding guidelines
| core = _core_module() | ||
| if core is not None and hasattr(core, "chi2_sf"): | ||
| return float(core.chi2_sf(float(x), float(df))) | ||
| if df <= 0: | ||
| return float("nan") | ||
| return _gammainc_upper_reg(df / 2.0, max(x, 0.0) / 2.0) | ||
| if core is None or not hasattr(core, "chi2_sf"): | ||
| raise RuntimeError("fit statistics require the compiled Rust core") | ||
| return float(core.chi2_sf(float(x), float(df))) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Treat non-callable core attributes as unavailable.
hasattr accepts chi2_sf = None or benjamini_hochberg = None. The subsequent call raises TypeError instead of the stable capability RuntimeError.
python/fast_mlsirm/fitstats.py#L214-L217: retrievechi2_sfand requirecallable(entrypoint).python/fast_mlsirm/fitstats.py#L226-L231: retrievebenjamini_hochbergand requirecallable(entrypoint).tests/test_fitstats_rust_required.py#L27-L38: add a core sentinel with non-callable entrypoints and assert the same capability error.
Proposed fix
- if core is None or not hasattr(core, "chi2_sf"):
+ entrypoint = None if core is None else getattr(core, "chi2_sf", None)
+ if not callable(entrypoint):
raise RuntimeError("fit statistics require the compiled Rust core")
- return float(core.chi2_sf(float(x), float(df)))
+ return float(entrypoint(float(x), float(df)))📍 Affects 2 files
python/fast_mlsirm/fitstats.py#L214-L217(this comment)python/fast_mlsirm/fitstats.py#L226-L231tests/test_fitstats_rust_required.py#L27-L38
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@python/fast_mlsirm/fitstats.py` around lines 214 - 217, Update the
fit-statistics entrypoint checks in python/fast_mlsirm/fitstats.py lines 214-217
and 226-231 to retrieve chi2_sf and benjamini_hochberg respectively, requiring
each retrieved entrypoint to be callable before invoking it; otherwise raise the
existing capability RuntimeError. In tests/test_fitstats_rust_required.py lines
27-38, add a core sentinel whose entrypoints are non-callable and assert the
same capability error.
Partial/incompatible extension objects must not fall into AttributeError on optional Rust paths or skip fail-closed guards; require hasattr for each entrypoint before dispatch.
|
@opencode-agent address Exact-current CI RCA and bounded repair request for PR #723. Before any write, refetch source head CI run Minimum remedy only: preserve the S-X² native-vs-reference parity purpose without re-enabling production numerical fallback. In that test, replace the Acceptance: focused failing test GREEN on both semantics; |
Public chi2_sf/BH fail closed when the core is missing; the extreme-probability parity contract must keep tail helpers available while omitting s_x2_stat so the NumPy statistic path remains reachable.
67bb56e to
baa8cfe
Compare
Resolve CHANGELOG fragment ordering after #723 fitstats fail-closed landed.
1 participant
Why
Advances #720. After #646, public
chi2_sf/benjamini_hochbergstill silently re-ran pure Python when_core_module()was missing. Production numerical ownership must fail closed.What
RuntimeError("fit statistics require the compiled Rust core")when the core is absent or lacks the entrypoints.Relation
Supersedes draft #720 once this lands (close as absorbed).
Summary by CodeRabbit
Bug Fixes
Documentation
Tests