Skip to content

Cleanup custom MPI flow#762

Open
TysonRayJones wants to merge 23 commits into
develfrom
cleanup-custom-mpi
Open

Cleanup custom MPI flow#762
TysonRayJones wants to merge 23 commits into
develfrom
cleanup-custom-mpi

Conversation

@TysonRayJones
Copy link
Copy Markdown
Member

No description provided.

These exemptions for communicator NULL-ness enable an error message to reach the user even then the user has called MPI_Init themselves but then triggered a validation error before the communicator could be set
for consistency with e.g. env.isMultithreaded. The user arg to e.g. initQuESTEnv() is kept as "userOwnsMpi" for a very superficial consistency with e.g. "useMultithreaded" :^)
by just using extern. This is terrible and inadvisable, but offers an easier understanding of the software architecture (and so is easier to fix correctly) than the previous "macros change which signatures this header exposes" design.

Also, we removed the unnecessary avoiding of defining comm_setMpiComm when SUBCOMM was not defined, which made the architecture even more confusing. Now, SUBCOMM only influences the contents of subcommunicator.cpp and subcommunicator.hpp. Simple!
although I suspect this is a poor choice of name. The logic should always be considered "compiled" when MPI is known, and we choose instead whether to "expose" the MPI signature to the users
without triggering an internal error
replacing the original internal error. Note that all of the other MPI functions between comm_config.cpp and comm_subroutines.cpp are unguarded; we should create a macro around them
Comment on lines +1514 to +1518
// (B) If QuEST is instructed not to use distribution, we must demand the user is not
// using MPI, because we internally consult comm_isInit() to detect QuEST distribution
// in many functions, and that will give a false positive when the user inits MPI directly.
if (!useDistrib)
assertThat(!isMpiInit, report::QUEST_IS_NON_DISTRIBUTED_BUT_MPI_WAS_INIT, caller);
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@otbrown is this limitation okay? It is: when QuEST is compiled with MPI, but distribution is disabled at runtime (during initCustom(Mpi...)QuESTEnv), we forbid the user to use MPI themselves. This is due to a relatively minor technical limitation; QuEST internally consults comm_isInit() (which will detect the user's MPI_Init() call) in many places as a proxy for "is QuEST distributed?".

A current workaround for such users is to re-compile QuEST without MPI, in which case comm_isInit() decouples from MPI. And alas currently, a user initailising MPI after a non-distributed (but MPI-compiled) QuEST initialisation will corrupt QuEST - it will survive initQuESTEnv validation but later will be tricked by comm_isInit().

EDIT: during writing this, I've convinced myself that it should be valid/legal for users to compile QuEST with MPI, but runtime disable QuEST distribution, and thereafter discretionarily call MPI_Init. Corrupting QuEST is a major pitfall. I'll fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant