Make MPI install optional

[shwang]

closes #430

[araffin]

Hello,
Thanks for the PR. The failure on Travis seems to come from the recent update of the docker images... (Cf #428)
If the issue is not identified, i will rollback to the previous docker image.

[AdamGleave]

docs/misc/changelog.rst should be updated to mention this.

It'd be nice to test things work as expected with or without mpi4py installed. Ideal would be to run Travis in two environments, but that'd be quite expensive, and we'd need to go through and disable the tests that actually depend on MPI in the non-MPI environment.

Just checking you can import stable_baselines without import errors if mpi4py is not installed would catch a lot of potential issues. I think something like this should work:

import sys

def test_no_mpi():
  mpi = sys.modules['mpi4py']
  sys.modules['mpi4py'] = None
  import stable_baselines
  # ... maybe do some simple smoke test in something in stable_baselines
  sys.modules['mpi4py'] = mpi  # only need to do this if process reused

[araffin]

@AdamGleave @shwang to test travis for now, you can use the v2.6.0 docker images (see d7e9591), I'm trying to find out what dependency makes it fail (it looks like it is pytest fault).

[araffin]

I rolled back the docker images for now, travis should be working again

[AdamGleave]

@shwang is this ready for review?

[shwang]

Need to add a test, not yet. Thanks for the rollback, @araffin

…

On Sun, Aug 4, 2019 at 11:47 AM Adam Gleave ***@***.***> wrote: @shwang <https://github.com/shwang> is this ready for review? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#433?email_source=notifications&email_token=AANLOM53IU2P2SBJPMZRM33QC4P3LA5CNFSM4II2P5TKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3QHLYA#issuecomment-518026720>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AANLOMZ2IWXUV6R53L42BU3QC4P3LANCNFSM4II2P5TA> .

[AdamGleave]

Need to add a test, not yet. Thanks for the rollback, @araffin
OK, ping me when you want me to take a look.

[shwang]

del sys.modules["mpi4py"] actually matters in the test_logger because misc_util.mpi_rank_or_zero() imports mpi4py inside the function definition.

I also considered adding a disabled_mpi parametrization to test_ppo2 but that doesn't seem to add anything substantive.

[shwang]

@AdamGleave ready for review

[AdamGleave]

LGTM, will merge once tests pass.

[araffin]

A bit late... I thought the README was also updated but this is not the case.
I also need to check colab notebook to ensure stable baselines with mpi is installed.

EDIT: another minor remark: the link is not showing properly in the doc https://stable-baselines.readthedocs.io/en/master/guide/install.html and we should put the "stable baselines with OpenMPI" bloc at the top I think.

[AdamGleave]

Ah, sorry for not catching that README.md wasn't updated, I've opened a new PR.