Repository: horovod/horovod
Pull request: fix MPICH implementation
## Checklist before submitting

- [x] Did you read the [contributor guide](https://github.com/horovod/horovod/blob/master/CONTRIBUTING.md)?
- [ ] Did you update the docs?
- [x] Did you write any tests to validate this change?  
- [ ] Did you update the [CHANGELOG](https://github.com/horovod/horovod/blob/master/CHANGELOG.md), if this change affects users?

## Description

Fixes #1637.

I notice horovod's MPICH implementation is broken (#1637) but Intel MPI implementation is good (#2374). Per [Intel MPI's official page](https://software.intel.com/content/www/us/en/develop/tools/oneapi/components/mpi-library.html), it implements the MPICH specification (in fact it's MPICH derivative), so we can safely use Intel MPI's implementation for MPICH.

## Review process to land 

1. All tests and other checks must succeed.
2. At least one member of the [technical steering committee](https://github.com/horovod/horovod/blob/master/CONTRIBUTING.md) must review and approve.
3. If any member of the technical steering committee requests changes, they must be addressed.


Problem statement:
Support MPICH when using horovodrun
I'm using MPICH instead of OpenMPI. 
I'm able to train models using mpiexec and mpirun on my cluster but when I try to use horovodrun, it fails with the following error

```python
[ec2-user@master ~]$python3 ./horovodrun -np 8 python3 <train command>
[mpiexec@master.amazon.com] match_arg (utils/args/args.c:159): unrecognized argument allow-run-as-root
[mpiexec@master.amazon.com] HYDU_parse_array (utils/args/args.c:174): argument matching returned error
[mpiexec@master.amazon.com] parse_args (ui/mpich/utils.c:1596): error parsing input array
[mpiexec@master.amazon.com] HYD_uii_mpx_get_parameters (ui/mpich/utils.c:1648): unable to parse user arguments
[mpiexec@master.amazon.com] main (ui/mpich/mpiexec.c:153): error parsing parameters
```

Horovod version: 0.18.2
MPICH version: 3.2

From looking at it, it looks like certain args are not available in MPICH.
Is horovodrun compatible with MPICH?

Hints from issue discussion:
Hey @chandana1332, `horovodrun` should support OpenMPI, Spectrum MPI, MPICH, and Gloo.  However, we've run into some issues with MPICH in the past around `--allow-run-as-root`, and possibly other commands.  Contributions would definitely be welcome, if you know what needs to be modified to fully support MPICH.
Thanks for confirming. I'm not as familiar with the horovod codebase yet to contribute but will look into it. 

I was trying to use horovodrun to profile horovod. Given that I can't use it with MPICH at the moment, are you aware of any workarounds to profile horovod? Would it help if I manually altered the mpirun command to remove the flags that are not supported in MPICH yet?
@karan6181 would you like to take a look at this if available? Thanks!

@chandana1332 when you say you want to profile horovod, have you looked at horovod timeline?
@chandana1332 Please use this option to profile horovod timeline for mpich library. `-env HOROVOD_TIMELINE=<path where you want to save a json file>`

e.g.:
`mpirun -np 1 -env HOROVOD_TIMELINE=/home/ubuntu/sample.json python -u horovod/examples/mxnet_mnist.py`

Below are my details of mpirun version:

```shell
ubuntu@ip-172-31-22-85:~$ mpirun --version
HYDRA build details:
    Version:                                 3.2
    Release Date:                            Wed Nov 11 22:06:48 CST 2015
    CC:                              gcc   -Wl,-Bsymbolic-functions -Wl,-z,relro
    CXX:                             g++   -Wl,-Bsymbolic-functions -Wl,-z,relro
    F77:                             gfortran  -Wl,-Bsymbolic-functions -Wl,-z,relro
    F90:                             gfortran  -Wl,-Bsymbolic-functions -Wl,-z,relro
    Configure options:                       '--disable-option-checking' '--prefix=/usr' '--build=x86_64-linux-gnu' '--includedir=${prefix}/include' '--mandir=${prefix}/share/man' '--infodir=${prefix}/share/info' '--sysconfdir=/etc' '--localstatedir=/var' '--disable-silent-rules' '--libdir=${prefix}/lib/x86_64-linux-gnu' '--libexecdir=${prefix}/lib/x86_64-linux-gnu' '--disable-maintainer-mode' '--disable-dependency-tracking' '--enable-shared' '--enable-fortran=all' '--disable-rpath' '--disable-wrapper-rpath' '--sysconfdir=/etc/mpich' '--libdir=/usr/lib/x86_64-linux-gnu' '--includedir=/usr/include/mpich' '--docdir=/usr/share/doc/mpich' '--with-hwloc-prefix=system' '--enable-checkpointing' '--with-hydra-ckpointlib=blcr' 'CPPFLAGS= -Wdate-time -D_FORTIFY_SOURCE=2 -I/build/mpich-jQtQ8p/mpich-3.2/src/mpl/include -I/build/mpich-jQtQ8p/mpich-3.2/src/mpl/include -I/build/mpich-jQtQ8p/mpich-3.2/src/openpa/src -I/build/mpich-jQtQ8p/mpich-3.2/src/openpa/src -D_REENTRANT -I/build/mpich-jQtQ8p/mpich-3.2/src/mpi/romio/include' 'CFLAGS= -g -O2 -fstack-protector-strong -Wformat -Werror=format-security -O2' 'CXXFLAGS= -g -O2 -fstack-protector-strong -Wformat -Werror=format-security -O2' 'FFLAGS= -g -O2 -fstack-protector-strong -O2' 'FCFLAGS= -g -O2 -fstack-protector-strong -O2' 'build_alias=x86_64-linux-gnu' 'MPICHLIB_CFLAGS=-g -O2 -fstack-protector-strong -Wformat -Werror=format-security' 'MPICHLIB_CPPFLAGS=-Wdate-time -D_FORTIFY_SOURCE=2' 'MPICHLIB_CXXFLAGS=-g -O2 -fstack-protector-strong -Wformat -Werror=format-security' 'MPICHLIB_FFLAGS=-g -O2 -fstack-protector-strong' 'MPICHLIB_FCFLAGS=-g -O2 -fstack-protector-strong' 'LDFLAGS=-Wl,-Bsymbolic-functions -Wl,-z,relro' 'FC=gfortran' 'F77=gfortran' 'MPILIBNAME=mpich' '--cache-file=/dev/null' '--srcdir=.' 'CC=gcc' 'LIBS=-lpthread '
    Process Manager:                         pmi
    Launchers available:                     ssh rsh fork slurm ll lsf sge manual persist
    Topology libraries available:            hwloc
    Resource management kernels available:   user slurm ll lsf sge pbs cobalt
    Checkpointing libraries available:       blcr
    Demux engines available:                 poll select
```
Hello @karan6181 , I am also using mpirun with mpich and want to force to use TCP as mentioned here:

```
mpirun -np 4 \
    -bind-to none -map-by slot \
    -x NCCL_DEBUG=INFO -x LD_LIBRARY_PATH -x PATH \
    -mca pml ob1 -mca btl ^openib \
    python train.py

```

but I am able to run these -mca flags with mpirun, can you suggest alternate to use -mca with mpich

Thanks

Commit message: enable tests for MPICH and Intel MPI

Signed-off-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu>

Patch to review:
diff --git a/horovod/runner/mpi_run.py b/horovod/runner/mpi_run.py
index 7b5be2787d..a9b39cc085 100644
--- a/horovod/runner/mpi_run.py
+++ b/horovod/runner/mpi_run.py
@@ -153,7 +153,7 @@ def mpi_run(settings, nics, env, command, stdout=None, stderr=None):
     if mpi_impl_flags is None:
         raise Exception(_MPI_NOT_FOUND_ERROR_MSG)
 
-    impi = _IMPI_IMPL == mpi
+    impi = (_IMPI_IMPL == mpi or _MPICH_IMPL == mpi)
 
     ssh_args = []
     if settings.ssh_port:
diff --git a/test/integration/test_static_run.py b/test/integration/test_static_run.py
index 34e90714de..88322f8667 100644
--- a/test/integration/test_static_run.py
+++ b/test/integration/test_static_run.py
@@ -29,7 +29,7 @@
 from horovod.runner.common.util import safe_shell_exec
 from horovod.runner import _HorovodArgs
 from horovod.runner.launch import _check_all_hosts_ssh_successful, _run
-from horovod.runner.mpi_run import mpi_available, is_mpich, is_intel_mpi
+from horovod.runner.mpi_run import mpi_available
 
 sys.path.append(os.path.join(os.path.dirname(__file__), os.pardir, 'utils'))
 
@@ -141,10 +141,6 @@ def test_run_success(self, controller, mode, run):
         if controller == 'mpi':
             if not (mpi_built() and mpi_available()):
                 self.skipTest("MPI is not available")
-            if is_mpich():
-                self.skipTest("MPICH is not testable")
-            if is_intel_mpi():
-                self.skipTest("Intel(R) MPI is not testable because it is based on MPICH")
 
         self.do_test_run_with_controller_success(controller, mode, run)
 
@@ -156,10 +152,6 @@ def test_run_failure(self, controller, mode, run):
         if controller == 'mpi':
             if not (mpi_built() and mpi_available()):
                 self.skipTest("MPI is not available")
-            if is_mpich():
-                self.skipTest("MPICH is not testable")
-            if is_intel_mpi():
-                self.skipTest("Intel(R) MPI is not testable because it is based on MPICH")
 
         self.do_test_run_with_controller_failure(controller, mode, run)
 


Code review comments:
diff_hunk: @@ -153,7 +153,7 @@ def mpi_run(settings, nics, env, command, stdout=None, stderr=None):
     if mpi_impl_flags is None:
         raise Exception(_MPI_NOT_FOUND_ERROR_MSG)
 
-    impi = _IMPI_IMPL == mpi
+    impi = (_IMPI_IMPL == mpi or _MPICH_IMPL == mpi)
path: horovod/runner/mpi_run.py
text: @user1:
Can this logic be rephrased to this, which I think is more readable?
```suggestion
    impi = mpi in (_IMPI_IMPL, _MPICH_IMPL)
```

@user1:
I think, `impi` should be renamed to `impi_or_mpich` as this is what it now encodes.

@author:
Fixed.