[Horovod] Fix Reduce for Horovod

[amogkam]

What does this PR do?

#6410 added an additional reduce_boolean_decision step in ModelCheckpoint and EarlyStopping. However Horovod's reduce functionality is not currently working, therefore preventing Horovod backend from being used with the ModelCheckpoint. This PR fixes the bug with Horovod's reduce function.

cc @tchaton

Fixes #<issue_number>

Before submitting

• Was this discussed/approved via a GitHub issue? (not for typos and docs)
• Did you read the contributor guideline, Pull Request section?
• Did you make sure your PR does only one thing, instead of bundling different changes together?
• Did you make sure to update the documentation with your changes? (if necessary)
• Did you write any new necessary tests? (not for typos and docs)
• Did you verify new and existing tests pass locally with your changes?
• Did you update the CHANGELOG? (not for typos, docs, test updates, or internal minor changes/refactorings)

PR review

Anyone in the community is free to review the PR once the tests have passed.
Before you start reviewing make sure you have read Review guidelines. In short, see the following bullet-list:

• Is this pull request ready for review? (if not, please submit in draft mode)
• Check that all items from Before submitting are resolved
• Make sure the title is self-explanatory and the description concisely explains the PR
• Add labels and milestones (and optionally projects) to the PR so it can be classified

Did you have fun?

Make sure you had fun coding 🙃

[Borda]

can we pls add test for this case

[pep8speaks]

Hello @amogkam! Thanks for updating this PR.

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-04-19 10:28:00 UTC

[tchaton]

Great catch ! Yes, definitely miss this.

[codecov]

Codecov Report

Merging #6585 (d22297d) into master (4c07ab5) will decrease coverage by 7%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #6585    +/-   ##
=======================================
- Coverage      92%     85%    -7%     
=======================================
  Files         194     196     +2     
  Lines       12403   13047   +644     
=======================================
- Hits        11433   11139   -294     
- Misses        970    1908   +938     

[awaelchli]

great fix!
horovod ftw

[carmocca]

I changed the test to re-use the same code we already have for ddp, and also added it to special_tests.sh.
However, the test does not work:

tests/checkpointing/test_checkpoint_callback_frequency.py:139: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
pytorch_lightning/trainer/connectors/env_vars_connector.py:40: in insert_env_defaults
    return fn(self, **kwargs)
pytorch_lightning/trainer/trainer.py:308: in __init__
    replace_sampler_ddp, deterministic, precision, amp_backend, amp_level, plugins
pytorch_lightning/trainer/connectors/accelerator_connector.py:127: in __init__
    self.set_distributed_mode()
pytorch_lightning/trainer/connectors/accelerator_connector.py:546: in set_distributed_mode
    self._set_horovod_backend()
pytorch_lightning/trainer/connectors/accelerator_connector.py:567: in _set_horovod_backend
    self.check_horovod()
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <pytorch_lightning.trainer.connectors.accelerator_connector.AcceleratorConnector object at 0x7f78aee90950>

    def check_horovod(self):
        """Raises a `MisconfigurationException` if the Trainer is not configured correctly for Horovod."""
        if not _HOROVOD_AVAILABLE:
            raise MisconfigurationException(
                'Requested `distributed_backend="horovod"`, but Horovod is not installed.'
                "Install with \n $HOROVOD_WITH_PYTORCH=1 pip install horovod[pytorch]"
            )
    
        if self.num_gpus > 1 or self.num_nodes > 1:
            raise MisconfigurationException(
>               "Horovod does not support setting num_nodes / num_gpus explicitly. Use "
                "horovodrun / mpirun to configure the number of processes."
            )
E           pytorch_lightning.utilities.exceptions.MisconfigurationException: Horovod does not support setting num_nodes / num_gpus explicitly. Use horovodrun / mpirun to configure the number of processes.

pytorch_lightning/trainer/connectors/accelerator_connector.py:601: MisconfigurationException

I guess the test needs to use?:

https://github.com/PyTorchLightning/pytorch-lightning/blob/master/tests/models/test_horovod.py#L46-L63

But this doesn't allow passing a custom ModelCheckpoint so a different way of testing this is necessary.

[awaelchli]

This change recently got merged here #6958
Apologies, we didn't remember this PR was open and I just saw it.

[awaelchli]

I would not include horovod in the parameterization here yet, otherwise we risk getting another flaky test. I believe we need to improve our testing integration with horovod first. #6935

[awaelchli]

since the main fix of this PR was merged, I suggest to close this one