Skip reconciliate_processes if used within a cluster environment that creates processes externally by ananthsub · Pull Request #9389 · Lightning-AI/pytorch-lightning

ananthsub · 2021-09-09T01:55:06Z

What does this PR do?

Does your PR introduce any breaking changes? If yes, please list them.

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 list all the breaking changes introduced by this pull request?
Did you update the CHANGELOG? (not for typos, docs, test updates, or internal minor changes/refactorings)

PR review

Anyone in the community is welcome to review the PR.
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 🙃

awaelchli

req. review from @tchaton

codecov · 2021-09-10T00:58:52Z

Codecov Report

Merging #9389 (d1804bc) into master (81687aa) will decrease coverage by 4%.
The diff coverage is 0%.

❗ Current head d1804bc differs from pull request most recent head 93b389a. Consider uploading reports for the commit 93b389a to get more accurate results

@@           Coverage Diff           @@
##           master   #9389    +/-   ##
=======================================
- Coverage      93%     89%    -4%     
=======================================
  Files         180     180            
  Lines       14965   15065   +100     
=======================================
- Hits        13904   13370   -534     
- Misses       1061    1695   +634

tchaton

LGTM !

ananthsub · 2021-09-10T20:46:54Z

@tchaton @awaelchli I'm not sure why this test is failing:

are special tests run with torch.distributed.run or torchelastic somehow? it doesn't seem like this is the case with https://github.com/PyTorchLightning/pytorch-lightning/blob/15434a9c357745c8f6df5739c6a423bbb03787ad/tests/special_tests.sh#L20-L65

tchaton · 2021-09-11T13:52:29Z

Hey @ananthsub. Interesting. Mind taking CustomException out of the test ? The test should run as special one, so without TorchElastic.

ananthsub · 2021-09-11T21:24:46Z

ah @tchaton @awaelchli - I believe it's due to the check in the LightningEnvironment here: https://github.com/PyTorchLightning/pytorch-lightning/blob/b294c5760eee30a995fcd400127be209b12c4d7c/pytorch_lightning/plugins/environments/lightning_environment.py#L43-L49

LOCAL_RANK is added to the environment here: https://github.com/PyTorchLightning/pytorch-lightning/blob/b294c5760eee30a995fcd400127be209b12c4d7c/pytorch_lightning/plugins/training_type/ddp.py#L198

So while cluster_environment.creates_children() is False at the beginning of the script, it is True at the point of deadlock detection. This leads to the deadlock detection being disabled, causing the test to fail.

This was added in #7480

awaelchli · 2021-09-11T22:37:19Z

So while cluster_environment.creates_children() is False at the beginning of the script, it is True at the point of deadlock detection. This leads to the deadlock detection being disabled, causing the test to fail.

This was added in #7480

You are right, I should have captured the value like you do in this PR

tchaton · 2021-09-15T08:10:35Z

Hey @ananthsub,

Do you see blockers to introduce an env variable to enable Lightning to kill the processes even with a cluster env.

Something like PL_PREVENT_DEADLOCK_MECHANISM='1'

I believe this would be useful for Grid.ai.

Best,
T.C

ananthsub · 2021-09-15T09:00:41Z

Hey @ananthsub,

Do you see blockers to introduce an env variable to enable Lightning to kill the processes even with a cluster env.

Something like PL_PREVENT_DEADLOCK_MECHANISM='1'

I believe this would be useful for Grid.ai.

Best,
T.C

@tchaton @awaelchli could you take another look to confirm the opt-in deadlock detection looks reasonable?

tchaton

LGTM !

… creates processes externally (#9389) * [RFC] Skip reconciliate_processes if used within a cluster environment that creates processes externally

ananthsub added bug Something isn't working distributed Generic distributed-related topic labels Sep 9, 2021

ananthsub changed the title ~~[RFC] Skip reconciliate_processes if used within a cluster environment that creates processes externally~~ Skip reconciliate_processes if used within a cluster environment that creates processes externally Sep 9, 2021

ananthsub marked this pull request as ready for review September 9, 2021 21:19

ananthsub requested review from Borda, SeanNaren, awaelchli, carmocca, justusschock, kaushikb11, tchaton and williamFalcon as code owners September 9, 2021 21:19

ananthsub added this to the v1.4.x milestone Sep 9, 2021

carmocca approved these changes Sep 9, 2021

View reviewed changes

awaelchli previously approved these changes Sep 10, 2021

View reviewed changes

Comment thread CHANGELOG.md Outdated

mergify Bot added the ready to be merged PRs ready to be merged label Sep 10, 2021

mergify Bot added the has conflicts label Sep 10, 2021

tchaton approved these changes Sep 10, 2021

View reviewed changes

mergify Bot added has conflicts and removed has conflicts labels Sep 10, 2021

ananthsub enabled auto-merge (squash) September 10, 2021 15:39

ananthsub disabled auto-merge September 11, 2021 18:34

ananthsub mentioned this pull request Sep 11, 2021

ClusterEnvironment interface and improvements #6303

Closed

8 tasks

ananthsub commented Sep 11, 2021

View reviewed changes

Comment thread pytorch_lightning/plugins/environments/lightning_environment.py Outdated

mergify Bot removed the has conflicts label Sep 15, 2021

ananthsub added 2 commits September 14, 2021 23:58

Update lightning_environment.py

f5805ce

Update ddp.py

9174413

ananthsub commented Sep 15, 2021

View reviewed changes

Comment thread pytorch_lightning/plugins/training_type/ddp.py Outdated

Comment thread pytorch_lightning/plugins/training_type/ddp.py Outdated

ananthsub added 2 commits September 15, 2021 00:13

Update ddp.py

d11b3ae

Update ddp.py

ca48d22

ananthsub commented Sep 15, 2021

View reviewed changes

Comment thread pytorch_lightning/plugins/environments/lightning_environment.py Outdated

ananthsub added 4 commits September 15, 2021 00:15

Update lightning_environment.py

7ee7b7f

Update ddp.py

dda119e

Update ddp.py

f96dba9

Update lightning_environment.py

e5afff7

tchaton requested a review from awaelchli September 15, 2021 08:06

awaelchli approved these changes Sep 15, 2021

View reviewed changes

Comment thread pytorch_lightning/plugins/training_type/ddp.py Outdated

Comment thread pytorch_lightning/plugins/training_type/ddp.py Outdated

mergify Bot added the ready to be merged PRs ready to be merged label Sep 15, 2021

ananthsub added 2 commits September 15, 2021 01:56

add env

6448a5a

Update ddp.py

e5b2801

awaelchli reviewed Sep 15, 2021

View reviewed changes

Comment thread pytorch_lightning/plugins/training_type/ddp.py Outdated

Update ddp.py

18c209b

tchaton approved these changes Sep 15, 2021

View reviewed changes

tchaton reviewed Sep 15, 2021

View reviewed changes

Comment thread CHANGELOG.md

Update CHANGELOG.md

377e7cf

ananthsub commented Sep 15, 2021

View reviewed changes

Comment thread CHANGELOG.md

awaelchli reviewed Sep 15, 2021

View reviewed changes

Comment thread pytorch_lightning/plugins/training_type/ddp.py

Comment thread pytorch_lightning/plugins/training_type/ddp.py

Comment thread pytorch_lightning/plugins/training_type/ddp.py Outdated

Update ddp.py

10a36a8

ananthsub merged commit 08b591f into Lightning-AI:master Sep 15, 2021

Conversation

ananthsub commented Sep 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Does your PR introduce any breaking changes? If yes, please list them.

Before submitting

PR review

Did you have fun?

Uh oh!

awaelchli left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov Bot commented Sep 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tchaton left a comment

Choose a reason for hiding this comment

Uh oh!

ananthsub commented Sep 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tchaton commented Sep 11, 2021

Uh oh!

ananthsub commented Sep 11, 2021

Uh oh!

Uh oh!

awaelchli commented Sep 11, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tchaton commented Sep 15, 2021

Uh oh!

Uh oh!

Uh oh!

ananthsub commented Sep 15, 2021

Uh oh!

Uh oh!

tchaton left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ananthsub commented Sep 9, 2021 •

edited

Loading

codecov Bot commented Sep 10, 2021 •

edited

Loading

ananthsub commented Sep 10, 2021 •

edited

Loading