Skip to content

test(jmle): require Rust-owned optimizer control - #717

Closed
seonghobae wants to merge 1 commit into
mainfrom
test/jmle-rust-optimizer-ownership-626
Closed

test(jmle): require Rust-owned optimizer control#717
seonghobae wants to merge 1 commit into
mainfrom
test/jmle-rust-optimizer-ownership-626

Conversation

@seonghobae

Copy link
Copy Markdown
Contributor

Production numerical-ownership gap

Advances the remaining JMLE portion of #626. Protected main d2301dccb7d39a794d08cc96ea8c858b9ff49125 still executes Adam state updates, L-BFGS two-loop recursion/backtracking, convergence control and optimizer-phase sequencing in python/fast_mlsirm/fit.py::_run_single_fit, _adam, _lbfgs, and _lbfgs_direction. The objective/gradient may be Rust-backed, but the optimizer itself remains a second Python production numerical owner.

Intentional fail-first boundary

Exact test-only head: 638632cfc01f839367d875bb98be7585e5e09442, directly based on current protected main.

tests/test_jmle_rust_optimizer_ownership.py exercises the installed public JMLE path for all three advertised optimizer modes (adam, lbfgs, adam_lbfgs) with backend="rust", replaces the legacy Python Adam and L-BFGS loops with fail-fast sentinels, and requires a successful Rust-backed fit without either Python optimizer being called.

Current production behavior is expected to fail at the intended optimizer boundary because _run_single_fit directly invokes _adam and/or _lbfgs. Setup/import/fixture/changelog failure is not valid RED; CI must build the Rust/PyO3 extension and reach these runtime assertions.

Minimum GREEN after exact RED proof

  1. Move JMLE optimizer state/update/convergence control into mlsirm-core: Adam moments/bias correction, gradient clipping, L-BFGS history/two-loop recursion, Armijo/backtracking, phase sequencing and restart ranking where they affect the fitted result.
  2. Expose the smallest stable PyO3 contract and reduce Python to bounded validation, deterministic seed/input marshalling, immutable result construction and reporting. Do not replace the Python loop with another Python/SciPy optimizer.
  3. Preserve the public optimizer identities, current model-specific active parameter blocks, deterministic seed/restart semantics, traces/statuses and objective/gradient estimand unless a separately reviewed scientific correction proves a defect.
  4. Add direct Rust↔public parity/property evidence plus realistic true-parameter recovery across representative JMLE model families using aligned bias/MAE/RMSE and convergence. Correlation alone is insufficient.
  5. Use bounded low-context-switch CPU execution; benchmark before any new GPU optimizer path. Existing GPU objective/gradient parity does not prove optimizer parity.
  6. Keep the NumPy/reference implementations test-only/non-production where retained. Missing/incompatible compiled capability must fail closed rather than selecting an independent Python optimizer.
  7. Add complete rustdoc/docstrings, primary-source/APA-7 doctoring for the actual optimization contract, authoritative changelog material after GREEN, exact changed-production statement/branch coverage, and one unchanged final head with all applicable Python/Rust/PyO3/package/reinstall/GPU-existing/fuzz/Security/SAST/current-head review gates.
  8. Canonical Architecture/TRD/ADR/UML/ERD/traceability remains owned only by docs: establish canonical product and architecture baseline #604/docs: close canonical documentation fitness gaps without parallel authority #621; update that line after the implementation stabilizes rather than creating parallel documentation authority.

Do not widen this first ownership slice into MMLE (already separately fail-closed), new estimator identities, model redesign, dependencies/workflows, package version/release or hosted persistence. Keep Draft until RED→GREEN history and exact-head gates are complete.

@coderabbitai

coderabbitai Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: ea1f9838-2ed9-4092-9e5a-23b3c812b205

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

Copy link
Copy Markdown
Contributor Author

@opencode-agent address

Exact bounded implementation handoff for Draft #717 / issue #626. Before any write, refetch source head 638632cfc01f839367d875bb98be7585e5e09442, protected main 9b7335466c1d61349228d33e7b1e58d279f984c4, comments/reviews and the fit.py/Rust optimizer target blobs; abort this branch if any source-affecting state moved or another writer appeared. Preserve the intentional RED history and reconcile protected main non-destructively first. Fresh compare is diverged: this branch has exactly one unique fail-first test path and is 31 protected-main commits behind.

Exact CI run 31477073508 reached the intended numerical-ownership boundary on both Python 3.12 and 3.14. Rust/PyO3, package/reinstall/release acceptance, GPU smoke, fuzz, Security Scan and SAST are green. Python 3.12 completed 3 failed, 3191 passed, 2 skipped; the only failures are the three new optimizer modes because public JMLE calls legacy Python _adam/_lbfgs. This is valid runtime RED, not setup/import/fixture failure. The unrelated marginal overflow warning remains #583/#564.

Implement the minimum GREEN only:

  1. Move result-affecting JMLE optimizer arithmetic/control into mlsirm-core: Adam first/second moments, bias correction, gradient clipping and update; L-BFGS history/two-loop recursion, curvature handling, line-search/Armijo/backtracking; and adam_lbfgs phase sequencing/convergence state needed to preserve the public result. Do not replace it with another Python/SciPy optimizer.
  2. Expose the smallest stable PyO3 contract. Python may validate inputs/config, initialize/marshal deterministic parameters, construct immutable results/traces and report only. Missing/incompatible Rust optimizer capability must fail closed for backend="rust" rather than selecting legacy Python numerical loops.
  3. Preserve public optimizer identities, deterministic seed/restart behavior, active parameter blocks/model semantics, gradient/objective estimand, clipping/search constants, statuses/traces and restart ranking unless a separate test-first scientific defect proves a change necessary.
  4. Add direct Rust/public parity and properties for adam, lbfgs, and adam_lbfgs, including deterministic replay, malformed controls, convergence/bounds and realistic true-parameter recovery with scale/alignment plus bias/MAE/RMSE; correlation alone is insufficient.
  5. Keep CPU execution bounded/low-context-switch. Do not add GPU optimizer semantics merely because objective/gradient GPU exists; profile first.
  6. Ground doctoring/equation traceability in primary optimization sources, including Kingma & Ba's Adam paper and the canonical limited-memory BFGS / Armijo line-search literature actually implemented. Record APA 7 references and distinguish implementation parity from psychometric validity.
  7. After focused GREEN, add authoritative changelog material and complete rustdoc/docstrings, then run focused JMLE ownership/recovery tests, Rust workspace/PyO3, Python 3.12/3.14, package/reinstall, existing GPU/fuzz, Security/SAST, changelog check and git diff --check. Leave Draft for fresh exact-head review and governance.

Do not widen into MMLE, new estimator names, model redesign, dependencies/workflows/version/release, hosted state or competing canonical documentation. Cross-cutting Architecture/TRD/ADR/UML/ERD/traceability belongs only in #604/#621 after the implementation stabilizes.

Copy link
Copy Markdown
Contributor Author

@opencode-agent address

Fresh replacement implementation lease for Draft #717 / issue #626 after protected-main movement invalidated the prior exact-base handoff. Immediately before writing, refetch source head, live main, comments/reviews, and target fit.py/Rust optimizer blobs; proceed only while source remains 638632cfc01f839367d875bb98be7585e5e09442, live protected main remains compatible with 198f236241955b5f707d140d4883ea481a2beca1, and no other writer targets this branch/paths. The branch is now diverged ahead 1 / behind 36 with exactly one unique fail-first test file; reconcile protected main non-destructively first. Preserve the validated RED history from CI 31477073508 rather than rewriting it.

Implement only the bounded JMLE Rust-optimizer ownership slice already defined by #626/#717: Rust owns Adam moments/bias correction/clipping/update, L-BFGS history/two-loop recursion/curvature/Armijo-backtracking, and adam_lbfgs phase/convergence control that determines the fitted result. Python may validate, seed/initialize/marshal, construct immutable results/traces, and report; it must not substitute SciPy or retain ordinary production _adam/_lbfgs execution for backend="rust". Missing/incompatible Rust optimizer capability fails closed.

Preserve public optimizer identities, current model-specific active blocks, deterministic seed/restart semantics, objective/gradient estimand, trace/status behavior and restart selection unless a separate test-first scientific defect proves a change. Add direct Rust↔public parity plus realistic aligned bias/MAE/RMSE/convergence evidence for all three optimizer modes. Ground doctoring in the actual primary Adam/L-BFGS/Armijo sources with APA 7 links/scope, add authoritative changelog material only after GREEN, then run focused ownership/recovery and full Python 3.12/3.14, Rust/PyO3, package/reinstall, existing GPU/fuzz, Security/SAST, coverage/docstring and current-head review gates on one unchanged head.

Do not widen into MMLE, new estimator names, model redesign, dependencies/workflows/version/release, hosted state, or canonical #604 docs. Stop writing if source/base/lease state moves.

Copy link
Copy Markdown
Contributor Author

@opencode-agent address

Fresh current-main replacement/reconciliation implementation lease for Draft #717 / issue #626. Immediately before any write, refetch exact source head 638632cfc01f839367d875bb98be7585e5e09442, live protected main 7b57f77747b9a9bd911660c76b1be0ca0dc2d145, comments/reviews/threads and target python/fast_mlsirm/fit.py / Rust optimizer / PyO3 blobs; abort if either ref moves or another writer targets this branch/paths. Every earlier handoff in this thread is stale because it was bound to predecessor protected heads.

Fresh compare is diverged, ahead 1 / behind 42 from merge base d2301dccb7d39a794d08cc96ea8c858b9ff49125; the only unique branch path is the validated fail-first tests/test_jmle_rust_optimizer_ownership.py. Preserve CI 31477073508 as RED history: Python 3.12 completed 3 failed, 3191 passed, 2 skipped, and the only failures are adam, lbfgs, and adam_lbfgs because the public Rust-backed JMLE route executes legacy Python _adam/_lbfgs; Rust/PyO3, package/reinstall/release acceptance, GPU, fuzz, Security Scan and SAST were GREEN.

Because this test-only branch is now 42 protected-main commits behind, first reconcile current main non-destructively. If normal reconciliation would make the one-test lineage unnecessarily conflict-prone, create exactly one clean-lineage replacement from current protected main, carry the exact fail-first ownership test and RED lineage, make it the sole #626 implementation line, then close #717 with precise supersession lineage. Never keep two optimizer writers active.

Implement only the bounded JMLE Rust-optimizer ownership slice:

  1. Rust owns Adam first/second moments, bias correction, clipping and parameter update; L-BFGS history/two-loop recursion, curvature handling and Armijo/backtracking; and adam_lbfgs phase/convergence control that determines the fitted result. Do not substitute SciPy or another Python optimizer.
  2. Python may validate, initialize/seed, marshal, construct immutable traces/results and report. For backend="rust", missing/incompatible compiled optimizer capability fails closed and never executes ordinary Python _adam/_lbfgs arithmetic.
  3. Preserve public optimizer identities, model-specific active blocks, deterministic seed/restart semantics, objective/gradient estimand, trace/status behavior and restart selection unless a separately reviewed test-first defect proves a correction.
  4. Add Rust unit/property tests and direct public↔Rust parity, plus realistic aligned true-parameter bias/MAE/RMSE and convergence evidence for all three modes; correlation alone is insufficient.
  5. Use bounded low-context-switch CPU execution. Benchmark before any GPU optimizer path; existing GPU objective/gradient parity does not prove optimizer parity.
  6. Add complete rustdoc/docstrings, primary Adam/L-BFGS/Armijo APA 7 doctoring and authoritative changelog material only after GREEN. Then run focused ownership/recovery, exact changed-production 100% statement/branch coverage and full Python 3.12/3.14, Rust/PyO3, package/reinstall, existing GPU/fuzz, Security/SAST and current-head review gates on one unchanged head.
  7. Do not widen into MMLE, new estimator names, model redesign, dependencies/workflows/version/release, hosted state or canonical docs: establish canonical product and architecture baseline #604/docs: close canonical documentation fitness gaps without parallel authority #621 docs.

Stop writing immediately if source/base/lease state moves.

Copy link
Copy Markdown
Contributor Author

Superseded by clean-lineage Draft #738. Fresh verification shows #717 is still open on exact head 638632cfc01f839367d875bb98be7585e5e09442 from protected-base d2301dcc..., while current protected main is 7b57f77747b9a9bd911660c76b1be0ca0dc2d145. #717 has exactly one unique path, tests/test_jmle_rust_optimizer_ownership.py; that fail-first contract was preserved verbatim on #738, which starts directly from current protected main. No checks, reviews, approvals, mergeability or writer authority transfer from #717 to #738. Closing this stale implementation path to preserve one current-lineage writer authority for issue #626.

@seonghobae seonghobae closed this Aug 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant