Skip to content

Fix misleading "All convergence criteria satisfied." message on interrupted runs - #2881

Merged
pcarruscag merged 1 commit into
su2code:developfrom
babybluechips:fix_interrupt_exit_banner
Sep 9, 2026
Merged

pcarruscag merged 1 commit into
su2code:developfrom
babybluechips:fix_interrupt_exit_banner

Conversation

@bellonarts

Copy link
Copy Markdown

Proposed Changes

Interrupt-driven termination is not limited to a user's Ctrl-C. SIGTERM is the normal way batch schedulers terminate a job at its wall-clock limit: SLURM sends SIGTERM when a job reaches its TimeLimit and on scancel (waiting KillWait before SIGKILL), and PBS/Torque and LSF behave equivalently. Today every SU2 job that exhausts its allocation on a cluster ends with a log stating "All convergence criteria satisfied." directly above a Solver Exit table whose every criterion reads "No". Any parameter sweep or optimization loop that classifies runs by scraping that banner silently admits timed-out, unconverged runs as converged, and a human skimming the log is misled the same way. That population is far larger than the manual-interrupt case.

Mechanism: on SIGTERM the signal handler sets STOP, and COutput::ConvergenceMonitoring forces convergence = true so the run stops and saves. Both CSinglezoneDriver::Monitor and CMultizoneDriver::Monitor then read that flag as InnerConvergence and print the satisfied banner.

Fix: a new convergenceInterrupted member of COutput records that the stop came from the interrupt path rather than from the criteria. It is propagated across ranks in the existing convergence Allreduce as a count-2 element-wise MPI_MAX (SU2's serial MPI stub does not define MPI_BOR, and the AD layer maps only SUM/MIN/MAX/PROD), exposed through GetConvergenceInterrupted(), and both drivers print

Interrupt signal received, exiting before the convergence criteria were satisfied.

on that path. Stop-and-save behavior, exit code 0, and the genuine-convergence and maximum-iteration messages are unchanged.

Files: SU2_CFD/include/output/COutput.hpp, SU2_CFD/src/output/COutput.cpp, SU2_CFD/src/drivers/CSinglezoneDriver.cpp, SU2_CFD/src/drivers/CMultizoneDriver.cpp (+29/-8).

Verification (QuickStart inv_NACA0012.cfg, serial release build of develop 07aa46b1):

  • SIGTERM sent to the exact SU2_CFD process about 8 s into the run: the new interrupt message is printed, the satisfied banner does not appear, restart and solution files are written, exit code 0.
  • Run to genuine convergence and run to maximum iterations: output byte-identical to the unpatched build (checked when the change was first made on develop 81ce6a68f9).
  • Touched files re-checked with -Wall -Wextra: no new warnings.

Related Work

The defect is present at v8.5.0 and at the current develop tip. I found no existing upstream issue or PR that addresses it. The change is independent of the other NEMO-related PRs from the same qualification campaign; it touches only the output and driver exit path.

PR Checklist

Put an X by all that apply. You can fill this out after submitting the PR. If you have any questions, don't hesitate to ask! We want to help. These are a guide for you to know what the reviewers will be looking for in your contribution.

  • I am submitting my contribution to the develop branch.
  • My contribution generates no new compiler warnings (try with --warnlevel=3 when using meson).
  • My contribution is commented and consistent with SU2 style (https://su2code.github.io/docs_v7/Style-Guide/).
  • I used the pre-commit hook to prevent dirty commits and used pre-commit run --all to format old commits.
  • I have added a test case that demonstrates my contribution, if necessary.
  • I have updated appropriate documentation (Tutorials, Docs Page, config_template.cpp), if necessary.

…rupted runs

When a SIGTERM/interrupt signal is received, ConvergenceMonitoring forces
convergence = true so the solver stops and saves the solution. The steady
exit path in CSinglezoneDriver and CMultizoneDriver then printed 'All
convergence criteria satisfied.' directly above a convergence summary
table marking every field 'No'.

Track the signal-forced stop in a separate flag, propagated across ranks
in the same Allreduce, and print an explicit interrupt message instead.
Genuine convergence and maximum-iteration exits are unchanged, as is the
stop-and-save behavior on interrupt.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
bellonarts added a commit to babybluechips/SU2 that referenced this pull request Sep 6, 2026
Reseal PR su2code#2881 at 2b0b332 after replacing the flagged shell invocation with posix_spawnp/waitpid. All other exact PR and stage identities remain unchanged.
@pcarruscag

Copy link
Copy Markdown
Member

Revert the changes after I approved please.
If the pull request is not ready for review mark it as draft.

@bellonarts
bellonarts force-pushed the fix_interrupt_exit_banner branch from 2b0b332 to 07801f0 Compare September 6, 2026 02:14
@bellonarts

Copy link
Copy Markdown
Author

You’re right. I accidentally kept working on the branch after you approved it because I was trying to add regression coverage. That test created a CodeFactor warning, so I pushed another change to address it. I have now removed both changes and restored the branch exactly to 07801f0, the commit you approved. The PR is ready for review again. Sorry about that, and thank you for flagging it.

@pcarruscag
pcarruscag merged commit 2b3c6a7 into su2code:develop Sep 9, 2026
73 of 74 checks passed
bigfooted pushed a commit that referenced this pull request Oct 2, 2026
…d finite maximum residuals (#2882)

## Proposed Changes

NEMO's MAX_RES and BGS_RES groups used the calorically perfect variable
layout even though the solution vector starts with one density per
species. For AIR-5, fields labeled as momentum and energy therefore read
species residuals, and the actual momentum, energy, and
vibrational-electronic residuals were missing. This change registers and
loads the complete species-first layout in 2D and 3D. RMS/MAX/BGS share
one loading lambda, with energy at `nSpecies + nDim`. Species loop
counters are local and use `auto iSpecies = 0u`.

`CSolver::SetResidual_RMS` floors maximum residuals at `EPS*EPS` after
the value and location reductions, matching the RMS convention and
preventing `log10(0)` in output. This shared reducer also applies to
non-NEMO solvers. BGS history uses the already-floored `Residual_BGS`;
the distinct `Residual_Max_BGS` storage remains unchanged.

**Breaking NEMO field-name change:** replace `MAX_DENSITY` and
`BGS_DENSITY` with explicit `MAX_DENSITY_i` and `BGS_DENSITY_i`
selectors in `CONV_FIELD`, `HISTORY_OUTPUT`, and `SCREEN_OUTPUT`. Update
relative and zone-qualified selectors similarly, for example
`REL_MAX_DENSITY_0` or `BGS_DENSITY_0[0]`. The old unsuffixed fields
sampled species zero, not total mixture density. List every species that
must satisfy the stopping criterion. New vibrational-electronic fields
are `MAX_ENERGY_VE` and `BGS_ENERGY_VE`.

Obsolete NEMO `CONV_FIELD` names now produce an actionable startup error
instead of being discarded and potentially weakening a mixed stopping
condition. The check requires a registered per-species replacement,
preserving valid non-NEMO names. Screen/history requests retain the
existing unavailable-field handling.

The residual assembly and solution-update formulas are unchanged.
Corrected group membership changes `AVG_MAX_RES` and `AVG_BGS_RES`,
including zone-qualified averages used by multizone convergence
monitoring. AIR-5 groups now include nine variables in 2D or ten in 3D,
so runs monitoring corrected fields or averages can stop at different
iterations.

## Validation

Current head:
[d193ede](d193ede).
This follow-up only changes the 13 species-loop declarations to unsigned
`auto`, as requested in review. Recompiled current production/test
objects passed **547 assertions in 4 focused cases** on macOS arm64,
non-MPI. This was an incremental diagnostic using existing native
libraries.

The complete RunPod matrix below was run at parent
[24162ed](24162ed),
before that loop-style follow-up. A clean Linux x86_64 build used GCC
13.3.0, MPI and MLPCpp enabled. All 9,458 packaged source/dependency
files matched their recorded hashes; retained build/test artifacts were
retrieved and SHA-256 verified.

- Focused output/convergence suite: **547 assertions in 4 cases per
rank**, passing as a singleton and with **2 and 4 MPI ranks**. Covers
2D/3D RMS/MAX/BGS indexing, single-zone and multizone loading, zero
maxima, rank-dependent reductions, and maximum point/location checks.
- Explicit first/last species and energy criteria survive preprocessing
and independently control convergence. Relative and zone-qualified names
are covered. Unzoned relative checks evaluate the predicate after fields
refresh; they do not change the existing monitoring/postprocessing
order.
- All **9 migration probes** passed as singletons and with **2 MPI
ranks**; the Meson migration test passed. Obsolete base, relative,
zone-qualified, and mixed selections fail with explicit-species
guidance. Normal-unit CI now invokes this runner explicitly.
- Full normal unit suite: **74,913 assertions in 44 cases passed**.
- Official two-rank regressions passed at unchanged **1e-5 absolute
tolerance**: NEMO inviscid wedge/AUSM at iteration 10 and Euler
channel/RK at iteration 20. The wedge emitted a warning about 20,768
nonphysical reconstructed upwind states. These are bounded
reference-regression passes, not convergence or physical-admissibility
claims.
- The clean build emitted **zero compiler warnings**. Direct checks with
clang-format **15.0.7** and Black **22.6.0** passed.

OpenMP, AD, DD and sanitizer builds were not tested on RunPod. [Fresh
upstream checks at the current
head](https://github.com/su2code/SU2/pull/2882/checks) remain the merge
gate; earlier-head green checks are not treated as validation of this
head. Original cold-start AIR-5 and QuickStart comparisons remain
historical evidence and were not rerun for this revision.

## Related Work

This PR repairs NEMO residual output indexing and maximum-residual
reporting, and protects convergence configuration during the field-name
migration. It is independent of the mass-fraction tolerance change in
#2880 and interrupt-exit reporting in #2881.

## PR Checklist

- [x] I am submitting my contribution to the develop branch.
- [x] The clean Linux build emitted no compiler warnings.
- [x] My contribution is commented and consistent with SU2 style.
- [ ] I used `pre-commit run --all`. Direct pinned formatter checks were
used for this revision; the complete hook was not run locally.
- [x] I have added test cases for this contribution.
- [x] The renamed fields, configuration migration, and
stopping-criterion impact are documented above.

---------

Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants