Add Sagemaker DDP Plugin #6271

kaushikb11 · 2021-03-01T19:14:35Z

What does this PR do?

Add Sagemaker DDP Plugin. Reference

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 🙃

codecov · 2021-03-01T19:16:24Z

Codecov Report

Merging #6271 (4a0f78d) into master (8193bae) will decrease coverage by 7%.
The diff coverage is 39%.

❗ Current head 4a0f78d differs from pull request most recent head 1ac88a8. Consider uploading reports for the commit 1ac88a8 to get more accurate results

@@           Coverage Diff            @@
##           master   #6271     +/-   ##
========================================
- Coverage      93%     86%     -7%     
========================================
  Files         212     216      +4     
  Lines       13720   14070    +350     
========================================
- Hits        12751   12035    -716     
- Misses        969    2035   +1066

tchaton

Overall looks good !
Questions:

I think you can subclass DDPPlugin directly.
What about DDPSpawn too ?
And Should we add an example with Sagemaker API ?

tchaton · 2021-03-02T17:02:44Z

pytorch_lightning/plugins/environments/smdist_environment.py

+
+    def __init__(self):
+        if not _SMDIST_AVAILABLE:
+            raise MisconfigurationException("`smdistributed` module is not available.")


Add a small description on how to make this work.

Suggested change

raise MisconfigurationException("`smdistributed` module is not available.")

raise MisconfigurationException("`smdistributed` package is not available.")

also add how to instal it

pytorch_lightning/plugins/training_type/smddp.py

pytorch_lightning/utilities/enums.py

pytorch_lightning/trainer/connectors/accelerator_connector.py

justusschock

Like it just a few minor comments

pytorch_lightning/plugins/environments/smdist_environment.py

pytorch_lightning/plugins/training_type/smddp.py

justusschock · 2021-03-08T08:53:07Z

pytorch_lightning/plugins/training_type/smddp.py

+
+        self.sync_batchnorm = sync_batchnorm
+        self.dist = SMLightningDistributed()
+        self.num_nodes = len(os.environ['SM_HOSTS'])


Is this something we maybe should extend the Environment class by?

cc @awaelchli

pytorch_lightning/utilities/enums.py

pep8speaks · 2021-07-03T19:13:23Z

Hello @kaushikb11! Thanks for updating this PR.

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

Comment last updated at 2021-07-14 12:54:55 UTC

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

tchaton

LGTM !

Borda · 2021-07-08T09:23:33Z

pytorch_lightning/plugins/training_type/ddp_sm.py

+                "`smdistributed` module is not available."
+                " You would need to enable distributed=smdistributed"
+                " in the Sagemaker Estimator Object."


Suggested change

"`smdistributed` module is not available."

" You would need to enable distributed=smdistributed"

" in the Sagemaker Estimator Object."

"`smdistributed` package is not available."

" You would need to enable `distributed=smdistributed` in the Sagemaker Estimator Object."

Borda · 2021-07-08T09:24:13Z

pytorch_lightning/plugins/training_type/ddp_sm.py

+        if not _SMDIST_AVAILABLE:
+            raise MisconfigurationException(
+                "`smdistributed` module is not available."
+                " You would need to enable distributed=smdistributed"


do you mean adding to to DDPSMPlugin or where?

Borda · 2021-07-08T09:25:27Z

pytorch_lightning/plugins/environments/smdist_environment.py

+
+    def __init__(self):
+        if not _SMDIST_AVAILABLE:
+            raise MisconfigurationException("`smdistributed` module is not available.")


Suggested change

raise MisconfigurationException("`smdistributed` module is not available.")

raise MisconfigurationException("`smdistributed` package is not available.")

also add how to instal it

Borda · 2021-07-08T09:28:23Z

pytorch_lightning/plugins/training_type/ddp_sm.py

+            log.info("-" * 100)
+            log.info(f"distributed_backend={self.distributed_backend}")
+            log.info(f"All DDP processes registered. Starting ddp with {self.world_size} processes")
+            log.info("-" * 100)


Suggested change

log.info("-" * 100)

log.info(f"distributed_backend={self.distributed_backend}")

log.info(f"All DDP processes registered. Starting ddp with {self.world_size} processes")

log.info("-" * 100)

log.info(

"-" * 100 + '\n',

f"distributed_backend={self.distributed_backend}" + '\n',

f"All DDP processes registered. Starting ddp with {self.world_size} processes" + '\n',

"-" * 100,

)

kaushikb11 · 2021-07-08T09:53:35Z

Converted the PR to draft, because seems like certain functionalities have changed and are breaking from Sagemaker's side.

sai-prasanna · 2021-08-24T18:10:11Z

pytorch_lightning/distributed/dist.py

+
+class SMLightningDistributed(LightningDistributed):
+
+    def broadcast(self, obj: Any, group=sm_dist.group.WORLD):


sm_dist is conditionally imported. So this line fails when it's not present in cases where you train without that package.

devashish-khatwani · 2021-08-31T07:38:07Z

Is there an ETA for this feature to be available?

lballes · 2022-08-09T08:42:37Z

Are there any plans to pick this up again? It seems that Sagemaker DDP now functions as a backend for torch.distributed, so it might be simpler to integrate now?

carmocca · 2022-08-09T14:55:45Z

@lballes Thank you for the heads up! Then we support it automatically just with:

from pytorch_lightning.strategies import DDPStrategy

# sagemaker backend: https://docs.aws.amazon.com/sagemaker/latest/dg/data-parallel-modify-sdp-pt.html
import smdistributed.dataparallel.torch.torch_smddp
ddp = DDPStrategy(process_group_backend="smddp")

# Configure the strategy on the Trainer
trainer = Trainer(strategy=ddp, accelerator="gpu", devices=8)

edit: also, a blogpost! https://aws.amazon.com/blogs/machine-learning/run-pytorch-lightning-and-native-pytorch-ddp-on-amazon-sagemaker-training-featuring-amazon-search/

The sagemaker release has deprecated all the constructs in this PR, so we can close it.

kaushikb11 added 7 commits March 1, 2021 23:00

feat: add smddp plugin & environment

ad77ff2

update accelerator connector

270e4df

update smddp plugin

33c9891

update smddp plugin

0380efb

update lightning distributed

c115ece

fix typo

50c045d

update smddp plugin

eab4f58

kaushikb11 requested a review from tchaton March 1, 2021 19:15

kaushikb11 added 3 commits March 2, 2021 00:55

add comment for parallel devices

f233b1a

debug

2e9280c

debug

c91e9f2

Borda requested review from Borda and SeanNaren March 2, 2021 17:00

tchaton reviewed Mar 2, 2021

View reviewed changes

carmocca reviewed Mar 2, 2021

View reviewed changes

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

pytorch_lightning/trainer/connectors/accelerator_connector.py Outdated Show resolved Hide resolved

ddp name consistency

a13a675

justusschock approved these changes Mar 8, 2021

View reviewed changes

kaushikb11 and others added 11 commits March 8, 2021 14:44

fix global rank

88b2b4b

update smdist environment

ffc85ea

Update Type Annotation

833bb57

DDP plugin as base class

467e76b

add test

c2a508c

Merge branch 'master' into smddp

f5a2cf4

add creates_children mthod

f5675e8

change backend to mpi

0dff5c7

set broadcast buffers set to False

eacf9a8

mini refactor

d1bf909

address reviews

da66a19

kaushikb11 marked this pull request as ready for review March 10, 2021 11:34

mergify bot removed the has conflicts label Jul 3, 2021

pre-commit-ci bot and others added 2 commits July 3, 2021 19:14

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

13dac0b

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

Fix circular import

e38fb37

tchaton approved these changes Jul 8, 2021

View reviewed changes

Borda requested review from carmocca, SeanNaren and ananthsub July 8, 2021 09:19

Borda reviewed Jul 8, 2021

View reviewed changes

kaushikb11 marked this pull request as draft July 8, 2021 09:52

kaushikb11 added 8 commits July 8, 2021 18:40

Update environment

7797770

Add set_world_ranks

4a0f78d

Add updates

574887f

Add updates

19fa542

Fix broadcasting

5699a32

Fix broadcasting

7a715a0

Update group

e43fc1b

Update logger

1ac88a8

Borda modified the milestones: v1.4, v1.5 Jul 19, 2021

sai-prasanna reviewed Aug 24, 2021

View reviewed changes

awaelchli modified the milestones: v1.5, v1.6 Nov 1, 2021

carmocca removed this from the 1.6 milestone Mar 28, 2022

carmocca closed this Aug 9, 2022

SeanNaren mentioned this pull request Feb 9, 2023

Add AWS SageMaker ASR Examples NVIDIA/NeMo#5638

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Sagemaker DDP Plugin #6271

Add Sagemaker DDP Plugin #6271

kaushikb11 commented Mar 1, 2021

codecov bot commented Mar 1, 2021 •

edited

Loading

tchaton left a comment

tchaton Mar 2, 2021

kaushikb11 Mar 10, 2021

Borda Jul 8, 2021

justusschock left a comment

justusschock Mar 8, 2021

pep8speaks commented Jul 3, 2021 •

edited

Loading

tchaton left a comment

Borda Jul 8, 2021

Borda Jul 8, 2021

Borda Jul 8, 2021

Borda Jul 8, 2021

kaushikb11 commented Jul 8, 2021

sai-prasanna Aug 24, 2021

devashish-khatwani commented Aug 31, 2021

lballes commented Aug 9, 2022

carmocca commented Aug 9, 2022 •

edited

Loading

	raise MisconfigurationException("`smdistributed` module is not available.")
	raise MisconfigurationException("`smdistributed` package is not available.")


		class SMLightningDistributed(LightningDistributed):

		def broadcast(self, obj: Any, group=sm_dist.group.WORLD):

Add Sagemaker DDP Plugin #6271

Add Sagemaker DDP Plugin #6271

Conversation

kaushikb11 commented Mar 1, 2021

What does this PR do?

Before submitting

PR review

Did you have fun?

codecov bot commented Mar 1, 2021 • edited Loading

Codecov Report

tchaton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justusschock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pep8speaks commented Jul 3, 2021 • edited Loading

Comment last updated at 2021-07-14 12:54:55 UTC

tchaton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaushikb11 commented Jul 8, 2021

Choose a reason for hiding this comment

devashish-khatwani commented Aug 31, 2021

lballes commented Aug 9, 2022

carmocca commented Aug 9, 2022 • edited Loading

codecov bot commented Mar 1, 2021 •

edited

Loading

pep8speaks commented Jul 3, 2021 •

edited

Loading

carmocca commented Aug 9, 2022 •

edited

Loading