Skip to content

Single-process multi-node CPU training#9603

Merged
carmocca merged 23 commits intoLightning-AI:masterfrom
borchero:patch-2
Oct 14, 2021
Merged

Single-process multi-node CPU training#9603
carmocca merged 23 commits intoLightning-AI:masterfrom
borchero:patch-2

Conversation

@borchero
Copy link
Copy Markdown
Contributor

@borchero borchero commented Sep 20, 2021 •
edited by awaelchli
Loading

What does this PR do?

Whenever a model is small, it is not necessarily useful to use a GPU. However, one might still benefit from distributing computations for model training across nodes -- especially when you consider that containers are also "nodes".

This PR sets DistributedType.DDP instead of DistributedType.DDP_SPAWN whenever DistributedType.DDP_CPU is set and only a single process or a single process per node is used.

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

  • Minor breaking change: whenever one requests a num_nodes > 1 and sets num_processes = None on the Trainer, num_processes now defaults to 1 and DistributedType.DDP is used. Before, num_processes was set to os.cpu_count() and DistributedType.DDP_SPAWN was used. However, it is unlikely that people rely on that behavior, especially since num_processes defaults to 1.

One could decide to keep the old behavior, however, I would argue that it is far more likely to only want a single process when distributing across nodes (think containers).

Fixes #9877

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

@stale stale Bot added the won't fix This will not be worked on label Oct 5, 2021
@Lightning-AI Lightning-AI deleted a comment from stale Bot Oct 5, 2021
@stale stale Bot removed the won't fix This will not be worked on label Oct 5, 2021
@borchero borchero mentioned this pull request Oct 9, 2021
Closed
@awaelchli awaelchli added distributed Generic distributed-related topic feature Is an improvement or enhancement labels Oct 9, 2021
@awaelchli awaelchli added this to the v1.5 milestone Oct 10, 2021
awaelchli
awaelchli suggested changes Oct 10, 2021
View reviewed changes
Comment thread CHANGELOG.md Outdated
Comment thread pytorch_lightning/trainer/connectors/accelerator_connector.py Outdated
Comment thread pytorch_lightning/trainer/trainer.py Outdated
Comment thread tests/accelerators/test_accelerator_connector.py Outdated
@awaelchli
Copy link
Copy Markdown
Contributor

awaelchli commented Oct 10, 2021

Thanks for the PR. LGTM, just a few changes requested.

borchero and others added 3 commits October 10, 2021 22:22
@borchero @awaelchli
Update CHANGELOG.md
86b1d2a 
Co-authored-by: Adrian Wälchli <aedu.waelchli@gmail.com>
@borchero
Update changelog
f1fc767
@borchero @awaelchli
Update pytorch_lightning/trainer/connectors/accelerator_connector.py
15a6f4e 
Co-authored-by: Adrian Wälchli <aedu.waelchli@gmail.com>
@borchero
Copy link
Copy Markdown
Contributor Author

borchero commented Oct 10, 2021

Implemented the changes, thanks for the pointers @awaelchli :)

awaelchli
awaelchli approved these changes Oct 10, 2021
View reviewed changes
Comment thread pytorch_lightning/trainer/connectors/accelerator_connector.py Outdated
Comment thread tests/accelerators/test_accelerator_connector.py Outdated
Comment thread docs/source/common/trainer.rst Outdated
@awaelchli
Copy link
Copy Markdown
Contributor

awaelchli commented Oct 10, 2021

Thanks @borchero <3

@mergify mergify Bot removed the has conflicts label Oct 11, 2021
rohitgr7
rohitgr7 reviewed Oct 11, 2021
View reviewed changes
Comment thread tests/accelerators/test_accelerator_connector.py Outdated
SkafteNicki
SkafteNicki reviewed Oct 11, 2021
View reviewed changes
Comment thread tests/accelerators/test_accelerator_connector.py Outdated
@borchero
Copy link
Copy Markdown
Contributor Author

borchero commented Oct 14, 2021

Don't think the failing test is caused by my changes.

@mergify mergify Bot added the ready to be merged PRs ready to be merged label Oct 14, 2021
@codecov
Copy link
Copy Markdown

codecov Bot commented Oct 14, 2021 •
edited
Loading

Codecov Report

Merging #9603 (146123b) into master (6feda08) will decrease coverage by 4%.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master   #9603    +/-   ##
=======================================
- Coverage      93%     89%    -4%     
=======================================
  Files         179     179            
  Lines       15805   15807     +2     
=======================================
- Hits        14648   14029   -619     
- Misses       1157    1778   +621

rohitgr7
rohitgr7 reviewed Oct 14, 2021
View reviewed changes
Comment thread tests/accelerators/test_accelerator_connector.py Outdated
rohitgr7
rohitgr7 approved these changes Oct 14, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@rohitgr7 rohitgr7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice work!

@carmocca carmocca merged commit afbf703 into Lightning-AI:master Oct 14, 2021
rohitgr7 pushed a commit to Tshimanga/pytorch-lightning that referenced this pull request Oct 18, 2021
@borchero @awaelchli
@tchaton @rohitgr7
Single-process multi-node CPU training (Lightning-AI#9603)
07ba0b9 
Co-authored-by: Adrian Wälchli <aedu.waelchli@gmail.com>
Co-authored-by: thomas chaton <thomas@grid.ai>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@SkafteNicki SkafteNicki SkafteNicki left review comments

@Borda Borda Awaiting requested review from Borda

@SeanNaren SeanNaren Awaiting requested review from SeanNaren

@tchaton tchaton Awaiting requested review from tchaton tchaton is a code owner

@justusschock justusschock Awaiting requested review from justusschock justusschock is a code owner

@kaushikb11 kaushikb11 Awaiting requested review from kaushikb11

@williamFalcon williamFalcon Awaiting requested review from williamFalcon

@edenlightning edenlightning Awaiting requested review from edenlightning

+3 more reviewers

@awaelchli awaelchli awaelchli approved these changes

@carmocca carmocca carmocca approved these changes

@rohitgr7 rohitgr7 rohitgr7 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@awaelchli awaelchli

Labels

distributed Generic distributed-related topic feature Is an improvement or enhancement ready to be merged PRs ready to be merged

Projects

None yet

Milestone

pl:1.5

Development

Successfully merging this pull request may close these issues.

Single-process multi-node CPU training

6 participants

@borchero @awaelchli @carmocca @SkafteNicki @rohitgr7 @tchaton