Switching of MPI communicator#139
Conversation
There was a problem hiding this comment.
Hi @Snapex2409, thanks for the PR!
I agree with the communicator abstraction, but I do not think "no_mpi" belongs in the JSON input file. The JSON should describe the simulation problem, while the communicator is runtime/execution concern. As it is, it is potentially unsafe if "no_mpi": true and one runs mpirun -n 4 FANS inp.json result.h5.
If MPI_COMM_SELF functionality is really needed, I would prefer passing the communicator to the Reader constructor:
Reader reader(MPI_COMM_WORLD); // or Reader reader(MPI_COMM_SELF);
reader.ReadInputFile(input_fn);Then we can remove the "no_mpi" key entirely. With this design, I also do not see a need to split ReadInputFile() into ReadInputFile() and ReadJson().
This would make the PR much smaller.
So I would suggest simplifying this PR by removing "no_mpi", passing the communicator explicitly through the Reader constructor, and keeping ReadInputFile() as the single parsing function.
Please let me know if I overlooked/misunderstood something. Thanks a lot!
|
If it is passed as an argument to the Based on how I understood your suggestion, it seemed to be fixed at compile time. PS: Ideally, I require a method to dynamically change the MPI comm based on some config file for pyFANS. |
|
Thanks, that makes sense. I think there is a small misunderstanding though.. passing the communicator to the Reader reader(comm);
reader.ReadInputFile(input_fn);where For pyFANS, you should decide this dynamically from your runtime config and then pass the selected communicator. My main point is that the communicator choice should live one level above Please let me know again, if I overlooked/misunderstood something. |
|
From a design perspective, I fully understand your point. Regarding:
In pyFANS, I cannot extend the constructor to accommodate that without requiring another API change for the Micro Manager. Since the input file was the only configurable component at run time, I implemented it there initially. Would it be acceptable for the meantime to load a separate pyFANS-config file? |
|
Thanks, I understand the constraint. Regarding pyFANS, you and @IshaanDesai, of course, have complete and absolute liberty to design the runtime/API in whatever way that works best for you (this also applies to subsequent PRs from you). So yes, a separate pyFANS config file sounds much better to me, at least as an intermediate solution. Then pyFANS can read its own runtime config, decide whether it wants MPI_COMM_WORLD, MPI_COMM_SELF, or something else, and pass the corresponding communicator to the Reader. |
|
Having a separate config file for pyFANS would be fine in my opinion 👍 |
|
@Snapex2409 , Thanks a lot! I am merging this now! |
ea32461
into
DataAnalyticsEngineering:develop
This enables the selection between
MPI_COMM_WORLDandMPI_COMM_SELF.The primary use-case is for pyFANS within the Micro Manager to support both single- and multi-level parallelization (workers).
Without workers, pyFANS instances are distributed across Micro Manager ranks. However, pyFANS and the Micro Manager share the same
MPI_COMM_WORLDcommunicator, leading to incorrect computation. For such cases, parallel computation must be deactivated within pyFANS.With workers, this issue no longer persists, as workers are a separate process and, hence, have their own
MPI_COMM_WORLDcommunicator.Checklist:
CHANGELOG.md.