search

schism-dev / schism

Semi-implicit Cross-scale Hydroscience Integrated System Model (SCHISM)
http://ccrm.vims.edu/schismweb/
Apache License 2.0
78 stars 84 forks source link

Match MPI_Finalize behavior to MPI_Init #131

Closed PhilMiller closed 3 months ago

PhilMiller commented 3 months ago

For the benefit of library users of SCHISM, it should only call MPI_FInalize if it 'owns' the MPI state. If the client code passed SCHISM a communicator, then that client code owns the process's MPI. In that case, SCHISM should not finalize MPI, which may corrupt the client's state, causing a crash or other unexpected behavior near the end of the run.

PhilMiller commented 3 months ago

This change was inspired by NWM work on integrating code from ESMF that also invariably called MPI_Finalize whether we wanted it to or not.

josephzhang8 commented 3 months ago

@danishyo: how would this change impact PDAF?

josephzhang8 commented 3 months ago

Thx @PhilMiller; we understand your reasoning. I just wanted to make sure it works for another ESMF app.

danishyo commented 3 months ago

I don't use it in PDAF, but there is an interface in schism_bmi.F90 (parallel_finalize)

josephzhang8 commented 3 months ago

@platipodium: can u plz check whether it's necessary to include parallel_finalize in schism_bmi.F90? Not a big deal, but with Phil's change, it won't do anything when the communicator is inherited.

I'll go ahead and accept Phil's PR. Thx

PhilMiller commented 3 months ago

Thanks for the incredibly quick review and merge on this @josephzhang8

@danishyo What's the schism_bmi.F90 you mentioned? Is there another BMI implementation for SCHISM besides the one that Jason developed?

josephzhang8 commented 3 months ago

schism_bmi.F90 was written by @platipodium for NUOPC/UFS.

PhilMiller commented 3 months ago

Where does that implementation live? I'd like to compare it with what we have in development.

josephzhang8 commented 3 months ago

See: https://github.com/schism-dev/schism-esmf/blob/master/src/schism/schism_bmi.F90