Skip to content

refactor(jmle): replace stale Rust optimizer ownership branch - #738

Closed
seonghobae wants to merge 1 commit into
mainfrom
replacement/jmle-rust-optimizer-main-20260811
Closed

refactor(jmle): replace stale Rust optimizer ownership branch#738
seonghobae wants to merge 1 commit into
mainfrom
replacement/jmle-rust-optimizer-main-20260811

Conversation

@seonghobae

Copy link
Copy Markdown
Contributor

Controlled clean-lineage replacement for stale Draft #717 / issue #626

This Draft starts directly from protected main 7b57f77747b9a9bd911660c76b1be0ca0dc2d145 and preserves the only unique fail-first contract from stale Draft #717 without transferring any predecessor checks, reviews, approvals, mergeability, or base assumptions.

Intentional current-main RED contract

The first commit changes only tests/test_jmle_rust_optimizer_ownership.py. It requires installed public JMLE fitting under backend="rust" to complete for all advertised optimizer identities (adam, lbfgs, adam_lbfgs) while fail-fast sentinels replace the legacy Python _adam and _lbfgs loops.

Valid RED must build/import the actual Rust/PyO3 package and reach the optimizer ownership boundary. Setup/import/fixture/changelog failure is not acceptance evidence. The predecessor #717 runtime RED was useful historical evidence only; this replacement must recreate exact-current-main evidence.

Minimum GREEN

  1. Move result-affecting JMLE optimizer arithmetic and control into mlsirm-core: Adam moments/bias correction/gradient clipping/update; L-BFGS history/two-loop recursion/curvature and line-search/backtracking; and adam_lbfgs phase sequencing/convergence state needed to preserve the public result.
  2. Expose the thinnest stable PyO3 contract. Python remains bounded validation, deterministic input/seed marshalling, immutable result construction and reporting only. Missing/incompatible Rust optimizer capability must fail closed rather than select legacy Python numerical loops.
  3. Preserve public optimizer names, deterministic seed/restart behavior, active parameter blocks/model semantics, objective/gradient estimand, clipping/search constants, statuses/traces and restart ranking unless a separate test-first scientific defect justifies a change.
  4. Add direct Rust/public parity and realistic true-parameter recovery for all three optimizer modes using aligned bias/MAE/RMSE and convergence evidence; correlation alone is insufficient.
  5. Keep CPU execution bounded and low-context-switch; do not add GPU optimizer semantics without measured benefit and end-to-end parity.
  6. Add complete Rust/Python docs and APA 7 equation/source doctoring for the actual Adam/L-BFGS/line-search implementation, plus authoritative changelog material after GREEN.
  7. Require one unchanged final head with focused ownership/recovery tests, exact changed-production statement/branch coverage, Python 3.12/3.14, Rust/PyO3, package/reinstall, existing GPU/fuzz, Security Scan, SAST, changelog and current-head review/governance gates.

Boundaries

Do not widen into MMLE, new estimator names, model redesign, dependencies/workflows/version/release, hosted state, or a parallel canonical architecture branch. Cross-cutting PRD/TRD/ADR/UML/ERD/traceability remains solely owned by #604/#621 after implementation stabilizes.

Advances #626. Supersedes stale Draft #717 as the sole implementation authority for this bounded JMLE optimizer-ownership slice.

@coderabbitai

coderabbitai Bot commented Aug 11, 2026

Copy link
Copy Markdown

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: ea421194-4d2d-458f-a56b-8280b3aa5a79

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-current GREEN implementation handoff for Draft #738 / issue #626. Before any write, refetch the source head and live protected base and abort if either moves or another writer targets the same JMLE optimizer paths. Proceed only while source head remains ab2d627ebd2fe60bca2365f93c8561739aaebbc8 and protected main remains 7b57f77747b9a9bd911660c76b1be0ca0dc2d145. Preserve this clean-lineage fail-first history; no force push, destructive rebase, temporary workflow or competing documentation branch.

The current-main RED is independently proven. CI run 31505692746 checked out synthetic merge 83a24853390ba484824cb5d8c89e0df38b99e6e8. Python 3.12 job 93826576184 built/installed the actual Rust/PyO3 package, verified Rust as the resolved default backend, and ran the full suite to exactly 3 failed, 3282 passed, 2 skipped. The only failures are tests/test_jmle_rust_optimizer_ownership.py for adam, lbfgs, and adam_lbfgs: python/fast_mlsirm/fit.py reaches _adam / _lbfgs and the sentinels prove public Rust-backed JMLE still executes Python optimizer arithmetic. Rust/PyO3, package/reinstall/release acceptance, GPU smoke, fuzz, Security Scan and SAST are GREEN on the same exact head. The existing marginal.py::_log_sigmoid warning belongs to #583/#564 and is out of scope.

Implement the minimum GREEN only:

  1. Move result-affecting JMLE optimizer numerical ownership/control into mlsirm-core: Adam moments, bias correction, gradient clipping and update; L-BFGS history, two-loop recursion, curvature handling and line-search/backtracking; and adam_lbfgs phase sequencing/convergence state required to preserve the public result.
  2. Expose the thinnest stable PyO3 entrypoint/result transport. Python may validate inputs/configuration, marshal deterministic seeds/restarts/arrays and construct/report immutable public results, but must not retain independent production _adam/_lbfgs arithmetic as a fallback. Missing/incompatible Rust optimizer capability must fail closed.
  3. Preserve public optimizer identities (adam, lbfgs, adam_lbfgs), active model/parameter blocks, objective/gradient estimand, deterministic seed/restart behavior, clipping/search constants, trace/status semantics and restart ranking unless a separate fail-first scientific defect proves a change is required.
  4. Add focused Rust unit/integration tests plus Python direct-delegation/parity, malformed/error and immutability cases. Add realistic true-parameter recovery across all three optimizer modes with aligned bias/MAE/RMSE and convergence evidence; correlation alone is not acceptance.
  5. Keep this CPU-f64 and bounded/low-context-switch. Do not add GPU optimizer semantics without a separately measured and parity-verified need.
  6. After focused GREEN, add complete Rust/Python documentation plus APA 7 equation/source doctoring for the actual Adam/L-BFGS/line-search implementation and an authoritative docs/changelog.d fragment; render/check managed CHANGELOG.md deterministically.
  7. Before Ready, require one unchanged final head with focused ownership/recovery tests, meaningful exact changed-production statement/branch coverage and public rustdoc/docstrings, full Python 3.12/3.14, Rust/PyO3, package/reinstall, existing GPU/fuzz, Security Scan, SAST, changelog and current-head review/governance gates.

Do not widen into MMLE, new estimator/model semantics, provider/LLM code, hosted persistence, dependencies/workflows/version/release, or cross-cutting canonical PRD/TRD/ADR/UML/ERD; that documentation remains owned only by #604/#621. Keep Draft while any gate remains.

Copy link
Copy Markdown
Contributor Author

@opencode-agent address

The prior exact-current GREEN handoff 5255166966 is stale because protected main advanced from 7b57f77747b9a9bd911660c76b1be0ca0dc2d145 to 914127ba227d3e02d0564aeeb4f27d76137610f9 when #733 merged. That old handoff explicitly required abort on base movement; do not continue from predecessor-base assumptions or transfer its synthetic-merge/check evidence.

Fresh lease/evidence boundary for Draft #738 / issue #626: source head is still ab2d627ebd2fe60bca2365f93c8561739aaebbc8; live protected main is 914127ba227d3e02d0564aeeb4f27d76137610f9. Before writing, refetch both and abort if either moves or another writer targets the JMLE optimizer paths.

First reconcile current protected main non-destructively while preserving the intentional fail-first ownership test and all accepted #733 behavior. Then recreate the runtime RED on the reconciled exact head; setup/import/changelog failure is not valid evidence. If the same three optimizer ownership failures remain, proceed with the already bounded minimum GREEN: move Adam/L-BFGS/adam_lbfgs result-affecting optimizer arithmetic/control into mlsirm-core, expose the thinnest PyO3 result transport, leave Python validation/marshalling/reporting only, fail closed without compatible Rust capability, preserve optimizer/model/seed/restart/objective/trace semantics, add direct parity plus aligned bias/MAE/RMSE/convergence recovery, keep CPU-f64 unless later profiling justifies GPU, then add doctoring/changelog and require one unchanged final head with full 3.12/3.14, Rust/PyO3, package/reinstall, GPU-existing, fuzz, Security, SAST, coverage/docstrings and current-head review gates.

Keep Draft. Do not widen into MMLE, providers, dependencies/workflows/version/release, hosted state, or canonical #604 documentation.

Copy link
Copy Markdown
Contributor Author

Superseded by clean current-main Draft #744. Exact verification before closure: #738's only changed path is tests/test_jmle_rust_optimizer_ownership.py, and #744 preserves that file byte-for-byte/content-addressed with the identical blob SHA 5f4512d6e2428b74010029956819fa0787b69cdc while starting from protected main 933ce6cdfedbbf92b30e73ed25f0ef0d926357f2. No checks, reviews, approvals, mergeability, or predecessor-base evidence transfer. Continue issue #626 only through #744 (or an explicitly reviewed successor).

@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