Fix MPI hang/abort on UNK read failure in plot_wannier - #647
Conversation
|
This is nice! May I modify it slightly to put the set_error_* calls closer to the error condition along with immediate returns (instead of waiting until the end of the subroutine); that makes the code a little simpler and matches the way we check errors elsewhere (eg in deallocs, etc)? |
I've tweaked, it, as far as MPI allows, but the return itself can't be moved. Take Cf. the error checks for deallocate you mentioned: these can do an immediate return because all ranks run the same I was able to tidy things a little using *a |
|
... actually, thinking about it a bit more this what we need is a helper |
|
... done! |
|
I think it would be ok to 'return' from within the k-point loop, no? (It's just a small question, I think we can merge this already.) |
|
No — if failing ranks return from within the k-point loop and succeeding ones do not, a combination of failing and succeeding ranks would encounter different comms routines (which was the reason for the original ugly crash). That said, the refactor into a helper routine achieves the effect that you're after with an in-loop return: inside the helper each failure returns immediately, rather than having ierr checks everywhere. |
|
We designed the comms/error such that this would exactly not be a problem--the next collective operation (regardless of what it is!) is when the error status is propagated. If what you write is true, that means there is a larger problem; may I ask, did you test it, actually? May I test it? |
103892b to
d4222d5
Compare
|
@JeromeCCP9 ah, you're right, sorry! Having another look at it, the real issue was the lack of a I have updated the PR description — have a read for more details (including how to reproduce). I have also updated the PR content so that it uses the immediate return as you suggested and the error handling remains clean. Thanks for the catch! |
JeromeCCP9
left a comment
JeromeCCP9
left a comment
There was a problem hiding this comment.
Adds error checking/handling (using the current design) for reading of UNK files.
Slightly verbose because the file read is encapsulated in a new function, which counts as an improvement.
Comments are in the "claude fable5 style", but all otherwise seems fine.
(Unfortunately, this functionality is not explicitly tested in the test suite right now.)
Symptom: with MPI and wannier_plot enabled, a missing, unreadable, or
header-mismatched UNK file for one k-point could hang or abort the run
with no clean error message, instead of reporting the offending file.
Cause: k-points are distributed over ranks (dist_k), so each rank reads
only its own UNK files. Two rank-local failure sites inside the read
loop were mishandled:
1. A header mismatch called set_error_file directly. That routine syncs
the error across the communicator (comms_sync_error -> mpi_allreduce).
When only the owning rank failed, it entered the allreduce while the
other ranks proceeded to the comms_reduce of wann_func: mismatched
collectives, i.e. a deadlock (observed as an abnormal termination on
OpenMPI, an indefinite hang on stricter MPI stacks).
2. A missing file was not detected: open without status='old' recreated
it empty and the subsequent unguarded read aborted with a raw EOF
runtime error and no W90 message.
Fix: detect missing files (inquire + status='old'), add iostat to the
open and all UNK reads, and record any rank-local failure in a local
status flag instead of erroring mid-loop. After the loop every rank
synchronises together: failing ranks call set_error_file (naming the
file), succeeding ranks call comms_sync_error(comm, error, 0), so the
collective is matched and all ranks return before comms_reduce. This is
the same reduce-then-set idiom already used elsewhere (e.g. the
unlucky_rank path in wannier_prog.F90 and write_kmesh in library_extra.F90).
Repro (test-suite testw90_cube_format, 8 k-points, formatted UNK):
mpirun -np 2 wannier90.x gaas # rank 1 owns k=5-8
deleting or corrupting UNK00005.1 previously hung/aborted; it now exits
nonzero within seconds with "plot_wannier: file UNK00005.1 not found
(rank: 1)". Control runs (all files present) are unchanged.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review follow-up: record each rank-local UNK read failure directly at the failure condition with the non-collective set_base_error (the same primitive the comms module uses in its rank-divergent paths), instead of carrying ierr_read/errmsg_read flags to the end of the loop. All ranks still meet at a single comms_sync_error before the comms_reduce; an immediate collective set_error_file at the site would recreate the mismatched-collective hang this branch fixes. Re-verified (gfortran-12, OpenMPI): np=2 control byte-identical to serial modulo timestamp; missing UNK at np=2 and truncated UNK at np=4 exit with the proper werr message and no empty file created. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Extract the inquire/open/header-check/band-read sequence into a private helper. Because the helper is purely rank-local, every failure site can now set the error (set_base_error) and return immediately; the caller exits the k-point loop on an allocated error and the single collective comms_sync_error after the loop is unchanged. The disentangled and direct read paths collapse into one loop (the direct case is inc_band all-true with num_inc = num_bands), and an internal plot_read_unk_band handles one band's grid sweep, removing the eight near-identical read blocks and their iostat cascades. Also add status='old'/iostat to the UNK00001 grid-dimension probe, which previously aborted with a raw EOF backtrace on a truncated file; that path is executed by all ranks, so the collective set_error_file is safe there. Verified (gfortran-12, OpenMPI): np=2 control cube byte-identical to serial; missing UNK00005.1 (np=2), truncated body (np=4), truncated header (np=2) and truncated UNK00001 (np=2) all exit cleanly with the proper werr message and no empty file created. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review follow-up correcting an earlier misdiagnosis. The comms layer is two-tier: every synchronised collective (comms_reduce etc.) opens with the same one-integer comms_sync_error handshake that set_error_* ends with, so a single rank erroring into set_error_file is matched by the healthy ranks' next collective and the error propagates by design - there is no mismatched-collective deadlock, as verified by driving a UNK header mismatch on one rank through the pre-fix code (clean exit, correct message). What the pre-fix code actually suffered from was rank death before any error call: a missing UNK file was silently created empty by the unguarded open and the subsequent read aborted with a raw Fortran EOF runtime error. Accordingly, drop the manual comms_sync_error after the k-point loop and the set_base_error bookkeeping: each failure site in plot_read_unk now calls the ordinary collective set_error_file and returns immediately, and the caller simply returns on an allocated error. Verified (OpenMPI, gfortran-12): np=2 control byte-identical to serial; missing UNK, truncated UNK (np=4), header mismatch, and two ranks failing simultaneously all exit cleanly with the proper werr message and no empty file created. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
d4222d5 to
4b65dcd
Compare
3 participants
Symptom
In
wannier_plot, a missing, truncated, or corrupted UNK file kills the run with a raw Fortran runtime error and no wannier90 error message. A missing UNK file is silently created empty in the process.This can be reproduced using the content of
testw90_cube_format: deleteUNK00005.1and run. The code writes a 0-byteUNK00005.1, then dies withFortran runtime error: End of fileand a backtrace.Cause
The UNK opens in
plot_wannierhad nostatus='old', so opening a missing file created it. None of the header/wavefunction reads hadiostatguards, so a malformed file caused an ugly abort.Earlier misdiagnosis
Earlier commits and review discussion wrongly attributed this failure to mismatched MPI collectives. However, the comms/error layer already handles a lone failing rank by design. The final commit reverts the error plumbing to the standard "return" idiom.
Changes
status='old'plus aninquireexistence check, so a missing file produces a proper erroriostatand report failures through the standardset_error_filemechanism with an immediate return upon an errorUNK00001grid-dimension probe at the top ofplot_wanniergets the samestatus='old'&iostatchecks (a truncatedUNK00001also previously produced a raw abort)