[bugfix] Add mechanism to prevent deadlock for DDP on Exception Trigger #8167

tchaton · 2021-06-28T09:20:33Z

What does this PR do?

This PR introduces a mechanism to prevent deadlock.

Mechanism:

On ddp instantiation, make all processes aware of pids
Share a temporary directory used to save some synchronisation state

On failure for a given process:

Save a dummy object to the temporary
Wait a little bit
Check if the other processes have actually wrote to the folder.
If the num files == world_size, all good !
else: kill all process and raised DeadLock Exception.

Other approaches tried:

Use timetout for TCPStore -> result in cuda failure
Use signal -> Very flicky and un-stable

Fixes #8130

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 🙃

pep8speaks · 2021-06-28T09:20:36Z

Hello @tchaton! Thanks for updating this PR.

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

Comment last updated at 2021-06-28 17:45:42 UTC

for more information, see https://pre-commit.ci

codecov · 2021-06-28T09:21:37Z

Codecov Report

Merging #8167 (650a4fe) into master (c3065c5) will decrease coverage by 0%.
The diff coverage is 92%.

@@           Coverage Diff           @@
##           master   #8167    +/-   ##
=======================================
- Coverage      93%     93%    -0%     
=======================================
  Files         211     211            
  Lines       13440   13607   +167     
=======================================
+ Hits        12450   12602   +152     
- Misses        990    1005    +15

…tning/pytorch-lightning into ddp_kill_on_deadlock

CHANGELOG.md

kaushikb11

LGTM!

pytorch_lightning/plugins/training_type/ddp.py

pytorch_lightning/trainer/trainer.py

awaelchli · 2021-06-28T10:25:33Z

@tchaton thanks for working on this.
What is the reason that this distributed-specific code is in the Trainer? With accelerators and plugins we aimed at abstracting this away. Can we consider implementing such a sync function in the parallel plugin / ddp plugin?

for more information, see https://pre-commit.ci

tchaton · 2021-06-28T10:55:54Z

@tchaton thanks for working on this.
What is the reason that this distributed-specific code is in the Trainer? With accelerators and plugins we aimed at abstracting this away. Can we consider implementing such a sync function in the parallel plugin / ddp plugin?

Everything has been moved to the plugin. Just wanted to confirm the logic :)

Best,
T.C

pytorch_lightning/plugins/training_type/ddp.py

CHANGELOG.md

pytorch_lightning/plugins/training_type/ddp.py

pytorch_lightning/plugins/training_type/training_type_plugin.py

for more information, see https://pre-commit.ci

…tning/pytorch-lightning into ddp_kill_on_deadlock

pytorch_lightning/plugins/training_type/ddp.py

awaelchli

overall LGTM just unsure about how the test works

pl_examples/basic_examples/autoencoder.py

tests/trainer/test_trainer.py

ananthsub

This is an interesting approach @tchaton !

How does this work if the trainer is running with torchelastic? and as @ethanwharris points out, what happens in the multinode case?

tchaton · 2021-06-28T17:37:35Z

This is an interesting approach @tchaton !

How does this work if the trainer is running with torchelastic? and as @ethanwharris points out, what happens in the multinode case?

Hey @ananthsub. Thanks for checking this out !

In the case of multinode or TorchElastic, we can check that the saved files are matching the node size.
We will add support for those in following PRs.
If you can help us test this feature on multinode or TorchElastic and help us add support for them,
this would be really appreciated !

Best,
T.C

pytorch_lightning/plugins/training_type/ddp.py

pytorch_lightning/plugins/training_type/parallel.py

pytorch_lightning/utilities/exceptions.py

Co-authored-by: Jirka Borovec <[email protected]>

carmocca · 2021-06-29T11:54:05Z

pytorch_lightning/plugins/training_type/ddp.py

+        for pid in self._pids:
+            if pid != os.getpid():
+                os.kill(pid, signal.SIGKILL)
+            shutil.rmtree(sync_dir)


Should this clean-up command be part of __del__ so we make sure it always gets removed? Or have it run in acceleratotr.on_train_end

no, we don't want to kill the processes.
the ddp processes must survive until the end of the script.

that is, unless there was an exception and we want to exit the program, which is what @tchaton tries to handle here.

I'm referring specifically to the line

shutil.rmtree(sync_dir)

Hey @carmocca,

Yes, this will make it cleaner.

…er (#8167) * add mechanism to prevent deadlock * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * resolve flake8 + update changelog * update on comments * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * update * remove space * resolve bugs * overwrite config * update on comments * update on comments * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * update * update * update test with comments * Update pytorch_lightning/plugins/training_type/parallel.py Co-authored-by: Jirka Borovec <[email protected]> * update on comments Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Adrian Wälchli <[email protected]> Co-authored-by: Jirka Borovec <[email protected]>

add mechanism to prevent deadlock

20d4726

tchaton self-assigned this Jun 28, 2021

tchaton added this to the v1.3.x milestone Jun 28, 2021

tchaton added bug Something isn't working distributed Generic distributed-related topic labels Jun 28, 2021

[pre-commit.ci] auto fixes from pre-commit.com hooks

8ddf6e1

for more information, see https://pre-commit.ci

tchaton added 2 commits June 28, 2021 10:27

resolve flake8 + update changelog

ae3240c

Merge branch 'ddp_kill_on_deadlock' of https://github.com/PyTorchLigh…

c14b250

…tning/pytorch-lightning into ddp_kill_on_deadlock

kaushikb11 reviewed Jun 28, 2021

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

kaushikb11 approved these changes Jun 28, 2021

View reviewed changes

justusschock reviewed Jun 28, 2021

View reviewed changes

tchaton and others added 4 commits June 28, 2021 06:42

update on comments

82bdce3

[pre-commit.ci] auto fixes from pre-commit.com hooks

0fffed6

for more information, see https://pre-commit.ci

update

fd6574a

remove space

b371fcf

tchaton marked this pull request as ready for review June 28, 2021 11:03

tchaton requested review from Borda, carmocca, SeanNaren and williamFalcon as code owners June 28, 2021 11:03

resolve bugs

b6918d6

justusschock approved these changes Jun 28, 2021

View reviewed changes

pytorch_lightning/plugins/training_type/ddp.py Outdated Show resolved Hide resolved

tchaton added 2 commits June 28, 2021 09:12

overwrite config

b00161e

update on comments

63591be

tchaton added the ready PRs ready to be merged label Jun 28, 2021

awaelchli reviewed Jun 28, 2021

View reviewed changes

tchaton and others added 5 commits June 28, 2021 10:17

update on comments

ee0c4d2

[pre-commit.ci] auto fixes from pre-commit.com hooks

45624f2

for more information, see https://pre-commit.ci

update

80de1a6

Merge branch 'ddp_kill_on_deadlock' of https://github.com/PyTorchLigh…

ad330fe

…tning/pytorch-lightning into ddp_kill_on_deadlock

update

c68fa6c

ethanwharris reviewed Jun 28, 2021

View reviewed changes

pytorch_lightning/plugins/training_type/ddp.py Show resolved Hide resolved

awaelchli approved these changes Jun 28, 2021

View reviewed changes

pl_examples/basic_examples/autoencoder.py Show resolved Hide resolved

tests/trainer/test_trainer.py Outdated Show resolved Hide resolved

update test with comments

f119e8f

ananthsub reviewed Jun 28, 2021

View reviewed changes

Borda approved these changes Jun 28, 2021

View reviewed changes

pytorch_lightning/plugins/training_type/ddp.py Show resolved Hide resolved

pytorch_lightning/plugins/training_type/parallel.py Outdated Show resolved Hide resolved

pytorch_lightning/utilities/exceptions.py Outdated Show resolved Hide resolved

tchaton and others added 2 commits June 28, 2021 18:43

Update pytorch_lightning/plugins/training_type/parallel.py

7b4d133

Co-authored-by: Jirka Borovec <[email protected]>

update on comments

650a4fe

tchaton enabled auto-merge (squash) June 28, 2021 17:45

tchaton merged commit c521624 into master Jun 28, 2021

tchaton deleted the ddp_kill_on_deadlock branch June 28, 2021 19:26

tchaton mentioned this pull request Jun 29, 2021

[Feat] Add Loops Restart #8131

Closed

11 tasks

carmocca reviewed Jun 29, 2021

View reviewed changes

carmocca mentioned this pull request Jun 30, 2021

ModelCheckpoint does not save checkpoint on training end #8126

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[bugfix] Add mechanism to prevent deadlock for DDP on Exception Trigger #8167

[bugfix] Add mechanism to prevent deadlock for DDP on Exception Trigger #8167

tchaton commented Jun 28, 2021 •

edited

Loading

pep8speaks commented Jun 28, 2021 •

edited

Loading

codecov bot commented Jun 28, 2021 •

edited

Loading

kaushikb11 left a comment

awaelchli commented Jun 28, 2021

tchaton commented Jun 28, 2021

awaelchli left a comment

ananthsub left a comment

tchaton commented Jun 28, 2021 •

edited

Loading

carmocca Jun 29, 2021

awaelchli Jun 29, 2021

carmocca Jun 29, 2021

tchaton Jun 29, 2021 •

edited

Loading

[bugfix] Add mechanism to prevent deadlock for DDP on Exception Trigger #8167

[bugfix] Add mechanism to prevent deadlock for DDP on Exception Trigger #8167

Conversation

tchaton commented Jun 28, 2021 • edited Loading

What does this PR do?

Before submitting

PR review

Did you have fun?

pep8speaks commented Jun 28, 2021 • edited Loading

Comment last updated at 2021-06-28 17:45:42 UTC

codecov bot commented Jun 28, 2021 • edited Loading

Codecov Report

kaushikb11 left a comment

Choose a reason for hiding this comment

awaelchli commented Jun 28, 2021

tchaton commented Jun 28, 2021

awaelchli left a comment

Choose a reason for hiding this comment

ananthsub left a comment

Choose a reason for hiding this comment

tchaton commented Jun 28, 2021 • edited Loading

carmocca Jun 29, 2021

Choose a reason for hiding this comment

awaelchli Jun 29, 2021

Choose a reason for hiding this comment

carmocca Jun 29, 2021

Choose a reason for hiding this comment

tchaton Jun 29, 2021 • edited Loading

Choose a reason for hiding this comment

tchaton commented Jun 28, 2021 •

edited

Loading

pep8speaks commented Jun 28, 2021 •

edited

Loading

codecov bot commented Jun 28, 2021 •

edited

Loading

tchaton commented Jun 28, 2021 •

edited

Loading

tchaton Jun 29, 2021 •

edited

Loading