Reservoir coupling: Implement time stepping

[hakonhagland]

Builds on #5521 which should be merged first.

Also refactors the original adaptive time stepping method.

[bska]

Builds on #5521 which should be merged first.

Okay, in that case I'm going to mark this as "work in progress/draft" to avoid inadvertent merging.

[hakonhagland]

I'm going to mark this as "work in progress/draft" to avoid inadvertent merging.

@bska Thanks. I was actually planning to get this merged, since it is becoming difficult to handle all the dependent PRs and keep them up to date. Is this a good idea, or should I wait until the complete reservoir coupling feature is ready?

[atgeirr]

Is this a good idea, or should I wait until the complete reservoir coupling feature is ready?

It is a very good idea. Do not wait for the complete feature, unless you expect significant changes to the parts you already did. This patch has grown to become quite large, so finishing and polishing the first PRs in the series so they can be merged should be a high priority.

[hakonhagland]

It is a very good idea.

@atgeirr Ok I agree. I have removed the draft state on #5436 as it is the first in the dependency chain and we should try to merge that one first.

[blattms]

jenkins build this serial please

[blattms]

I think the jenkins failure is due to unconditional usage of MPI and unconditionally including mpi.h. Unfortunately, we require compiling without MPI to work. There is a subtle reason for this, but I fail to remember it.

[blattms]

Review of commit 3564e1d, files .../flow/ReservoirCoupling*

Do we really need the TimePoint class?

[blattms]

This will be very surprising to everybody else. I would really prefer to make this fuzzy comparison explicit where it
used. E.g. using a special free function or creating a wrapper for the comparison where it is used.

I also think that this might break quite some assumptions for the other comparison operators, e.g. if a==b is true then a<b can be true too, or even if a==b is true a<=b might be false.

[blattms]

If we can wrap the communictar int a Communicator<MPI_Comm> then you can simplify this to

this->master_slave_comm_[i].recv(slave_next_report_time_offset, 0, static_cast<int>(MessageTag::SlaveNextReportDate));

Please don't put extra throw clauses here. The default MPI error handler is MPI_ERRORS_ARE_FATAL which will call MPI_Abort. To get return codes one would have to reset it to something else. Hence the if branch will never be false of we do not reset it.

It is also not clear to me how our simulator will handle exceptions here. It might just cut the time step...

If we ever want exceptions for failing MPI calls we can use a custom throwing MPI_Errhandler. I do that in some of my tests.

[hakonhagland]

Please don't put extra throw clauses here.

I have added a custom error handler for each slave-master communicator that will eventually call MPI_Abort(). This way we do not need to check the return value for each MPI_Recv() and MPI_Send() call. See the latest commit.

[blattms]

Is there a specific reason for the type or should we maybe use the same type for start date everywhere? Seems like it is boost::gregorian::date is SimulationTimer.

[hakonhagland]

Unfortunately, we require compiling without MPI to work.

@blattms How do I build locally without MPI? I tried cmake option -DUSE_MPI=NO but opm-grid fails to build with error /usr/bin/ld: cannot find -lMPI::MPI_C: No such file or directory

[hakonhagland]

jenkins build this serial please

[hakonhagland]

jenkins build this serial please

[hakonhagland]

jenkins build this serial please

[hakonhagland]

jenkins build this serial please

[hakonhagland]

jenkins build this serial please

[hakonhagland]

jenkins build this serial please

[hakonhagland]

jenkins build this please

[hakonhagland]

jenkins build this please

[hakonhagland]

jenkins build this please

[hakonhagland]

What happens if one of slaves would crash or abort?

@blattms I think that would be detected by the master. Tested it now by purposely creating a segmentation fault in a slave. It was immediately noticed by the master, which called MPI_Abort() with: mpirun noticed that process rank 1 with PID 0 on node hakon-Precision-3570 exited on signal 11 (Segmentation fault). Similarly, I think the master will abort the slaves and itself if any slave exits without calling MPI_Finalize() first (for example, due to an uncatched exception..). However, if a slave exits normally using e.g. MPI_Finalize(); exit(0); the master will not detect it unless it later tries to communicate with the terminated slave after that, in which case the error handler in the master should be called. I guess it can try to check the error code there to determine if the slave has terminated normally?

[hakonhagland]

What happens of someone starts a simulation using the --slave parameter

@blattms First thing to notice is that the parameter is hidden so it will not be known to the user that it exists, second if the user by accident gives the --slave parameter it will be detected here that the simulation is not part of a reservoir coupling run:

opm-simulators/opm/simulators/flow/ReservoirCouplingSlave.cpp

Line 50 in 157cf97

OPM_THROW(std::runtime_error, "Slave process is not spawned by a master process");

and the simulation will be aborted.

[blattms]

jenkins build this serial please

[blattms]

Sorry, but building without MPI does not work. This prevents merging.

[hakonhagland]

jenkins build this serial please

[hakonhagland]

Is there some documentation about what we are achieving in the time stepper? Where are the synchronization points exactly and how so we accomlish it?

@blattms Good idea. I have added a description in the latest commit.

[hakonhagland]

jenkins build this please

[hakonhagland]

Is there some documentation about what we are achieving in the time stepper?

@blattms See also #5893.

[blattms]

jenkins build this serial please