fix(tot): cascade eq_api_init in tot_init (#209)#211
Merged
Conversation
Before this change, tot_api_init brought up tr/ti/fp/wr but NOT eq_api. eq_api maintains its own module-local g_initialized flag (eq/eq_api.f90:71) separate from tot_api's. Direct ctypes callers chaining `tot_init() -> eq_set_param(...)` would hit EQ_ERR_NOT_INIT unless they also explicitly called eq_init() (which the Phase 2b Layer-C smoke test had to). Option A from #209: tot_api_init now cascades eq_api_init alongside the other per-module API inits, in binary-lifecycle order (eq first, then tr/ti/fp/wr). tot_api_finalize mirrors this with eq_api_finalize in reverse order. Per the existing tot_api.f90 header comment, the second equnit_eq_init call (from tr_api_init's CALL eq_init) is idempotent — so the cascade has no behavioral side-effect on the existing per-module path. The change is C-ABI extension only — no structural refactor of any Fortran module — and stays within the k-yoshimi-led boundary per feedback_fortran_refactor_needs_lead_signoff.md. Tests - python/totlib/tests/test_tot_init_eq_api_cascade.py (new): regression on both default libtotapi.so and libtotapi_mono.so. Asserts eq_set_param after tot_init returns EQ_OK without an explicit eq_init(). - python/totlib/tests/test_mono_bpsd_smoke.py::test_bpsd_round_trip: the now-redundant explicit lib.eq_init() call is removed, leaving the smoke test itself as a second regression guard. - 1e-10 equivalence (totlib/tests/test_equivalence.py) preserved. Closes #209. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…#209) Follow-up to 427f597 addressing reviewer findings: 1. tot/Makefile: tot_api.f90 now USEs eq_api, but TOT_L6_API_OBJ listed only tr/ti/fp/wrx state+api objects. Add ../eq/eq_state.o and ../eq/eq_api.o so the static `make tot_api_check` chain (used by CI's tot_api_check_all step after `make -C tot libs` bootstraps the non-PIC tree) can compile + link cleanly. Also add explicit delegated build rules `../eq/eq_state.o:` and `../eq/eq_api.o:` mirroring the existing tr/ti/fp/wrx delegations, and add ../eq/libeq.a + ../eq/{eq_state.o,eq_api.o} to tot_api.o's prerequisite list so make's topological order matches. The PIC path (libtotapi.so / libtotapi_mono.so) was already correct since it links against ../eq/libeqapi.so. (Codex retrospective MED.) 2. tot/tot_api.h: header docstring updated to mention eq_init in the fan-out list. (Both reviewers LOW; consumer-facing source-of-truth.) 3. python/totlib/tests/test_tot_init_eq_api_cascade.py: add a module-level `pytestmark = [pytest.mark.forked]` so the file self-documents the per-test fork-isolation requirement and gets forked even on a local run that forgets the `--forked` CLI flag. eq_api's g_initialized is module-level (eq/eq_api.f90:71); a prior in-process test flipping it to TRUE could let this test pass spuriously without isolation. (Codex retrospective LOW.) No behavioral change to the cascade itself. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CI link of tests/c_abi/test_smoke fails on develop because eq_api.o references eq_common_get_grid_counts_ / eq_common_get_scalars_ / etc. which live in eq_api_common.f. That helper is in eq's OBJ_API but NOT bundled into libeq.a, so the previously-added ../eq/eq_api.o entry in TOT_L6_API_OBJ (commit e676534) needs eq_api_common.o alongside it. Add ../eq/eq_api_common.o to TOT_L6_API_OBJ + delegated build rule. Verified locally with `make -C tot libs && make -C tot GFLIBS= tot_api_check` (CI's no-graphics build mode) — exits 0. The PIC paths (libtotapi.so / libtotapi_mono.so) are unaffected: they link against ../eq/libeqapi.so where eq_api_common.o is already co-linked via eq's OBJ_LIBEQAPI = OBJS_PIC + OBJ_API_PIC + stubs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Owner
Author
|
@cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit bda3a75. Configure here.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #209. tot_api_init now cascades
eq_api_init(and tot_api_finalize cascadeseq_api_finalize) so direct ctypes callers can driveeq_set_param/eq_runimmediately aftertot_init()— no extraeq_init()round trip.Option A from the issue body. Stays within the k-yoshimi-led C-ABI boundary per
feedback_fortran_refactor_needs_lead_signoff.md(no Fortran structural refactor of eq/tr/etc.).What's in the PR (2 commits)
427f5972fix(tot): cascade eq_api_init in tot_init — the core change.tot_api.f90: USE eq_api (init/finalize hooks only), addg_eq_presentflag, calleq_api_init()first in init (mirrors binary lifecycle order pl→eq→tr→…), calleq_api_finalize()last in finalize (reverse). Rollback paths on per-module init failure also tear down eq. New regression test + Layer-C smoke cleanup.e6765344fix(tot): wire eq deps into static check + harden test fork isolation — review-feedback follow-up.tot/Makefileadds../eq/eq_state.o+../eq/eq_api.otoTOT_L6_API_OBJandtot_api.o's prereq list so the staticmake tot_api_checkchain (CI'stot_api_check_allstep) can compile + link cleanly.tot/tot_api.hdoc updated. New test gets apytestmark = [pytest.mark.forked]so it self-enforces fork isolation locally (CI uses--forkedglobally).Why the cascade
eq_apikeeps its own module-localg_initializedflag (eq/eq_api.f90:71) separate fromtot_api's. Pre-this-PR,tot_api_init's call chain (tr_api_init → equnit::eq_init) brought up eq's COMMON state but did NOT flip eq_api's flag. A direct-ctypes caller runningtot_init() → eq_set_param("MODELG", 3.0)hitEQ_ERR_NOT_INIT (2). The Phase 2b Layer-C smoke (test_mono_bpsd_smoke.py) had to calllib.eq_init()explicitly as a workaround. This PR removes that workaround.Tests
python/totlib/tests/test_tot_init_eq_api_cascade.py— regression on BOTH defaultlibtotapi.so(TOTLIB_PATH) andlibtotapi_mono.so(MONO_LIB_PATH). Assertseq_set_paramreturnsEQ_OK (0)aftertot_init()with no expliciteq_init(). Module-levelpytest.mark.forkedfor per-test fork isolation.python/totlib/tests/test_mono_bpsd_smoke.py::test_bpsd_round_trip— explicitlib.eq_init()call removed; the test now implicitly serves as a second regression guard.totlib/tests/test_equivalence.py) preserved.pytest --forkedrun: 155/156 pass. The 1 fail (test_close_raises_exception_group_when_multiple_modules_fail) is a pre-existing Python 3.10ExceptionGroupcompat issue — unrelated; reproduces on develop HEAD baseline.Reviewer findings (in-house + Codex, both passes)
e6765344.g_eq_presentis decorative until L-7 state aggregation lands — accepted as-is (matches existingg_tr_presentstyle).e6765344.Out of scope (carried over)
Test plan
python-tests.yml(totlib + eq + tot_api_check_all)🤖 Generated with Claude Code
Note
Medium Risk
Touches orchestrator lifecycle by adding
eq_api_init/eq_api_finalizetotot_init/tot_finalizeand adjusting rollback order, which could affect initialization/finalization behavior across modules if ordering assumptions exist.Overview
tot_init()now cascadeseq_api_init()(andtot_finalize()cascadeseq_api_finalize()) so callers can use the eq C ABI (eq_set_param/eq_run) immediately aftertot_init()without a separateeq_init()call.Build/test coverage was updated to match: the
tot_api_checkstatic-link path now includes eq API/state objects in thetot/Makefile, the mono BPSD smoke test drops the expliciteq_init()workaround, and a new fork-isolated regression test asserts the cascade works for bothlibtotapi.soandlibtotapi_mono.so.Reviewed by Cursor Bugbot for commit bda3a75. Bugbot is set up for automated code reviews on this repo. Configure here.