Add RpcAgentTestFixture to extract duplicate code #29747

xush6528 · 2019-11-13T20:54:23Z

Stack from ghstack:

Add RpcAgentTestFixture to extract duplicate code #29747 Add RpcAgentTestFixture to extract duplicate code

There are duplicate code for component that rely on RpcAgent. Extract them into a re-usable test fixture class.

Differential Revision: D5689636

We plan to introduce different `RpcAgentOption`s, this requires us to allow different tests for differnet `RpcAgent`s to correctly configure the test by bringing in different test mixins. This refactoring PR is empowering this. Also, with more fixtures and mixins, moved tests into sub-folders. Differential Revision: [D5689636](https://our.internmc.facebook.com/intern/diff/D5689636/) [ghstack-poisoned]

… Fixtures" We plan to introduce different `RpcAgentOption`s, this requires us to allow different tests for differnet `RpcAgent`s to correctly configure the test by bringing in different test mixins. This refactoring PR is empowering this. Also, with more fixtures and mixins, moved tests into sub-folders. Differential Revision: [D5689636](https://our.internmc.facebook.com/intern/diff/D5689636/) [ghstack-poisoned]

Pull Request resolved: #29747 We plan to introduce different `RpcAgentOption`s, this requires us to allow different tests for differnet `RpcAgent`s to correctly configure the test by bringing in different test mixins. This refactoring PR is empowering this. Also, with more fixtures and mixins, moved tests into sub-folders. Differential Revision: [D5689636](https://our.internmc.facebook.com/intern/diff/D5689636/) ghstack-source-id: 93857085

… Fixtures" We plan to introduce different `RpcAgentOption`s, this requires us to allow different tests for differnet `RpcAgent`s to correctly configure the test by bringing in different test mixins. This refactoring PR is empowering this. Also, with more fixtures and mixins, moved tests into sub-folders. Differential Revision: [D5689636](https://our.internmc.facebook.com/intern/diff/D5689636/) [ghstack-poisoned]

Pull Request resolved: #29747 We plan to introduce different `RpcAgentOption`s, this requires us to allow different tests for differnet `RpcAgent`s to correctly configure the test by bringing in different test mixins. This refactoring PR is empowering this. Also, with more fixtures and mixins, moved tests into sub-folders. Differential Revision: [D5689636](https://our.internmc.facebook.com/intern/diff/D5689636/) ghstack-source-id: 93859285

… Fixtures" We plan to introduce different `RpcAgentOption`s, this requires us to allow different tests for differnet `RpcAgent`s to correctly configure the test by bringing in different test mixins. This refactoring PR is empowering this. Also, with more fixtures and mixins, moved tests into sub-folders. Differential Revision: [D5689636](https://our.internmc.facebook.com/intern/diff/D5689636/) [ghstack-poisoned]

Pull Request resolved: #29747 We plan to introduce different `RpcAgentOption`s, this requires us to allow different tests for differnet `RpcAgent`s to correctly configure the test by bringing in different test mixins. This refactoring PR is empowering this. Also, with more fixtures and mixins, moved tests into sub-folders. Differential Revision: [D5689636](https://our.internmc.facebook.com/intern/diff/D5689636/) ghstack-source-id: 93868349

mrshenli · 2019-11-14T00:18:48Z

test/run_test.py

    for exclude_test in exclude_list:
        for test in tests_copy:
-            if test.startswith(exclude_test):
+            if test == exclude_test:


why changing this to exact match?

Notice line 85,

WINDOWS_BLACKLIST = [ 'distributed', # line 85 'distributed/rpc/process_group/rpc_fork', ]

Initially selected_tests is like,

selected_tests = [ 'distributed', 'distributed/rpc/process_group/rpc_fork', ]

After iterating on exclude_test == 'distributed', selected_tests becomes,

selected_tests = [ 'distributed/rpc/process_group/rpc_fork', ]

Since the exclude_tests is,

def exclude_tests(exclude_list, selected_tests, exclude_message=None): tests_copy = selected_tests[:] for exclude_test in exclude_list: for test in tests_copy: if test.startswith(exclude_test): if exclude_message is not None: print_to_stderr('Excluding {} {}'.format(test, exclude_message)) selected_tests.remove(test) return selected_tests

when
test == 'distributed/rpc/process_group/rpc_fork',
and
exclude_test == 'distributed'

It will run selected_tests.remove('distributed'), raising a "list does not contain 'distributed'. " error.

mrshenli · 2019-11-14T00:22:45Z

test/run_test.py


    for test in selected_tests:
-        test_name = 'test_{}'.format(test)
+        splits = test.rsplit("/", 1)


jit does this by adding a test_jit.py in the torch/test folder and then import other tests from there. I wonder if we should do the same.

cc @yf225 @ezyang @suo

mrshenli · 2019-11-14T00:25:03Z

I tagged @ezyang because this PR touches run_test.py that might affect other modules test. Please let us know if we should keep the same convention as test_jit.py and only add minimum changes to run_test.py.

mrshenli · 2019-11-14T00:32:44Z

test/distributed/rpc/process_group/process_group_rpc_agent_mixin.py

+
+class ProcessGroupRpcAgentMixin(object):
+    @property
+    def rpc_backend_name(self):


will there be any other helper function be added to this class? If this is the only one, this looks like an overkill for just passing a string to a test class.

Yes, it's def rpc_agent_options in the next PR. Adding this Mixin + refactoring for this Mixin is the purpose of this PR.

… Fixtures" We plan to introduce different `RpcAgentOption`s, this requires us to allow different tests for differnet `RpcAgent`s to correctly configure the test by bringing in different test mixins. This refactoring PR is empowering this. Also, with more fixtures and mixins, moved tests into sub-folders. Differential Revision: [D5689636](https://our.internmc.facebook.com/intern/diff/D5689636/) [ghstack-poisoned]

Pull Request resolved: #29747 We plan to introduce different `RpcAgentOption`s, this requires us to allow different tests for differnet `RpcAgent`s to correctly configure the test by bringing in different test mixins. This refactoring PR is empowering this. Also, with more fixtures and mixins, moved tests into sub-folders. Differential Revision: [D5689636](https://our.internmc.facebook.com/intern/diff/D5689636/) ghstack-source-id: 93885275

ezyang · 2019-11-14T16:10:16Z

test/run_test.py

+    os.environ['PYTHONPATH'] = os.pathsep.join(
+        [path for path in [os.environ.get('PYTHONPATH', ''), test_directory] if path]
+    )
+


I think this is silly and you should just keep all the tests in the top level directory. If you do this, will running tests directly from the directory (without using run_test.py) work? I don't think so! And now you've just made it that much harder for people to run a specific test with, e.g., gdb attached.

@pietern Agree? Let's put them flatly?

ezyang

I'm NACK'ing the run_test.py changes. An important invariant I want to hold is that I can run python test/test_foo.py without anything special, and I will run that test. The multi-level folder structure that you've setup here causes this property to no longer hold. I don't think multiple folders is worth losing the invariant.

… Fixtures" We plan to introduce different `RpcAgentOption`s, this requires us to allow different tests for differnet `RpcAgent`s to correctly configure the test by bringing in different test mixins. This refactoring PR is empowering this. Also, with more fixtures and mixins, moved tests into sub-folders. Differential Revision: [D5689636](https://our.internmc.facebook.com/intern/diff/D5689636/) [ghstack-poisoned]

Pull Request resolved: #29747 We plan to introduce different `RpcAgentOption`s, this requires us to allow different tests for differnet `RpcAgent`s to correctly configure the test by bringing in different test mixins. This refactoring PR is empowering this. Also, with more fixtures and mixins, moved tests into sub-folders. Differential Revision: [D5689636](https://our.internmc.facebook.com/intern/diff/D5689636/) ghstack-source-id: 93890260

… Fixtures" We plan to introduce different `RpcAgentOption`s, this requires us to allow different tests for differnet `RpcAgent`s to correctly configure the test by bringing in different test mixins. This refactoring PR is empowering this. Also, with more fixtures and mixins, moved tests into sub-folders. Differential Revision: [D5689636](https://our.internmc.facebook.com/intern/diff/D5689636/) [ghstack-poisoned]

xush6528 · 2019-11-15T19:31:59Z

Replaced TestConfig with RpcAgentTestFixture. Because, TestConfig assumes test config passed from Environment Vars (strings). String type is not sufficient if we want to configure with typed objects like RpcAgentOptions.

ezyang · 2019-11-15T21:52:52Z

New structure looks fine. Did you want me to review the mixin code too, or are others getting it?

xush6528 · 2019-11-15T22:02:14Z

Thanks @ezyang. No other actions needed at the moment.

xush6528 · 2019-11-15T22:03:33Z

I have a conclusion with @pritamdamania87 , we don't want to have differnet Test classes for dirrent RPC backends.

We will use trap code to configure 1) which backend the test should us and 2) the RpcAgentOptions to use, so after settingup as a middle step, the test binary still falls to the single test entry class, RpcTest class, DistAutograd class and DistOptimizer class.

I will revert most of the refacting in this PR and only leave the TestFixture class as it's the duplicate code extracted from RpcTest class, DistAutograd class and DistOptimizer class.

… Fixtures" We plan to introduce different `RpcAgentOption`s, this requires us to allow different tests for differnet `RpcAgent`s to correctly configure the test by bringing in different test mixins. This refactoring PR is empowering this. Also, with more fixtures and mixins, moved tests into sub-folders. Differential Revision: [D5689636](https://our.internmc.facebook.com/intern/diff/D5689636/) [ghstack-poisoned]

Pull Request resolved: #29747 We plan to introduce different `RpcAgentOption`s, this requires us to allow different tests for differnet `RpcAgent`s to correctly configure the test by bringing in different test mixins. This refactoring PR is empowering this. Also, with more fixtures and mixins, moved tests into sub-folders. ProcessGroupRpcAgentTestMixin/ThriftTestRpcAgentTestMixin replace class TestConfig. Differential Revision: [D5689636](https://our.internmc.facebook.com/intern/diff/D5689636/) ghstack-source-id: 94014185

mrshenli

RpcAgentTestFixture and code refactoring look good to me!

mrshenli · 2019-11-18T17:49:53Z

test/dist_optimizer_test.py

 import torch.distributed.autograd as dist_autograd
 import torch.distributed.rpc as rpc
 import threading
+from rpc_agent_test_fixture import RpcAgentTestFixture


move this line up to put it together with dist_utils?

mrshenli · 2019-11-18T17:51:31Z

test/rpc_test.py

+import dist_utils
+from dist_utils import dist_init
 from torch.distributed.rpc.internal import PythonUDF, _internal_rpc_pickler
+from rpc_agent_test_fixture import RpcAgentTestFixture


We plan to introduce different `RpcAgentOption`s, this requires us to allow different tests for differnet `RpcAgent`s to correctly configure the test by bringing in different test mixins. This refactoring PR is empowering this. Also, with more fixtures and mixins, moved tests into sub-folders. Differential Revision: [D5689636](https://our.internmc.facebook.com/intern/diff/D5689636/) [ghstack-poisoned]

Pull Request resolved: #29747 We plan to introduce different `RpcAgentOption`s, this requires us to allow different tests for differnet `RpcAgent`s to correctly configure the test by bringing in different test mixins. This refactoring PR is empowering this. Also, with more fixtures and mixins, moved tests into sub-folders. ProcessGroupRpcAgentTestMixin/ThriftTestRpcAgentTestMixin replace class TestConfig. Differential Revision: [D5689636](https://our.internmc.facebook.com/intern/diff/D5689636/) ghstack-source-id: 94122697

mrshenli · 2019-11-18T18:31:05Z

Shall we remove this line in the PR description as it is no longer relevant?

Also, with more fixtures and mixins, moved tests into sub-folders.

xush6528 · 2019-11-18T19:51:52Z

@mrshenli Removed.

facebook-github-bot · 2019-11-18T22:34:13Z

This pull request has been merged in 8dd6705.

rohan-varma · 2019-11-19T13:51:14Z

it looks like this PR broke the windows build? https://ci.pytorch.org/jenkins/job/pytorch-builds/job/pytorch-win-ws2016-cuda9-cudnn7-py3-test2/56195/console - (also looks like the culprit from the CI page - https://ezyang.github.io/pytorch-ci-hud/build/pytorch-master)

rohan-varma · 2019-11-19T13:59:27Z

test/run_test.py


 WINDOWS_BLACKLIST = [
    'distributed',
-    'rpc_fork',


any reason these are removed from the windows blacklist?

@rohan-varma
Wanted to include changes in #29827 to avoid that PR rebasing again.
I should have removed line 71, 73 too.

ezyang · 2019-11-19T16:02:39Z

For future reference, if you identify that the PR broke a build, you should revert it.

xush6528 mentioned this pull request Nov 13, 2019

Add RpcAgentOptions struct type, which bundles different required arguments for different RpcAgents #29443

Closed

xush6528 requested review from mrshenli, pietern, pritamdamania87, rohan-varma and zhaojuanmao and removed request for pietern November 13, 2019 20:55

mrshenli reviewed Nov 14, 2019

View reviewed changes

mrshenli requested a review from ezyang November 14, 2019 00:23

mrshenli reviewed Nov 14, 2019

View reviewed changes

ezyang reviewed Nov 14, 2019

View reviewed changes

ezyang requested changes Nov 14, 2019

View reviewed changes

ezyang self-requested a review November 15, 2019 21:52

xush6528 changed the title ~~Refactoring torch.distributed.rpc tests to have Mixins and Fixtures~~ Add RpcAgentTestFixture to extract duplicate code Nov 16, 2019

xush6528 requested a review from mrshenli November 16, 2019 06:55

mrshenli approved these changes Nov 18, 2019

View reviewed changes

zhaojuanmao mentioned this pull request Nov 18, 2019

Remove rpc fork and dist autograd fork tests from PyTorch repo #29827

Closed

facebook-github-bot closed this in 8dd6705 Nov 18, 2019

facebook-github-bot added the merged label Nov 18, 2019

rohan-varma reviewed Nov 19, 2019

View reviewed changes

rohan-varma mentioned this pull request Nov 19, 2019

[rpc] poll for timed out futures in process group agent #29601

Closed

pietern mentioned this pull request Nov 19, 2019

[rpc][build] fix windows build #30078

Closed

This was referenced Nov 19, 2019

Resubmit #29747. "Add RpcAgentTestFixture to extract duplicate code" #30092

Closed

Resubmit #29747. "Add RpcAgentOptions struct type, which bundles different required arguments for different RpcAgents" #30093

Closed

mrshenli mentioned this pull request Nov 19, 2019

Polish rpc docstring. #30069

Closed

facebook-github-bot deleted the gh/xush6528/34/head branch November 22, 2019 15:17

mrshenli mentioned this pull request Nov 26, 2019

Move pytorch distributed tests to separate folder for contbuild. #30445

Closed

mruberry added the Merged label Oct 28, 2020

Add RpcAgentTestFixture to extract duplicate code #29747

Add RpcAgentTestFixture to extract duplicate code #29747

Uh oh!

Conversation

xush6528 commented Nov 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xush6528 Nov 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrshenli commented Nov 14, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ezyang left a comment

Choose a reason for hiding this comment

Uh oh!

xush6528 commented Nov 15, 2019

Uh oh!

ezyang commented Nov 15, 2019

Uh oh!

xush6528 commented Nov 15, 2019

Uh oh!

xush6528 commented Nov 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrshenli left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrshenli commented Nov 18, 2019

Uh oh!

xush6528 commented Nov 18, 2019

Uh oh!

facebook-github-bot commented Nov 18, 2019

Uh oh!

rohan-varma commented Nov 19, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ezyang commented Nov 19, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

xush6528 commented Nov 13, 2019 •

edited

Loading

xush6528 Nov 14, 2019 •

edited

Loading

xush6528 commented Nov 15, 2019 •

edited

Loading