deepmodeling / deepmd-kit

A deep learning package for many-body potential energy representation and molecular dynamics
https://docs.deepmodeling.com/projects/deepmd/
GNU Lesser General Public License v3.0
1.43k stars 494 forks source link

MPI communicator change suggestion #92

Closed PabloPiaggi closed 4 years ago

PabloPiaggi commented 4 years ago

https://github.com/deepmodeling/deepmd-kit/blob/2111ebb82d4401fbfcbff92691c417db4d961f89/source/lmp/pair_nnp.cpp#L44

I think the communicator in this line will not work when lammps is executed with the -partition option. The right communicator that should go here is called world. You can check how it's done here.

When one uses the -partition option each partition has its own communicator called "world". MPI_COMM_WORLD will include all MPI processes of all partitions while world only knows about the MPI processes of an individual partition. I assume that the parallelization of deepmd should only be done at the partition level and the communication among partitions is not relevant. Thus I suggest to change MPI_COMM_WORLD to world.

Let me know if I'm wrong. Thanks!

Pablo

amcadmus commented 4 years ago

The MPI_COMM_WOLRD is used in PairNNP::get_node_rank to determines the rank of a processes, regardless their partitions, within a node. The processes are then bound to GPU with id = node_rank % numb_GPU_on_node.

Suppose that we have 1 node, 4 processes and 4 GPUs, and run a multiple partition simulation with -p 4x1 (4 partitions each has 1 process), then each world has only one process. If we use world instead of MPI_COMM_WORLD, all processes has node_rank 0 and are bound to GPU with id 0. This is not the behavior we want.

In PairNNP::get_node_rank processes do talk to each other across partitions to determine the GPU device binding. I would not say this is the best solution, if you have any other idea please let use know. Thanks!

Han

PabloPiaggi commented 4 years ago

Then I was wrong. Thanks for the explanation!