Fix abort when re-running an IB simulation in place - #1760
Closed
YuJenLiu wants to merge 1 commit into
Closed
Conversation
s_write_serial_ib_data opens p_all/p<rank>/0/ib_data.dat with
STATUS='new', so a second run of the simulation binary against the same
pre_process output aborts inside s_initialize_modules, before the time
loop:
FIO-F-208/OPEN/unit=2/'NEW' specified for file which already exists.
File name = ./p_all/p0/0/ib_data.dat
In source file src/simulation/m_data_output.fpp, at line number 865
Its twin s_write_serial_ib_state, called on the next line of
s_initialize_modules, already opens with status='replace'. This makes
the two consistent.
s_write_serial_ib_data is the sole producer of the file and always
rewrites it in full, so there is nothing to preserve. Only the
parallel_io = F path is affected; s_write_parallel_ib_data opens with
ior(MPI_MODE_WRONLY, MPI_MODE_CREATE) and cannot raise this error.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1760 +/- ##
=======================================
Coverage 61.67% 61.67%
=======================================
Files 84 84
Lines 21619 21619
Branches 3196 3196
=======================================
Hits 13334 13334
Misses 6093 6093
Partials 2192 2192 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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
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.
The bug
Re-running the
simulationbinary against an existingpre_processoutput aborts at start-up, before the time loop, for any case withib = Tandparallel_io = F:s_write_serial_ib_datawritesp_all/p<rank>/0/ib_data.datwithSTATUS='new'(src/simulation/m_data_output.fpp:865) and nothing in thesimulationbinary removesp_all, so the second run trips over the file the first one left behind.Its twin
s_write_serial_ib_state— called on the very next line ofs_initialize_modules(m_start_up.fpp:924-925) — already opens withstatus='replace'(m_data_output.fpp:1046). This PR makes the two consistent.Reproduction
Any
ib = Tcase withparallel_io = F, e.g.examples/2D_ibm_ellipse/case.pywithparallel_ioswitched to"F":Fix
s_write_serial_ib_datais the sole producer of the file and always rewrites it in full (one unformatted record,:868), so there is nothing to preserve.Alternatives I rejected:
'unknown'has no defined truncation semantics, which would matter if a futureib_markerswere ever smaller than a previous one — the reader (post_process/m_data_input.f90:156-157) does a singleread (2) ib_markers%sf(...)and would see a malformed record.'old'fails on the first run in a fresh directory. Aninquire+ delete pair is three lines for behaviour identical to one word, and needs noproc_rank == 0gate here anyway, since each rank writes into its ownp_all/p<rank>/tree (:862).Scope
:865is the only unguardedSTATUS='new'insrc/. Scanning for the pattern turns up 15 sites and 39MPI_FILE_OPENcalls; every other one is guarded, and none of theMPI_FILE_OPENcalls usesMPI_MODE_EXCL, so MPI-IO cannot produce this error at all:p_all/.../*.dat(:319–:369):311-315:793),file_per_processrestart (:721)inquire+MPI_FILE_DELETEib_state(:1046)status='replace'file_per_processib_state(:964-968)inquire+ replace/newtime_data.dat,io_time_data.dat(m_start_up.fpp:682,694)inquireat:678,:690ib_data(:865)Only the
parallel_io = Fpath is affected. Withparallel_io = Tthe dispatcher at:914-925takess_write_parallel_ib_data, which opens withior(MPI_MODE_WRONLY, MPI_MODE_CREATE).parallel_iodefaults to.false.(m_global_parameters_common.fpp:377), but everyexamples/*ibm*case setsparallel_io: "T", which is why the shipped examples never show it.Running
pre_processis not affected either: it wipesp_all/p<rank>/0before writing (pre_process/m_data_output.fpp:650-657), so-t pre_process simulationis always safe. It is re-running the simulation alone that hits it — orold_grid = T, which skips that wipe, in which case even-t pre_process simulationaborts on the second run.rm -rf p_allis not a workaround: withparallel_io = Fthe simulation reads its IC out of that same directory and aborts if it is missing (m_start_up.fpp:161-171).--cleandoes not remove it either unlesspre_processis also a target (toolchain/mfc/run/input.py:143-149).Testing
No golden files move. The two dispositions differ only when the file already exists, and in that case the old code aborted — there is no prior output for the new code to differ from. The record written is unchanged: one unformatted record of the integer
ib_markers%sf.Reproduced the abort on unmodified
master; the patched build completes the second run../mfc.sh precheckand the test suite pass.Not tested on a GPU, and it should not matter: the statement is host-side Fortran I/O in a routine that runs once from
s_initialize_modules, before the time loop. The adjacent$:GPU_UPDATE(host=...)is untouched.Why the test suite never caught it, and why I did not add a test
toolchain/mfc/test/case.py:265-278(delete_output) removesp_allbefore every case, soib_data.datnever pre-exists and'new'and'replace'are byte-identical there. Theib: "T"cases do execute this line —POST_PROCESS_OFF_PARAMSsetsparallel_io: "F"(case.py:47-50) — they just never populate the file first. The one test that keepsp_allacross a secondsimulationinvocation is the restart round-trip (case.py:196-241), but its phase 2 setst_step_start = mid_step > 0, so theif (t_step_start == 0 ...)guard atm_start_up.fpp:923short-circuits and the write never executes — and norestart_check=Truecase setsib.A regression test would require exercising a second
simulationinvocation witht_step_start = 0while preserving the existing output. I did not add one, to keep this to a single line — happy to add that coverage in a separate PR if you would like it.Contribution Policy
We do not accept pull requests generated primarily by AI without genuine understanding or real-world usage context.
All contributions are expected to demonstrate:
If these expectations are not met, we would prefer to implement the changes ourselves rather than spend time reviewing low-effort submissions.
Acknowledgement
PR template credit: junegunn