Destroy process group in DDP destructor

[carmocca]

What does this PR do?

Discovered by running a DeepSpeed test without special=True which would produce the error:

RuntimeError: a leaf Variable that requires grad is being used in an in-place operation.

This is because a previous test initialized the process group with the gloo backend and we never destroyed it because it raised a SystemExit on setup. And since special tests are run separately this wasn't caught before.

https://github.com/PyTorchLightning/pytorch-lightning/blob/f79f0f9de112bc0a810587147b2ff5d1f3bfb256/tests/accelerators/test_ddp.py#L98-L122

So the fix is:

• Properly release the group in teardown (edit: destructor, we want to reuse the process group across fit/validate/test calls)
• Do not raise a SystemExit in setup as teardown won't get called in that case. We should eventually also clean up after ourselves in cases like that.

The changes in #8070 need this to work so that will act as a test

Closes #8115

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

• 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

[codecov]

Codecov Report

Merging #8080 (fc6338e) into master (0e19d16) will decrease coverage by 5%.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master   #8080    +/-   ##
=======================================
- Coverage      93%     88%    -5%     
=======================================
  Files         212     212            
  Lines       13720   13726     +6     
=======================================
- Hits        12751   12069   -682     
- Misses        969    1657   +688     

[tchaton]

LGTM ! Great catch !

[SeanNaren]

@awaelchli caught the fact that we probably want to keep the communication alive till the end of all trainer stages. Not entirely sure what would be the preferred approach if this is the case!

[awaelchli]

@ananthsub is destroying the process group on trainer destruction breaking any use cases you know of?

for example, this will create and destroy the group N times:

for n in range(1, N + 1):
   trainer = Trainer(max_epochs=n, ...):
   trainer.fit(...)
   ...

in current Lighting, it would only create it once

[carmocca]

this will create and destroy the group N times

So this fix is necessary because if the trainer config is different each iteration (maybe a different plugin is used), side effects from the previous process group can impact the new trainer.

[awaelchli]

@carmocca I think this is not needed anymore, right?

[carmocca]

@awaelchli We still don't destroy the process group after the Trainer ends. We do it in our CI through conftest.

However, it's unclear whether this is desired and even if we want to, it's a breaking change.

So I'm fine with closing this. Can always be revisited later.