make rpc and dist-autograd multiprocess test to use both fork and spawn by zhaojuanmao · Pull Request #25656 · pytorch/pytorch

zhaojuanmao · 2019-09-04T18:12:16Z

Stack from ghstack:

make python udf serialization format to be binary plus tensor tables #27136 make python udf serialization format to be binary plus tensor tables
make rpc and dist-autograd multiprocess test to use both fork and spawn #25656 make rpc and dist-autograd multiprocess test to use both fork and spawn

spawn multiprocessing can catch some issues that fork multiprocessing can not catch, meanwhile fork can work properly with asan tests, but spawn multiprocessing can not work with asan tests for some use cases right now.

so this diff adding support to launch both spawn and fork tests in multiProcessingTestCase class, also let test_rpc and test_dist_autograd to run both spawn and fork tests

Differential Revision: D17086007

mrshenli · 2019-09-04T18:15:30Z

 #!/usr/bin/env python3
 from __future__ import absolute_import, division, print_function, unicode_literals

+import numpy


why do we need this import?

it tried to see whether it can fix test error about "numpy + intel MKL", but I removed in new diff, will see whether tests passed

mrshenli · 2019-09-04T18:17:07Z

  }

  std::vector<char> generatePythonUDFResult(
    const Message& request) {


indent, does this fit into the line above?

ran clang-format in new diff

Pull Request resolved: #25656 1. current fork unit tests did not catch rpc exit crash issue, but spawn caught it. most of use cases also uses spawn, so changing multiprocessing start method from fork to spawn 2. it did not change fork to spawn for c10d tests for now -- if we need to make changes for c10d, it may need another PR as c10d has a lot of unit tests, may need some more changes to make spawn work for test_c10d 3. fix the real segment exit issue caused by python rpc handler temporarily by removing module_ global variable, separate diff D17097999 is to change python rpc handler to be singletion class for deterministic destruction order of variables ghstack-source-id: 89482622 Differential Revision: [D17086007](https://our.internmc.facebook.com/intern/diff/D17086007/)

Pull Request resolved: #25656 1. current fork unit tests did not catch rpc exit crash issue, but spawn caught it. most of use cases also uses spawn, so changing multiprocessing start method from fork to spawn 2. it did not change fork to spawn for c10d tests for now -- if we need to make changes for c10d, it may need another PR as c10d has a lot of unit tests, may need some more changes to make spawn work for test_c10d 3. fix the real segment exit issue caused by python rpc handler temporarily by removing module_ global variable, separate diff D17097999 is to change python rpc handler to be singletion class for deterministic destruction order of variables ghstack-source-id: 89487514 Differential Revision: [D17086007](https://our.internmc.facebook.com/intern/diff/D17086007/)

Pull Request resolved: #25656 1. current fork unit tests did not catch rpc exit crash issue, but spawn caught it. most of use cases also uses spawn, so changing multiprocessing start method from fork to spawn 2. it did not change fork to spawn for c10d tests for now -- if we need to make changes for c10d, it may need another PR as c10d has a lot of unit tests, may need some more changes to make spawn work for test_c10d 3. fix the real segment exit issue caused by python rpc handler temporarily by removing module_ global variable, separate diff D17097999 is to change python rpc handler to be singletion class for deterministic destruction order of variables ghstack-source-id: 89508433 Differential Revision: [D17086007](https://our.internmc.facebook.com/intern/diff/D17086007/)

pritamdamania87 · 2019-09-04T23:44:13Z

-        self.rank = self.MAIN_PROCESS_RANK
-        self.file = tempfile.NamedTemporaryFile(delete=False)
-        self.processes = [self._spawn_process(rank) for rank in range(int(self.world_size))]
+        if self.rank == self.MAIN_PROCESS_RANK:


Why do we need this change? Isn't the setUp method run only on the parent process?

this was added to check whether setup() is related after tests failures, it did not help, but I think it is no harm to keep there, if child process want to set up same variables as parent in the future, they can call setup() function

pritamdamania87 · 2019-09-04T23:46:20Z

+        # We're retrieving a corresponding test and executing it.
+        getattr(self, test_name)()
+        if multiprocessing.get_start_method() != "spawn":
+                sys.exit(0)


Doesn't this affect all c10d tests? Also, why are we checking for this after the test is done instead of before it?

c10d is not spawn, so it is not affected. we just change behavior for rpc, when it is spawn, we do not need to exit() here explicitly, as @xush6528 mentioned sys.exit(0) here is mainly for fork processes to avoid run teardown().

pritamdamania87 · 2019-09-04T23:47:43Z

    sys.version_info < (3, 0),
    "Pytorch distributed rpc package " "does not support python2",
 )
+@unittest.skipIf(C.is_asan, "Skip ASAN since torch + multiprocessing + fbcode doesn't work with asan")


don't mention fbcode in OSS code.

pritamdamania87 · 2019-09-04T23:48:37Z

+        super(RpcTest, cls). setUpClass()
+        try:
+            multiprocessing.set_start_method("spawn")
+        except Exception:


Why do we ignore this exception and continue? We should throw this exception and fail the test.

for some env, if there is no spawn, at least we should run fork?

xush6528 · 2019-09-05T00:25:27Z

        self.rank = rank
+        self.file_name = file_name
+
+        self.setUp()


Why do we need children to call self.setUp()? What was missing?

zhaojuanmao · 2019-09-05T05:59:07Z

although tests passed locally, CI still failed with two failures: 1) asan test, although I skipped asan 2) Numpy + Intel MKL incompatibility error

Tried to add some fast guessing changes (e.g., add setup() for child process as suspecting dependencies are not loaded properly; play with sys.exit(0) as suspecting process clean up has something wrong), but both of them did not work.

so will try following things:

download docker file to debug and try to fix for existing implementation, as it looks like it is close to finish
if it is found that it is hard to fix, will try to use torch.multiprocessing.spawn() - maybe it is better?

pietern · 2019-09-05T07:37:04Z

@zhaojuanmao torch.multiprocessing.spawn is just a wrapper for doing multiprocessing.Process and joining them once done. It won't address more fundamental problems.

Btw -- on the previous version of this PR I left a comment regarding the runtime. Did you happen to check how much impact the spawn start method has on the total runtime of the distributed tests?

zhaojuanmao · 2019-09-05T07:55:22Z

@pietern spawn is much slower, 28.67s vs 8.73s for test_rpc.

pietern · 2019-09-05T08:06:22Z

@zhaojuanmao Thanks for checking. We should investigate if we can't switch to a mode where we launch processes once and reuse them across tests. There are downsides to doing that as well, but with such a runtime it's worth exploring in my opinion.

Pull Request resolved: #25656 1. current fork unit tests did not catch rpc exit crash issue, but spawn caught it. most of use cases also uses spawn, so changing multiprocessing start method from fork to spawn 2. it did not change fork to spawn for c10d tests for now -- if we need to make changes for c10d, it may need another PR as c10d has a lot of unit tests, may need some more changes to make spawn work for test_c10d 3. fix the real segment exit issue caused by python rpc handler temporarily by removing module_ global variable, separate diff D17097999 is to change python rpc handler to be singletion class for deterministic destruction order of variables ghstack-source-id: 89587243 Differential Revision: [D17086007](https://our.internmc.facebook.com/intern/diff/D17086007/)

zhaojuanmao · 2019-09-05T23:08:11Z

I tried to use torch.multiprocessing.spawn() in another PR for my investigation purpose, and all tests passed!

indeed, for torch.multiprocessing, both fork and spawn methods caught crash issue and run almost the same amount of time for test_rpc (28s). native multiprocessing fork run only 8s, but it did not catch the crash issue -- which means something is wrong.

So I'm preferring to go ahead with torch.multiprocessing implementation in this diff. Is 28s acceptable? I guess it should be fine now, spawn multi processes for each test case feel like it is more clean set up. The purpose of unit tests is to test correctness, so it should be fine to sacrifice some performance?

in this diff, make dist_autograd to use spawn as well.

tried c10d tests, most of them works fine, a few ones are a little flaky some how, will address it in separate diff

mrshenli · 2019-09-06T14:14:18Z

Is 28s acceptable?

It is OK to me for now, as test_rpc.py does not have too many tests and is unlikely to grow super large very soon, but let's create an issue to track this. Let's get @pietern's input on this as well.

mrshenli

Do we have any new tests to make sure that spawn indeed catch crashes?

mrshenli · 2019-09-06T14:20:47Z

+    return TIMEOUT_OVERRIDE.get(test_id.split(".")[-1], TIMEOUT_DEFAULT)
+
+
+class MultiProcessTestCase(TestCase):


Let's add some comments here to explain the difference between this MultiProcessTestCase and the one in common_distributed.py. It might be helpful to give them different names.

mrshenli · 2019-09-06T14:21:46Z

+}
+
+
+def skip_if_not_multigpu(func):


Can we import the one from common_distributed, or is this one different than that?

mrshenli · 2019-09-06T14:22:25Z

+    return wrapper
+
+
+def skip_if_lt_x_gpu(x):


ditto, and for all the functions below.

zhaojuanmao · 2019-09-06T16:16:05Z

@mrshenli it is very hard to find another example to show this diff catch an issue that has not been caught previously right now, but we've already have one example, is not it?

eventually common_distributed.py will be killed after test_c10d.py starts to use the new multiprocessing_test_case.py, so planning not to let them share same codes, will add another PR for test_c10d.py to use multiprocessing_test_case.py

zhaojuanmao · 2019-09-06T16:32:06Z

@pietern regarding runtime concerns, you mentioned "We should investigate if we can't switch to a mode where we launch processes once and reuse them across tests. There are downsides to doing that as well"

would you please elaborate more about your idea? how could it be possible to insert each test into launched processes? you mentioned there are downsides, what kind of downsides in your mind?

for unit tests, I guess runtime is not most concerned, correctness is more important?

mrshenli · 2019-09-06T17:23:22Z

eventually common_distributed.py will be killed after test_c10d.py starts to use the new multiprocessing_test_case.py

Just want to make sure I understand the plan here. By "will be killed" do you mean eventually, we will still have a file named common_distributed.py, but the MultiProcessTestCase will be replaced by the one implemented here? Otherwise, if the plan is to keep MultiProcessTestCase in a separate file named multiprocessing_test_case.py, we might still need a common_distributed.py file to keep utility functions and decorators.

zhaojuanmao · 2019-09-06T17:52:22Z

@mrshenli multiprocessing_test_case.py has all the utilities and decorators that are same as common_distributed.py, it copies from common_distributed.py with different multiProcessTestCase class implementation.

Do you prefer to still name "multiprocessing_test_case.py " as "common_distributed.py"?

mrshenli · 2019-09-06T17:56:34Z

Do you prefer to still name "multiprocessing_test_case.py " as "common_distributed.py"?

I feel things like skip_if_lt_x_gpu and requires_gloo should be independent concepts of multiprocessing_test_case. "common_distributed.py" sounds like a more proper place to hold those utility functions.

Pull Request resolved: #25656 spawn multiprocessing can catch some issues that fork multiprocessing can not catch, meanwhile fork can work properly with asan tests, but spawn multiprocessing can not work with asan tests for some use cases right now. so this diff adding support to launch both spawn and fork tests in multiProcessingTestCase class, also let test_rpc and test_dist_autograd to run both spawn and fork tests ghstack-source-id: 91037514 Differential Revision: [D17086007](https://our.internmc.facebook.com/intern/diff/D17086007/)

…ork and spawn" spawn multiprocessing can catch some issues that fork multiprocessing can not catch, meanwhile fork can work properly with asan tests, but spawn multiprocessing can not work with asan tests for some use cases right now. so this diff adding support to launch both spawn and fork tests in multiProcessingTestCase class, also let test_rpc and test_dist_autograd to run both spawn and fork tests Differential Revision: [D17086007](https://our.internmc.facebook.com/intern/diff/D17086007/) [ghstack-poisoned]

Pull Request resolved: #25656 spawn multiprocessing can catch some issues that fork multiprocessing can not catch, meanwhile fork can work properly with asan tests, but spawn multiprocessing can not work with asan tests for some use cases right now. so this diff adding support to launch both spawn and fork tests in multiProcessingTestCase class, also let test_rpc and test_dist_autograd to run both spawn and fork tests ghstack-source-id: 91076542 Differential Revision: [D17086007](https://our.internmc.facebook.com/intern/diff/D17086007/)

mrshenli · 2019-10-01T02:59:53Z

02:34:22 Running test_rpc_fork ... [2019-10-01 02:34:23.013131]
02:34:23 Traceback (most recent call last):
02:34:23   File "test_rpc_fork.py", line 4, in <module>
02:34:23     from rpc_test import RpcTest
02:34:23   File "C:\Jenkins\workspace\pytorch-builds\pytorch-win-ws2016-cuda9-cudnn7-py3-test2\test\rpc_test.py", line 11, in <module>
02:34:23     from torch.distributed.rpc import RpcBackend
02:34:23   File "C:\Jenkins\workspace\pytorch-builds\pytorch-win-ws2016-cuda9-cudnn7-py3-test2\build\win_tmp\build\torch\distributed\rpc.py", line 3, in <module>
02:34:23     from . import invoke_rpc_builtin, invoke_rpc_python_udf, invoke_remote_builtin
02:34:23 ImportError: cannot import name 'invoke_rpc_builtin'
02:34:23 Traceback (most recent call last):
02:34:23   File "run_test.py", line 442, in <module>
02:34:23     main()
02:34:23   File "run_test.py", line 434, in main
02:34:23     raise RuntimeError(message)
02:34:23 RuntimeError: test_rpc_fork failed!

I think the above test failure is because we don't build rpc code for windows (see this line). Let's skip RPC tests for windows too (by adding it to the windows black list).

…ork and spawn" spawn multiprocessing can catch some issues that fork multiprocessing can not catch, meanwhile fork can work properly with asan tests, but spawn multiprocessing can not work with asan tests for some use cases right now. so this diff adding support to launch both spawn and fork tests in multiProcessingTestCase class, also let test_rpc and test_dist_autograd to run both spawn and fork tests Differential Revision: [D17086007](https://our.internmc.facebook.com/intern/diff/D17086007/) [ghstack-poisoned]

Pull Request resolved: #25656 spawn multiprocessing can catch some issues that fork multiprocessing can not catch, meanwhile fork can work properly with asan tests, but spawn multiprocessing can not work with asan tests for some use cases right now. so this diff adding support to launch both spawn and fork tests in multiProcessingTestCase class, also let test_rpc and test_dist_autograd to run both spawn and fork tests ghstack-source-id: 91083537 Differential Revision: [D17086007](https://our.internmc.facebook.com/intern/diff/D17086007/)

zhaojuanmao · 2019-10-01T07:15:14Z

@pytorchbot retest this please

zhaojuanmao · 2019-10-01T14:04:02Z

landing the diff, the failure is not related

…ork and spawn" spawn multiprocessing can catch some issues that fork multiprocessing can not catch, meanwhile fork can work properly with asan tests, but spawn multiprocessing can not work with asan tests for some use cases right now. so this diff adding support to launch both spawn and fork tests in multiProcessingTestCase class, also let test_rpc and test_dist_autograd to run both spawn and fork tests Differential Revision: [D17086007](https://our.internmc.facebook.com/intern/diff/D17086007/) [ghstack-poisoned]

Pull Request resolved: #25656 spawn multiprocessing can catch some issues that fork multiprocessing can not catch, meanwhile fork can work properly with asan tests, but spawn multiprocessing can not work with asan tests for some use cases right now. so this diff adding support to launch both spawn and fork tests in multiProcessingTestCase class, also let test_rpc and test_dist_autograd to run both spawn and fork tests ghstack-source-id: 91096705 Differential Revision: [D17086007](https://our.internmc.facebook.com/intern/diff/D17086007/)

zhaojuanmao · 2019-10-01T15:12:57Z

@pytorchbot retest this please

zrphercule · 2019-10-01T19:53:09Z

Hi @zhaojuanmao it seems this pr breaks pytorch master ci test, wonder if you are fixing it, or should I revert it?

mrshenli · 2019-10-01T20:06:50Z

@zrphercule we are landing #27158 to disable the flaky test now. Please don't revert.

mrshenli · 2019-10-01T20:23:15Z

#27158 is landed

facebook-github-bot · 2019-10-01T20:39:37Z

This pull request has been merged in 1d2d59d.

…wn (pytorch#25656) Summary: Pull Request resolved: pytorch#25656 spawn multiprocessing can catch some issues that fork multiprocessing can not catch, meanwhile fork can work properly with asan tests, but spawn multiprocessing can not work with asan tests for some use cases right now. so this diff adding support to launch both spawn and fork tests in multiProcessingTestCase class, also let test_rpc and test_dist_autograd to run both spawn and fork tests ghstack-source-id: 91096705 Test Plan: unit tests Reviewed By: xush6528 Differential Revision: D17086007 fbshipit-source-id: af2446e7abe948c37081cff24ed060fd87f84922

Summary: pietern discovered that `test_invalid_names` is flaky on master. pytorch#25656 is potentially the fix. Disable this test for now and will try to add it again when pytorch#25656 is in. Pull Request resolved: pytorch#25916 Differential Revision: D17287496 Pulled By: mrshenli fbshipit-source-id: 9313958d3480c2bab20cd2341837c7821e3bb1b5

…wn (pytorch#25656) Summary: Pull Request resolved: pytorch#25656 spawn multiprocessing can catch some issues that fork multiprocessing can not catch, meanwhile fork can work properly with asan tests, but spawn multiprocessing can not work with asan tests for some use cases right now. so this diff adding support to launch both spawn and fork tests in multiProcessingTestCase class, also let test_rpc and test_dist_autograd to run both spawn and fork tests ghstack-source-id: 91096705 Test Plan: unit tests Reviewed By: xush6528 Differential Revision: D17086007 fbshipit-source-id: af2446e7abe948c37081cff24ed060fd87f84922

zhaojuanmao requested review from mrshenli and pietern as code owners September 4, 2019 18:12

pytorchbot added oncall: distributed Add this issue/PR to distributed oncall triage queue module: pybind Related to our Python bindings / interactions with other Python libraries labels Sep 4, 2019

zhaojuanmao mentioned this pull request Sep 4, 2019

make rpc multiprocess test to use spawn instead of fork #25558

Closed

zhaojuanmao requested a review from xush6528 September 4, 2019 18:14

mrshenli reviewed Sep 4, 2019

View reviewed changes

pritamdamania87 reviewed Sep 4, 2019

View reviewed changes

pritamdamania87 suggested changes Sep 4, 2019

View reviewed changes

xush6528 reviewed Sep 5, 2019

View reviewed changes

zhaojuanmao mentioned this pull request Sep 6, 2019

make python rpc handler to be singleton class #25742

Closed

mrshenli reviewed Sep 6, 2019

View reviewed changes

zhaojuanmao mentioned this pull request Oct 1, 2019

Make RpcTest class re-usable by other RPC backends #27114

Closed

zhaojuanmao mentioned this pull request Oct 1, 2019

make python udf serialization format to be binary plus tensor tables #27136

Closed

facebook-github-bot closed this in 1d2d59d Oct 1, 2019

xush6528 mentioned this pull request Oct 1, 2019

Move imports to after is_available in test_rpc to fix windows builds #27126

Closed

facebook-github-bot added the merged label Oct 1, 2019

zhaojuanmao mentioned this pull request Oct 1, 2019

add "pytorch_linux_xenial_py3_5_test" into CI tests #27168

Closed

facebook-github-bot deleted the gh/zhaojuanmao/1/head branch October 28, 2019 22:23

mruberry added the Merged label Oct 28, 2020

		return TIMEOUT_OVERRIDE.get(test_id.split(".")[-1], TIMEOUT_DEFAULT)


		class MultiProcessTestCase(TestCase):

		}


		def skip_if_not_multigpu(func):

Conversation

zhaojuanmao commented Sep 4, 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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xush6528 Sep 5, 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!

zhaojuanmao commented Sep 5, 2019

Uh oh!

pietern commented Sep 5, 2019

Uh oh!

zhaojuanmao commented Sep 5, 2019

Uh oh!

pietern commented Sep 5, 2019

Uh oh!

zhaojuanmao commented Sep 5, 2019

Uh oh!

mrshenli commented Sep 6, 2019

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zhaojuanmao commented Sep 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zhaojuanmao commented Sep 6, 2019

Uh oh!

mrshenli commented Sep 6, 2019

Uh oh!

zhaojuanmao commented Sep 6, 2019

Uh oh!

mrshenli commented Sep 6, 2019

Uh oh!

mrshenli commented Oct 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zhaojuanmao commented Oct 1, 2019

Uh oh!

zhaojuanmao commented Oct 1, 2019

Uh oh!

zhaojuanmao commented Oct 1, 2019

Uh oh!

zrphercule commented Oct 1, 2019

Uh oh!

mrshenli commented Oct 1, 2019

Uh oh!

mrshenli commented Oct 1, 2019

Uh oh!

facebook-github-bot commented Oct 1, 2019

Uh oh!

zhaojuanmao commented Sep 4, 2019 •

edited

Loading

xush6528 Sep 5, 2019 •

edited

Loading

zhaojuanmao commented Sep 6, 2019 •

edited

Loading

mrshenli commented Oct 1, 2019 •

edited

Loading