Reservoir coupling: Continue the master run when a slave run ends before it - #7379
Reservoir coupling: Continue the master run when a slave run ends before it#7379hakonhagland wants to merge 1 commit into
Conversation
|
jenkins build this serial please |
|
Note: if this is merged after #7366, it needs a patch |
A slave whose schedule ran out before the master's deadlocked the whole
coupled run. The slave called MPI_Comm_disconnect(), which blocks until
the master joins the collective close, while the master blocked in
MPI_Recv() waiting for a next report date that the slave would never
send. Neither side could proceed, and the remaining slaves were starved
behind the stuck master.
Two things were wrong. The message protocol had no way for a slave to
say that its run had ended. And receiveTerminateAndDisconnect()
disconnected without checking the signal value it had just received, so
it mistook the master's routine "keep going" for a shutdown order and
also logged a misleading "Received terminate signal from master
process".
A slave that runs out of report steps now answers on the
next-report-date channel with a negative sentinel value. A genuine
offset is measured from the slave's own start and is therefore never
negative, so no new message tag is needed, and the master learns about
the ended slave at exactly the point where it used to block. The master
acknowledges, disconnects that one intercommunicator, reports
Slave run RES-1 has ended.
Master run will continue with no flow from this slave.
and carries on without that slave.
Add slaveHasEnded() and slaveIsCoupled() alongside the existing
slaveIsActivated(). slaveIsActivated() keeps its literal meaning and
stays monotonic, while slaveIsCoupled() means "activated and not ended"
- the question every gate actually wants to ask before exchanging
messages with a slave, or before letting its master groups contribute
rates and guide rates. Switching those gates is most of the diff.
Little more was needed, because the master already had a complete
zero-flow path for a slave that has not activated yet:
receiveProductionDataFromSlaves() and receiveInjectionDataFromSlaves()
already substituted zeros, and
excludeInactiveSlaveMasterGroupsFromDistribution_() already forced the
effective group-controlled-wells count to zero so those master groups
drop out of guide-rate distribution. An ended slave reuses all of it.
Verified on a two-slave case with the first slave's last report step
removed: the run now completes, and from the moment that slave ends the
field oil production rate equals the remaining slave's group rate
exactly. With the unmodified deck, results are bit-identical to a run
without this change at every shared report point.
Making the master stop instead of continuing, via item 8 of GECON, is a
separate option that remains unimplemented.
99466cd to
5fa821e
Compare
|
jenkins build this serial please |
|
Seems like the previous jenkins build died due to a git fetch auth issue? I am going to retry the build. |
|
jenkins build this serial please |
Same failure again. @akva2 Any idea? |
|
github stability issues, nothing changed on our end. |
|
jenkins build this serial please |
There was a problem hiding this comment.
🔵 Needs a closer look
It changes a cross-process MPI message protocol and collective disconnect behavior without automated integration test coverage, so it warrants final human validation in realistic coupled-run scenarios.
Pull request overview
This PR fixes a deadlock in reservoir coupling runs where a slave can finish its schedule before the master, causing the master to block indefinitely waiting for a “next report date” message that will never arrive. The solution extends the master/slave protocol so an ending slave can notify the master at exactly the receive point that previously deadlocked, after which the master drops that slave from the coupling and continues with zero flow from it.
Changes:
- Add an end-of-run sentinel on the existing
SlaveNextReportDatechannel, with corresponding master-side detection and collective disconnect. - Introduce explicit “ended” state on the master (
slaveHasEnded) and switch coupling gates fromslaveIsActivatedtoslaveIsCoupled. - Update master-side behaviors (constraints, network activity, timestep chopping, logging) to treat ended slaves like permanently uncoupled participants.
File summaries
| File | Description |
|---|---|
| opm/simulators/wells/rescoup/RescoupConstraintsCalculator.cpp | Switch master-group constraint/dispatch gating to slaveIsCoupled() and update comments to reflect ended-slave behavior. |
| opm/simulators/wells/BlackoilWellModelRescoup_impl.hpp | Gate network-related sends/queries on slaveIsCoupled() so ended slaves don’t participate in exchanges. |
| opm/simulators/wells/BlackoilWellModelNetworkGeneric.cpp | Update network active-state logic/comments to treat ended slaves as non-contributing. |
| opm/simulators/timestepping/AdaptiveTimeStepping_impl.hpp | Report coupled-slave count via numCoupledSlaves() in master sync-step logging. |
| opm/simulators/flow/SimulatorFullyImplicit_impl.hpp | Use the new slave shutdown path (notifyEndOfRunAndDisconnect()) when the slave runs out of report steps. |
| opm/simulators/flow/rescoup/ReservoirCouplingTimeStepper.hpp | Add accessors for slaveHasEnded() / slaveIsCoupled() (and remove the old timestepper-level slaveIsActivated() accessor). |
| opm/simulators/flow/rescoup/ReservoirCouplingTimeStepper.cpp | Ignore ended slaves in timestep chopping; detect sentinel next-report offsets, then mark ended + disconnect collectively. |
| opm/simulators/flow/rescoup/ReservoirCouplingSlave.hpp | Replace terminate/disconnect API with notifyEndOfRunAndDisconnect() and document the two-case behavior. |
| opm/simulators/flow/rescoup/ReservoirCouplingSlave.cpp | Implement notifyEndOfRunAndDisconnect() and remove the old unconditional receiveTerminateAndDisconnect(). |
| opm/simulators/flow/rescoup/ReservoirCouplingMasterReportStep.hpp | Gate report-step data collection on slaveIsCoupled(). |
| opm/simulators/flow/rescoup/ReservoirCouplingMasterReportStep.cpp | Skip receives and substitute zero data when a slave is uncoupled (not activated or ended). |
| opm/simulators/flow/rescoup/ReservoirCouplingMaster.hpp | Add ended-state tracking, slaveHasEnded() / slaveIsCoupled(), and numCoupledSlaves(); declare markSlaveEndedAndDisconnect(). |
| opm/simulators/flow/rescoup/ReservoirCouplingMaster.cpp | Implement markSlaveEndedAndDisconnect(), update signal-sending to skip uncoupled/ended slaves, and avoid disconnecting ended slaves twice. |
| opm/simulators/flow/rescoup/ReservoirCoupling.hpp | Define the end-of-run sentinel and helper predicate for decoding it. |
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
In a reservoir coupling run, a slave whose schedule runs out before the master's currently deadlocks the whole simulation. This PR makes the master drop that slave from the coupling and continue with no flow from it. Thanks to @totto82 for reporting this issue.
The deadlock
The slave leaves its report-step loop and calls
run()⟶MPI_Comm_disconnect(), which is collective over the intercommunicator and therefore blocks until the master joins the close. The master, meanwhile, has started another sync step and is blocked inrunStepReservoirCouplingMaster_()⟶receiveNextReportDateFromSlaves()⟶MPI_Recv()waiting for a next report date that this slave will never send. Neither side can proceed. Any remaining slaves are starved behind the stuck master, and nothing times out — from the outside the run looks alive, because MPI busy-polls while blocked.Two separate defects were involved:
ReservoirCoupling::MessageTagexpressed it and the master had no state for it.receiveTerminateAndDisconnect()disconnected without checking the signal value it had just received. The master's routine "keep going" (0) was therefore mistaken for a shutdown order, which is also why the slave logged a misleadingReceived terminate signal from master process. Its siblingmaybeReceiveTerminateSignalFromMaster()does check the value.The protocol change
A slave that runs out of report steps now calls
notifyEndOfRunAndDisconnect()instead. It reads the master's signal first: non-zero means the master finished as well and the old path applies unchanged; zero means the master is still running and is waiting for a report date the slave does not have, so the slave answers on that same next-report-date channel withReservoirCoupling::slave_end_of_run_sentinel(-1.0) and then waits for the master's acknowledgement before joining the disconnect.Reusing the existing channel rather than adding a message tag is deliberate: it delivers the news at exactly the point where the master was previously blocked, which is what removes the deadlock. A genuine next-report-date offset is measured from the slave's own simulation start and is therefore never negative, so the sentinel is unambiguous.
On the master side,
receiveNextReportDateFromSlaves()checks each received offset for the sentinel. The offsets are broadcast to all master ranks as before, so every rank derives "this slave has ended" from the broadcast value without a second broadcast — which matters, because theMPI_Comm_disconnect()inReservoirCouplingMaster::markSlaveEndedAndDisconnect()is collective and must run on all of them.The
slaveIsCoupled()rename and the two added accessorsReservoirCouplingMastergains a per-slave "ended" state and two accessors next to the existing one:slaveIsActivated(i)— unchanged, and still monotonic: the activation handshake has happened.slaveHasEnded(i)— the slave notified us and the intercommunicator is closed.slaveIsCoupled(i)—slaveIsActivated(i) && !slaveHasEnded(i).slaveIsCoupled()is the question the code actually wants to ask before exchanging any message with a slave, and before letting its master groups contribute rates, potentials or guide rates. Every such gate was switched to it.Keeping both names, rather than quietly redefining
slaveIsActivated(), means one can see at each site which of the two questions is being asked. Only two places still ask the literal activation question, both inside the master: the activation handshake itself, and the definition ofslaveIsCoupled().Almost nothing else was needed
The master already had a complete zero-flow path, written for slaves that have not activated yet.
receiveProductionDataFromSlaves()andreceiveInjectionDataFromSlaves()already skipped the receive and substituted zeros, andexcludeInactiveSlaveMasterGroupsFromDistribution_()already switched those master groups to individual control and forced their effective group-controlled-wells count to zero, removing them from guide-rate distribution. An ended slave is the same situation, permanently, so it reuses all of it. The work was in telling the master, not in reacting to it. ATODOinRescoupConstraintsCalculator.cppanticipated exactly this change and has been replaced.Verification
Tested on a two-slave prediction case in which the first slave's last report step is removed, so it ends one month before the master:
Slave run RES-1 has ended./Master run will continue with no flow from this slave.at the first sync handshake of the affected report step, drops from "2 active slave processes" to "1", and runs the remaining month with the other slave alone. The message also reaches the master's.PRTfile.0across all 253 shared report points, forFOPR,FGPR,FGIR,FOPT, every master-group rate, and the group nodal pressureGPR:PLAT-A.The no-regression result is what the code should give by construction. With no slave ended,
slaveIsCoupled()is equivalent toslaveIsActivated()at every site, noslaveHasEnded()skip is ever taken, real offsets are never negative so the sentinel check never fires, and a master that finishes first sends a non-zero signal, which takes the branch that is identical in effect to the previous code.Not in this PR
GECON, which lets a deck ask for the master run to stop when a slave finishes rather than continue without it, remains unimplemented. It is the opt-in to the opposite behavior and is independent of the deadlock fixed here.GroupEconomicLimitsChecker::endRun()still throws for that flag, and theTODOs referring to it have been reworded to describe what is genuinely still missing.