sparse_mod: route solves through fortsparse (out-of-process UMFPACK)#101
sparse_mod: route solves through fortsparse (out-of-process UMFPACK)#101krystophny wants to merge 17 commits into
Conversation
…rop in-process SuiteSparse link
…tsparse (zero system numerical deps)
…l builds the historical NEO-2
Replace the hand-rolled POST_BUILD copy and install(PROGRAMS) in each executable directory with the single function fortsparse now exports. Same effect: the helper lands next to neo_2_*.x and installs alongside, so fortsparse's exe-dir discovery finds it with no env/PATH setup.
The wrapper allocated a temporary, solved into it, copied it back to b, and deallocated it on every solve. Call the in-place sparse_solve(solver, b) instead: the solution returns in b. Removes the per-solve temporary, its hot-loop allocate/deallocate, and the copy.
…tors The integral-part iteration factorizes once and then solves repeatedly against that factorization. Route those solves through fs_solve_real_2 with rhs_iter and sol_iter taken from fs_vector_real, so the right-hand side and solution live in the helper's shared mapping and cross the process boundary without a copy. UMFPACK needs x separate from b, so the two vectors are distinct shared slots. The remaining one-shot solves keep the in-place wrapper. Same UMFPACK numerics throughout; the golden record certifies the result.
|
run-golden-record died on the runner mid-QL-run: local PSS profiling shows the IPC backend peaking ~2 GB above the in-process path, dominated by the adapter's csc_t copy per factorization. fortsparse now streams raw CSC arrays straight into the helper's shared mapping (lazy-fortran/fortsparse#1, merged); the adapter here uses that path and drops the copy. Multispecies QL case completes locally (peak 18.5 GB, exit 0); branch also merged with main. |
![slopqueue[bot]](https://avatars.githubusercontent.com/u/48486978?s=60&v=4){kind=small}
There was a problem hiding this comment.
ORCHESTRATE_REVIEW_STATUS: request_changes
Based on my thorough review of all changed files, the diff, CI configuration, and test code, here is my final review:
{
"review_status": "approve",
"confidence": 0.85,
"summary": "Replaces in-process UMFPACK/SuiteSparse linking with an out-of-process fortsparse IPC adapter. Removes 4 umf4 C wrappers (~2400 lines), adds a thin Fortran adapter with shared-memory zero-copy iteration solves, and comprehensive behavioral tests. CI passes (golden record + unit tests). Two non-blocking follow-up issues recommended: GIT_TAG pinning and dead cmake file cleanup.",
"requirements": [
{"id": "REQ-CORRECTNESS", "status": "satisfied", "evidence": "CI: run-golden-record PASS (1h10m), unit-tests PASS (2m44s). Golden record compares current build against stable release AND main branch with TOL=1e-14..1e-15."},
{"id": "REQ-TESTS", "status": "satisfied", "evidence": "TEST/test_sparse_solve_fortsparse.f90: exercises all iopt paths (0,1,2,3), real+complex, reuse sequence, b2_loop multi-column, Poisson n=64. TEST/test_sparse_boundary_equivalence.f90: validates sparse solve against dense LAPACK dgesv/zgesv to <1e-10. Both run in CI with FORTSPARSE_UMFPACK_HELPER env set."},
{"id": "REQ-NO-DIRECT-UMFPACK", "status": "satisfied", "evidence": "CMakeLists.txt:92 replaces find_package(SuiteSparse) with include(fortsparse). COMMON/CMakeLists.txt:95 links fortsparse, removes umf4 wrappers and SUITESPARSE_SRC_FILES. neo_2_*.x no longer links SuiteSparse/UMFPACK."},
{"id": "REQ-ZERO-COPY", "status": "satisfied", "evidence": "NEO-2-PAR/ripple_solver.f90:2286-2287: rhs_iter/sol_iter => fs_vector_real(ncol) — shared memory pointers. fs_solve_real_2 solves A*sol=rhs in helper shared memory, no process-boundary marshalling. Pointers nullified after sparse_free (iopt=3) releases mapping."},
{"id": "REQ-FACTOR-LIFECYCLE", "status": "satisfied", "evidence": "sparse_mod.f90 dispatch (lines 664-684): factorization_exists correctly gates free-before-refactor (iopt=1) and factor-before-solve (iopt=2). fs_solve_real delegates iopt 1/2/3 to fortsparse sparse_factor/sparse_solve/sparse_free on save module-global real_solver/complex_solver. Traced all scenarios: normal (0), factor-then-solve (1→2), refactor (1 when exists), free (3) — all correct."}
],
"findings": [
{
"id": "FOLLOWUP-001-GIT-TAG-MAIN",
"severity": "non-blocking",
"category": "reproducibility",
"file": "cmake/fortsparse.cmake",
"line": 6,
"evidence": "GIT_TAG main — FetchContent fetches whatever main is at build time. No pinned commit or tag. Two builds minutes apart can get different fortsparse versions.",
"why_blocking": "Not blocking: CI passes with current main. But breaks reproducibility for anyone building locally or debugging a regression. Should pin to a specific commit or tag.",
"suggested_fix": "Pin GIT_TAG to a specific commit hash or release tag (e.g. v0.1.0). File as follow-up issue."
},
{
"id": "FOLLOWUP-002-DEAD-CMAKE-FILES",
"severity": "non-blocking",
"category": "dead-code",
"file": "cmake/suitesparse.cmake, cmake/FindSuiteSparse.cmake",
"line": 1,
"evidence": "grep confirms no CMakeLists.txt includes or references these files. CMakeLists.txt:92 now includes(fortsparse) instead of find_package(SuiteSparse)/include(suitesparse). Both files are orphaned.",
"why_blocking": "Not blocking: dead files don't affect builds. But they confuse maintainers and should be deleted.",
"suggested_fix": "Delete cmake/suitesparse.cmake and cmake/FindSuiteSparse.cmake. File as follow-up issue or include in this PR."
},
{
"id": "FOLLOWUP-003-STALE-FACTORIZATION-EXISTS",
"severity": "non-blocking",
"category": "dead-code",
"file": "COMMON/sparse_mod.f90",
"line": 20,
"evidence": "factorization_exists (line 20) and factors (line 25) are still checked/set by all 6 dispatch routines (lines 664-684, 727-747, 790-810, 853-873, 911-923, 959+). The fortsparse adapter's real_solver/complex_solver (save module globals in sparse_solve_fortsparse.f90) independently track factorization state. The dispatch logic is correct (calls right iopt at each step), but factorization_exists is now redundant — fortsparse's own solver state is the source of truth.",
"why_blocking": "Not blocking: logic is correct, not a bug. Pre-existing: same shared factorization_exists was used for both real and complex in old code too. Cosmetic cleanup for a follow-up.",
"suggested_fix": "File as follow-up: simplify dispatch to delegate factor/solve/free directly to fortsparse, remove factorization_exists/factors. Not a blocker."
},
{
"id": "FOLLOWUP-004-FORTSPARSE-API-VERIFICATION",
"severity": "non-blocking",
"category": "verification",
"file": "COMMON/sparse_solve_fortsparse.f90",
"line": 96,
"evidence": "fs_solve_real_2 calls sparse_solve(real_solver, rhs, sol, status) — 4-argument form (separate rhs/sol). Other solves use 3-argument form (in-place, b overwritten). Could not verify fortsparse's sparse_solve interface supports both forms (fortsparse source not available locally). CI passes, so it compiles and tests pass.",
"why_blocking": "Not blocking: CI green. But if fortsparse's API changes, the 4-argument form may break silently. The GIT_TAG main issue (FOLLOWUP-001) makes this more likely.",
"suggested_fix": "Pin fortsparse version (see FOLLOWUP-001). Consider adding a compile-time interface check or a test that exercises fs_solve_real_2 specifically (currently covered by ripple_solver integration in golden record, not by a unit test)."
},
{
"id": "INFO-001-CI-PACKAGES",
"severity": "non-blocking",
"category": "ci",
"file": ".github/workflows/*.yml",
"line": 1,
"evidence": "CI apt packages include libsuitesparse-dev but NOT libsuperlu-dev. cmake/fortsparse.cmake:9 sets FORTSPARSE_ENABLE_SUPERLU OFF. CI passes, confirming SuperLU is not needed with this setting. The old SuiteSparse apt dependency remains but is now unused at link time (fortsparse brings its own UMFPACK helper).",
"why_blocking": "Not blocking. libsuitesparse-dev is now unnecessary in CI but harmless. Could be removed in a follow-up cleanup.",
"suggested_fix": "Optional: remove libsuitesparse-dev from CI apt packages once confirmed fortsparse doesn't need it at build time."
}
],
"escalation_required": false,
"escalation_reason": ""
}Bottom line: Approve. The core change is sound — UMFPACK numerics are correctly preserved via the out-of-process fortsparse adapter, tests are comprehensive (all iopt paths, real/complex, reuse, multi-column, Poisson, LAPACK reference), and CI is green. File follow-up issues for GIT_TAG main pinning and dead cmake file cleanup. The factorization_exists redundancy is pre-existing and cosmetic.
The fortsparse backend replaced find_package(SuiteSparse) and include(suitesparse) with include(fortsparse), leaving cmake/suitesparse.cmake and cmake/FindSuiteSparse.cmake unreferenced. Delete the dead modules.
Pin the fortsparse FetchContent GIT_TAG to a specific commit so builds are reproducible instead of tracking whatever main points at. Remove the unused factors handle and its long kind parameter from sparse_mod; the fortsparse solver now owns factorization state. Add a unit test that exercises fs_solve_real_2 directly through the shared fs_vector_real slots, reusing one factorization across two right-hand sides, so the zero-copy two-vector path is covered outside the golden record. Drop libsuitesparse-dev from the CI apt package lists: fortsparse builds the SuiteSparse/UMFPACK chain from source, so the system package is no longer used at build or link time.
There was a problem hiding this comment.
Review verdict: Request changes
Now I have the complete picture. Here is the review:
Summary
Verdict: Request changes. The adapter design is clean and well-tested, but CI is red.
Root cause of CI failure: libmpfr-dev missing from apt packages. The PR removed libsuitesparse-dev (which transitively provided MPFR) when switching to fortsparse's from-source SuiteSparse build, but didn't add libmpfr-dev explicitly. SuiteSparse's SPEX component requires MPFR >= 4.0.2 at CMake configure time, even though SPEX is not in the SUITESPARSE_ENABLE_PROJECTS list — SuiteSparse v7.8.3 configures all projects unconditionally.
Why unit tests passed but golden-record failed: Non-deterministic apt cache. The unit-test CI run likely hit a cache populated before this PR (containing libsuitesparse-dev and transitively libmpfr-dev). The golden-record run hit a fresh install without it.
Fix: Add libmpfr-dev to the apt packages in all three workflow files (test-on-pr.yml:43, unit-tests.yml:44, unit-tests-coverage.yml:38).
Pre-existing (not a regression): factorization_exists flag shared between real and complex solvers in sparse_mod.f90:655. If sparse_free on an unfactored solver is unsafe, that's a separate issue.
Code quality: Adapter is 119 lines, no dead code, no numerical changes. Tests cover all iopt paths, complex, Poisson, two-vector shared-memory, and boundary equivalence vs LAPACK. Pointer handling in ripple_solver.f90 is correct — no use-after-free.
1 participant
Summary
sparse_modnow routes every sparse solve through a thin wrapper instead of the in-process umf4 C wrappers. The newCOMMON/sparse_solve_fortsparse.f90exposes two leaf procedures,fs_solve_realandfs_solve_complex, and the four call-site clusters insparse_mod(real/complex, single- and multi-RHS) call only those. No client code outsidesparse_modchanges; callers still drive the same factor/solve/free lifecycle throughiopt.neo_2_ql.xandneo_2_par.xno longer link GPL UMFPACK. UMFPACK now runs out-of-process in the fortsparse helper, reached over shared memory. The numerics are unchanged: the same UMFPACK does the factorization and solve, just in a separate process.The four in-process umf4 C wrappers (
COMMON/umf4_f77*wrapper*.c) are removed. fortsparse is fetched from the publiclazy-fortran/fortsparseviacmake/fortsparse.cmake. CI gainslibsuperlu-dev, required by fortsparse.Verification
Local build (
make ninja): green, binaries current.Unit tests (
ctest --test-dir TEST --output-on-failure):UMFPACK linkage (
ldd ... | grep -i umfpack):The helper links UMFPACK; the NEO-2 executables do not.
Before marking ready
The existing golden-record and performance CI (
test-on-pr.yml, TOL 1e-14..1e-15) runs only once this draft is marked ready for review. NEO-2 CI now also needslibsuperlu-dev(added in this PR) and fetches the publiclazy-fortran/fortsparse.