Skip to content

Test case for c10d DDP #9670

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
goldsborough wants to merge 4 commits into from
Closed

Test case for c10d DDP #9670

goldsborough wants to merge 4 commits into from

Conversation

@goldsborough
Copy link
Contributor

@goldsborough goldsborough commented Jul 21, 2018 •
edited
Loading

Before I can rewrite portions of the c10d DDP in C++ I need proper tests in place to make sure I am not breaking anything as I port code. There were no tests for the c10d DDP in place so I wrote some.

I refactored the c10d tests to derive some tests cases from a general MultiGPUTestCase and followed lots of patterns from test_distributed.py w.r.t. how tests are skipped (such that the main process doesn't initialize CUDA, which I found is a super important detail!!!).

I am largely unfamiliar with this code so feel free to scrutinize. The DDP test code itself is also largely taken from test_distributed.py but more inlined which I find easier to read.

Test Plan:

  1. Ran gloo backend tests successfully on my devgpu
  2. Ran NCCL (and gloo) backend tests on devfair successfully.

@apaszke @pietern @teng-li

ssnl
ssnl reviewed Jul 21, 2018
View reviewed changes
test/test_c10d.py Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

@soumith
Copy link
Contributor

soumith commented Jul 21, 2018

@pytorchbot retest this please

@ezyang
Copy link
Contributor

ezyang commented Jul 23, 2018

test failures look real

05:04:25 ======================================================================
05:04:25 FAIL: test_nccl_backend (__main__.DistributedDataParallelTest)
05:04:25 ----------------------------------------------------------------------
05:04:25 Traceback (most recent call last):
05:04:25   File "test_c10d.py", line 190, in wrapper
05:04:25     self._join_processes(fn)
05:04:25   File "test_c10d.py", line 235, in _join_processes
05:04:25     self._check_return_codes()
05:04:25   File "test_c10d.py", line 244, in _check_return_codes
05:04:25     self.assertEqual(p.exitcode, first_process.exitcode)
05:04:25   File "/var/lib/jenkins/workspace/test/common.py", line 364, in assertEqual
05:04:25     super(TestCase, self).assertEqual(x, y, message)
05:04:25 AssertionError: None != 1 :

@goldsborough goldsborough mentioned this pull request Jul 23, 2018
Closed
@goldsborough
Copy link
Contributor Author

goldsborough commented Jul 23, 2018

@teng-li @apaszke looking for quick feedback so that I can post more PRs that need this test

apaszke
apaszke approved these changes Jul 24, 2018
View reviewed changes
test/test_c10d.py Outdated

This comment was marked as off-topic.

Sign in to view

test/test_c10d.py Outdated

This comment was marked as off-topic.

Sign in to view

test/test_c10d.py Outdated

This comment was marked as off-topic.

Sign in to view

test/test_c10d.py Outdated

This comment was marked as off-topic.

Sign in to view

test/test_c10d.py Outdated

This comment was marked as off-topic.

Sign in to view

test/test_c10d.py Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

facebook-github-bot
facebook-github-bot reviewed Jul 24, 2018
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@goldsborough has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@avmgithub avmgithub mentioned this pull request Jul 26, 2018
Closed
jramseyer pushed a commit to jramseyer/pytorch that referenced this pull request Jul 30, 2018
@goldsborough @jramseyer
Test case for c10d DDP (pytorch#9670)
c411c95 
Summary:
Before I can rewrite portions of the c10d DDP in C++ I need proper tests in place to make sure I am not breaking anything as I port code. There were no tests for the c10d DDP in place so I wrote some.

I refactored the c10d tests to derive some tests cases from a general `MultiGPUTestCase` and followed lots of patterns from `test_distributed.py` w.r.t. how tests are skipped (such that the main process doesn't initialize CUDA, which I found is a super important detail!!!).

I am largely unfamiliar with this code so feel free to scrutinize. The DDP test code itself is also largely taken from `test_distributed.py` but more inlined which I find easier to read.
Pull Request resolved: pytorch#9670

Differential Revision: D8977724

Pulled By: goldsborough

fbshipit-source-id: 186eab38a72384d7992a2ec5c89f304ad42d5944
facebook-github-bot pushed a commit that referenced this pull request Aug 1, 2018
@goldsborough @facebook-github-bot
Move DDP broadcast coalesced to C++ (#9729)
7d2bda7 
Summary:
This PR depends on the tests added in #9670. It moves the first, tiny function from the c10d DDP to C++: `dist_broadcast_coalesced`. Let me know if ` torch/csrc/distributed/c10d/ddp.h` will be a good place to put these rewritten functions.

pietern The controller you requested could not be found. apaszke
Pull Request resolved: #9729

Differential Revision: D8985308

Pulled By: goldsborough

fbshipit-source-id: dc459fe9040273714044152063585e746974752f
goodlux pushed a commit to goodlux/pytorch that referenced this pull request Aug 15, 2018
@goldsborough
Test case for c10d DDP (pytorch#9670)
811d1c4 
Summary:
Before I can rewrite portions of the c10d DDP in C++ I need proper tests in place to make sure I am not breaking anything as I port code. There were no tests for the c10d DDP in place so I wrote some.

I refactored the c10d tests to derive some tests cases from a general `MultiGPUTestCase` and followed lots of patterns from `test_distributed.py` w.r.t. how tests are skipped (such that the main process doesn't initialize CUDA, which I found is a super important detail!!!).

I am largely unfamiliar with this code so feel free to scrutinize. The DDP test code itself is also largely taken from `test_distributed.py` but more inlined which I find easier to read.
Pull Request resolved: pytorch#9670

Differential Revision: D8977724

Pulled By: goldsborough

fbshipit-source-id: 186eab38a72384d7992a2ec5c89f304ad42d5944
goodlux pushed a commit to goodlux/pytorch that referenced this pull request Aug 15, 2018
@goldsborough
Move DDP broadcast coalesced to C++ (pytorch#9729)
838a2ed 
Summary:
This PR depends on the tests added in pytorch#9670. It moves the first, tiny function from the c10d DDP to C++: `dist_broadcast_coalesced`. Let me know if ` torch/csrc/distributed/c10d/ddp.h` will be a good place to put these rewritten functions.

pietern The controller you requested could not be found. apaszke
Pull Request resolved: pytorch#9729

Differential Revision: D8985308

Pulled By: goldsborough

fbshipit-source-id: dc459fe9040273714044152063585e746974752f
@ezyang ezyang added the merged label Jun 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ssnl ssnl ssnl left review comments

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

@pietern pietern Awaiting requested review from pietern

@teng-li teng-li Awaiting requested review from teng-li

+1 more reviewer

@apaszke apaszke apaszke approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@goldsborough @soumith @ezyang @apaszke @ssnl @facebook-github-bot