Add an option to run RPC tests with TCP init

[rohan-varma]

Stack from ghstack:

• Add an option to run RPC tests with TCP init #48248 Add an option to run RPC tests with TCP init

We have found a few bugs where initializing/de-initializing/re-initializing RPC, and using RPC along with process groups does not work as expected, usually under TCP/env initialization (which is used over file which is the init that we use in our test in multi-machine scenarios).

Due to this motivation, this PR adds an environment variable RPC_INIT_WITH_TCP that allows us to run any RPC test with TCP initialization.

To avoid port collisions, we use common.find_free_port().
Differential Revision: D25085458

NOTE FOR REVIEWERS: This PR has internal Facebook specific changes or comments, please review them on Phabricator!

[rohan-varma]

As I understand it, this actually may not be enough to prevent port collisions if there are multiple tests running in parallel? Test 1 could get port X, but before it creates the store and binds the port, Test 2 could also get port X and we would still have the race. If my understanding is correct, we probably need to write currently in use ports to a shared/locked data structure to completely avoid races?

[rohan-varma]

(This probably won't be an issue anymore now that we are not enabling it for all tests)

[dr-ci]

💊 CI failures summary and remediations

As of commit a8f9d8b (more details on the Dr. CI page):

---

• 1/3 failures possibly* introduced in this PR
  • 1/1 non-CircleCI failure(s)
• 2/3 broken upstream at merge base ccd20e9 since Nov 30

---

🚧 1 ongoing upstream failure:

These were probably caused by upstream breakages that are not fixed yet:

• pytorch_linux_xenial_py3_clang7_onnx_ort_test2 since Nov 30
  • 🔁 rerun

---

🚧 1 fixed upstream failure:

These were probably caused by upstream breakages that were already fixed.

Please rebase on the viable/strict branch (expand for instructions)

If your commit is newer than viable/strict, you can try basing on an older, stable commit:

git fetch https://github.com/pytorch/pytorch viable/strict
git rebase --onto FETCH_HEAD $(git merge-base origin/master HEAD)

If your commit is older than viable/strict:

git fetch https://github.com/pytorch/pytorch viable/strict
git rebase FETCH_HEAD

Check out the recency history of this "viable master" tracking branch.

• pytorch_linux_xenial_cuda10_2_cudnn7_py3_gcc7_test2 from Nov 30 until Dec 01 (8 commits; d2e4298 - c81f2d9)
  • 🔁 rerun

---

ci.pytorch.org: 1 failed

• Failed: pr/pytorch-linux-bionic-rocm3.9-py3.6

---

This comment was automatically generated by Dr. CI (expand for details).

Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions on the GitHub issue tracker or post in the (internal) Dr. CI Users group.

See how this bot performed.

This comment has been revised 30 times.

[lw]

My main comment on this is whether we need to re-run the entire RPC suite to check for differences between file-based and TCP-based init. The suite already takes quite a while to run, and once we'll add GPU tests and tests for other backends (like shared memory) it might lead to a "combinatorial explosion" of the time it takes to run it.

Would there be a way to replicate what RPC does during its init, and add that as a test case for the Stores? I'll also take a look at the issue you referenced in the comments to get more context.

[rohan-varma]

@lw Thanks for bringing that up, my line of thinking was that it could be worth it to run with TCP init as this change already caught ~4 tests that are broken with TCP init but fine with file init.

We could always leave it as a flag and developers would have the option to set the flag to run with TCP init, but this probably doesn't eliminate the chance of checking in changes that may not work with TCP init. By running it in CI, there's probably a higher chance of preventing these bugs. If we're not sure about this, maybe we can just keep it as an env variable for now and not land the code that adds it as part of CI (at least in OSS, I think it's fine to run these internally)?

Also for clarification, for the increased time to run RPC test suite, is the concern mainly from developer time/productivity perspective or CI env time/cost perspective (or both)?

For context around shared memory, that will be a transport built into TensorPipe right? Would we need to run all RPC tests under that transport or would it be sufficient to test that at the TensorPipe level?

[lw]

is the concern mainly from developer time/productivity perspective or CI env time/cost perspective (or both)?

I was mainly thinking in terms of time-to-signal for developers.

To ground this discussion in data, I looked at the CI logs of a recent job from master and from this PR:

• master
  https://app.circleci.com/pipelines/github/pytorch/pytorch/241692/workflows/63bedff3-95bb-46e3-9635-2382b5c4f3d5/jobs/9084940

  • ProcessGroup: from 10:29:10 to 10:40:51, hence ~11.5 minutes
  • TensorPipe: from 11:16:14 to 11:29:42, hence ~13.5 minutes

  Total: 25 minutes

• this PR
  https://app.circleci.com/pipelines/github/pytorch/pytorch/241655/workflows/c796c3dc-f85e-4308-882a-fde048b36122/jobs/9081850

  • ProcessGroup: from 06:02:32 to 06:26:48, hence ~24 minutes
  • TensorPipe: from 06:41:22 to 07:08:39, hence ~27 minutes

  Total: 51 minutes

It's just a single data point but it suggests that this PR would cause every PR to take 25 minutes longer to receive the green light from the CI. This may slow down developers (or encourage to land without waiting for CI).

Let me just clarify that I think we should absolutely run some tests with TCP. As you said, even just using it in our current tests revealed a bunch of errors that we should have caught earlier. However, those tests that fail do so because they're doing some "unusual" operations around initing and shutting down. My point is that tests of that type are the only ones that should be enrolled in TCP init, as they're the ones most likely to reveal something interesting. The vast majority of RPC tests do a "standard" init and then just test a wide variety of RPC-only features. It should be enough to run only one test of that type, as they all have effectively the same behavior when it comes to file-vs-TCP init.

Do you think we could take a pass over the RPC tests and try to select the ones we think would stress "unusual" code paths in init and tag them with a decorator, and then run only those with the TCP store?

For context around shared memory, that will be a transport built into TensorPipe right? Would we need to run all RPC tests under that transport or would it be sufficient to test that at the TensorPipe level?

We didn't figure it out yet, but indeed if we were to run the entire RPC suite with the shared memory transports we'd also greatly increase the total PyTorch CI test time (by ~13.5 minutes, as only the TensorPipe suite needs to be repeated). This is certainly not great and ideally we should do as you say and test this transport in isolation within TensorPipe and then just "trust" that it works in PyTorch, without re-testing it. Unfortunately for now the PyTorch test suite is much more comprehensive than TensorPipe's own one... :/

[rohan-varma]

Do you think we could take a pass over the RPC tests and try to select the ones we think would stress "unusual" code paths in init and tag them with a decorator, and then run only those with the TCP store?

Sounds good, so I guess the proposal is the following:

• Don't run all tests with TCP init in addition to file in CI
• Add a decorator to run certain tests with the TCP init as well

[codecov]

Codecov Report

Merging #48248 (21e9af6) into gh/rohan-varma/201/base (ccd20e9) will increase coverage by 0.31%.
The diff coverage is 31.74%.

@@                     Coverage Diff                     @@
##           gh/rohan-varma/201/base   #48248      +/-   ##
===========================================================
+ Coverage                    80.46%   80.77%   +0.31%     
===========================================================
  Files                         1855     1855              
  Lines                       200297   200229      -68     
===========================================================
+ Hits                        161160   161741     +581     
+ Misses                       39137    38488     -649     

[rohan-varma]

@lw Updated the PR to just add the flag as an environment variable which will control whether the test runs with TCP or not, so that we can just run then under tcp init on an ad-hoc basis. I disabled a couple of the tests that don't work under it which we should fix.

[lw]

Ok, I'm fine with landing it as it is. It would be nice if we could enable some of these tests to run with TCP init in the CI at a later point.

[rohan-varma]

@lw Sounds good, there are 2 tests right now that probably should run with TCP init but don't work, I'll check and see if there are any additional.

[facebook-github-bot]

This pull request has been merged in 8b2ca28.