Delete all user forks tracked in RRefContext before shutting down by xush6528 · Pull Request #31893 · pytorch/pytorch

xush6528 · 2020-01-06T20:14:05Z

Stack from ghstack:

Delete all user forks tracked in RRefContext before shutting down #31893 Delete all user forks tracked in RRefContext before shutting down

In order to resolve the issue summarized in #31325.

The overal solution is to proactively send out delete fork messages from user nodes, before user nodes detecting rref leaks.

As the first step, we want to have a weak ref tracker to track all user rrefs.

Differential Revision: D19292254

In order to resolve the issue summarized in #31325. The overal solution is to proactively send out delete fork messages from user nodes, before user nodes detecting rref leaks. As the first step, we want to have a weak ref tracker to track all user rrefs. Differential Revision: [D19292254](https://our.internmc.facebook.com/intern/diff/D19292254/) [ghstack-poisoned]

In order to resolve the issue summarized in #31325. The overal solution is to proactively send out delete fork messages from user nodes, before user nodes detecting rref leaks. As the first step, we want to have a weak ref tracker to track all user rrefs. Differential Revision: [D19292254](https://our.internmc.facebook.com/intern/diff/D19292254/) ghstack-source-id: 96331929 Pull Request resolved: #31893

In order to resolve the issue summarized in #31325. The overal solution is to proactively send out delete fork messages from user nodes, before user nodes detecting rref leaks. As the first step, we want to have a weak ref tracker to track all user rrefs. Differential Revision: [D19292254](https://our.internmc.facebook.com/intern/diff/D19292254/) [ghstack-poisoned]

Pull Request resolved: #31893 In order to resolve the issue summarized in #31325. The overal solution is to proactively send out delete fork messages from user nodes, before user nodes detecting rref leaks. As the first step, we want to have a weak ref tracker to track all user rrefs. ghstack-source-id: 96366029 Differential Revision: [D19292254](https://our.internmc.facebook.com/intern/diff/D19292254/)

torch/csrc/distributed/rpc/rref_context.h

torch/csrc/distributed/rpc/rref_context.cpp

kostmo · 2020-01-07T19:51:50Z

💊 CircleCI build failures summary and remediations

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

💚 💚 Looks good so far! There are no CircleCI failures yet. 💚 💚

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.

This comment has been revised 144 times.

torch/csrc/distributed/rpc/rref_impl.cpp

…ng down" In order to resolve the issue summarized in #31325. The overal solution is to proactively send out delete fork messages from user nodes, before user nodes detecting rref leaks. As the first step, we want to have a weak ref tracker to track all user rrefs. Differential Revision: [D19292254](https://our.internmc.facebook.com/intern/diff/D19292254/) [ghstack-poisoned]

Pull Request resolved: #31893 In order to resolve the issue summarized in #31325. The overal solution is to proactively send out delete fork messages from user nodes, before user nodes detecting rref leaks. As the first step, we want to have a weak ref tracker to track all user rrefs. ghstack-source-id: 96693731 Differential Revision: [D19292254](https://our.internmc.facebook.com/intern/diff/D19292254/)

…ext before shutting down" In order to resolve the issue summarized in #31325. The overal solution is to proactively send out delete fork messages from user nodes, before user nodes detecting rref leaks. As the first step, we want to have a weak ref tracker to track all user rrefs. Differential Revision: [D19292254](https://our.internmc.facebook.com/intern/diff/D19292254/) [ghstack-poisoned]

Pull Request resolved: #31893 In order to resolve the issue summarized in #31325. The overal solution is to proactively send out delete fork messages from user nodes, before user nodes detecting rref leaks. As the first step, we want to have a weak ref tracker to track all user rrefs. ghstack-source-id: 96753553 Differential Revision: [D19292254](https://our.internmc.facebook.com/intern/diff/D19292254/)

pritamdamania87

Thanks for fixing this! I have a few comments inline.

torch/csrc/distributed/rpc/rref_context.h

test/rpc_test.py

torch/csrc/distributed/rpc/rref_impl.h

torch/csrc/distributed/rpc/rref_impl.cpp

mrshenli

Test failures are real.

Jan 16 01:22:10 ======================================================================
Jan 16 01:22:10 FAIL [1.326s]: test_nested_rref (__main__.RpcTestWithSpawn)
Jan 16 01:22:10 ----------------------------------------------------------------------
Jan 16 01:22:10 Traceback (most recent call last):
Jan 16 01:22:10   File "/var/lib/jenkins/workspace/test/common_distributed.py", line 130, in wrapper
Jan 16 01:22:10     self._join_processes(fn)
Jan 16 01:22:10   File "/var/lib/jenkins/workspace/test/common_distributed.py", line 211, in _join_processes
Jan 16 01:22:10     self._check_return_codes(elapsed_time)
Jan 16 01:22:10   File "/var/lib/jenkins/workspace/test/common_distributed.py", line 231, in _check_return_codes
Jan 16 01:22:10     self.assertEqual(p.exitcode, first_process.exitcode)
Jan 16 01:22:10   File "/var/lib/jenkins/workspace/test/common_utils.py", line 890, in assertEqual
Jan 16 01:22:10     super(TestCase, self).assertLessEqual(abs(x - y), prec, message)
Jan 16 01:22:10 AssertionError: 7 not less than or equal to 1e-05 :

mrshenli · 2020-01-16T02:05:13Z

torch/csrc/distributed/rpc/rref_impl.h

    return rrefId_;
  }

+  virtual void tryDel() {}


ideally, this should only live in UserRRef. This is OK for now, as otherwise we would have to do static_cast. After #30630, we should be able to let confirmedUsers_ store UserRRef, and can then delete this from the base class.

This was my concern too. Thanks for telling me about #30630.

torch/distributed/rpc/api.py

…ng down" In order to resolve the issue summarized in #31325. The overal solution is to proactively send out delete fork messages from user nodes, before user nodes detecting rref leaks. As the first step, we want to have a weak ref tracker to track all user rrefs. Differential Revision: [D19292254](https://our.internmc.facebook.com/intern/diff/D19292254/) [ghstack-poisoned]

Pull Request resolved: #31893 In order to resolve the issue summarized in #31325. The overal solution is to proactively send out delete fork messages from user nodes, before user nodes detecting rref leaks. As the first step, we want to have a weak ref tracker to track all user rrefs. ghstack-source-id: 96765512 Differential Revision: [D19292254](https://our.internmc.facebook.com/intern/diff/D19292254/)

torch/csrc/distributed/rpc/rref_context.h

mrshenli

test_nested_rref_stress has failed. Seems one worker quits too soon.

Jan 16 05:38:51 ......s...................terminate called after throwing an instance of 'gloo::IoException'
Jan 16 05:38:51   what():  [/var/lib/jenkins/workspace/third_party/gloo/gloo/transport/tcp/pair.cc:563] Read error [172.17.0.2]:59041: Connection reset by peer
Jan 16 05:38:51 terminate called after throwing an instance of 'gloo::IoException'
Jan 16 05:38:51   what():  [/var/lib/jenkins/workspace/third_party/gloo/gloo/transport/tcp/pair.cc:563] Read error [172.17.0.2]:32406: Connection reset by peer
Jan 16 05:38:56 ERROR:torch.distributed.rpc.api:worker2 failed to respond to 'Shutdown Proceed.' request in 0:00:05
Jan 16 05:38:56 ERROR:torch.distributed.rpc.api:worker3 failed to respond to 'Shutdown Proceed.' request in 0:00:05

…ext before shutting down" In order to resolve the issue summarized in #31325. The overal solution is to proactively send out delete fork messages from user nodes, before user nodes detecting rref leaks. As the first step, we want to have a weak ref tracker to track all user rrefs. Differential Revision: [D19292254](https://our.internmc.facebook.com/intern/diff/D19292254/) [ghstack-poisoned]

…ng down" Stack from [ghstack](https://github.com/ezyang/ghstack): * **#31893 Delete all user forks tracked in RRefContext before shutting down** In order to resolve the issue summarized in #31325. The overal solution is to proactively send out delete fork messages from user nodes, before user nodes detecting rref leaks. As the first step, we want to have a weak ref tracker to track all user rrefs. Differential Revision: [D19292254](https://our.internmc.facebook.com/intern/diff/D19292254/) [ghstack-poisoned]

…xt before shutting down" Stack from [ghstack](https://github.com/ezyang/ghstack): * **#31893 Delete all user forks tracked in RRefContext before shutting down** In order to resolve the issue summarized in #31325. The overal solution is to proactively send out delete fork messages from user nodes, before user nodes detecting rref leaks. As the first step, we want to have a weak ref tracker to track all user rrefs. Differential Revision: [D19292254](https://our.internmc.facebook.com/intern/diff/D19292254/) [ghstack-poisoned]

… down Pull Request resolved: #31893 In order to resolve the issue summarized in #31325. The overal solution is to proactively send out delete fork messages from user nodes, before user nodes detecting rref leaks. As the first step, we want to have a weak ref tracker to track all user rrefs. ghstack-source-id: 99956149 Differential Revision: [D19292254](https://our.internmc.facebook.com/intern/diff/D19292254/)

xush6528 · 2020-03-11T19:31:39Z

On this version, the error is

[E rref_impl.cpp:91] Error occurred when deleting UserRRef instance, RRefId = GloballyUniqueId(0, 0), ForkId = GloballyUniqueId(0, 1) : responseMessageFuturePtr != nullptr INTERNAL ASSERT FAILED at /var/lib/jenkins/workspace/torch/csrc/distributed/rpc/rref_impl.cpp:87, please report a bug to PyTorch. (tryDel at /var/lib/jenkins/workspace/torch/csrc/distributed/rpc/rref_impl.cpp:87)

The assertion does not hold.

mrshenli · 2020-03-11T20:31:13Z

On this version, the error is

[E rref_impl.cpp:91] Error occurred when deleting UserRRef instance, RRefId = GloballyUniqueId(0, 0), ForkId = GloballyUniqueId(0, 1) : responseMessageFuturePtr != nullptr INTERNAL ASSERT FAILED at /var/lib/jenkins/workspace/torch/csrc/distributed/rpc/rref_impl.cpp:87, please report a bug to PyTorch. (tryDel at /var/lib/jenkins/workspace/torch/csrc/distributed/rpc/rref_impl.cpp:87)

The assertion does not hold.

This is weird. This would only happen when deletedOnOwner_ == false and destroyed_==true? It means the RRefContext has been destroyed but somehow there are still UserRRefs that have never called tryDel?

…ng down" Stack from [ghstack](https://github.com/ezyang/ghstack): * **#31893 Delete all user forks tracked in RRefContext before shutting down** In order to resolve the issue summarized in #31325. The overal solution is to proactively send out delete fork messages from user nodes, before user nodes detecting rref leaks. As the first step, we want to have a weak ref tracker to track all user rrefs. Differential Revision: [D19292254](https://our.internmc.facebook.com/intern/diff/D19292254/) [ghstack-poisoned]

…xt before shutting down" Stack from [ghstack](https://github.com/ezyang/ghstack): * **#31893 Delete all user forks tracked in RRefContext before shutting down** In order to resolve the issue summarized in #31325. The overal solution is to proactively send out delete fork messages from user nodes, before user nodes detecting rref leaks. As the first step, we want to have a weak ref tracker to track all user rrefs. Differential Revision: [D19292254](https://our.internmc.facebook.com/intern/diff/D19292254/) [ghstack-poisoned]

… down Pull Request resolved: #31893 In order to resolve the issue summarized in #31325. The overal solution is to proactively send out delete fork messages from user nodes, before user nodes detecting rref leaks. As the first step, we want to have a weak ref tracker to track all user rrefs. ghstack-source-id: 99982072 Differential Revision: [D19292254](https://our.internmc.facebook.com/intern/diff/D19292254/)

xush6528 · 2020-03-11T22:22:12Z

merged the version in #34610, so the change compared to last night are,

In RRefContext::delAllUserRRef(), copy confirmedUsers to a temp, so that we can release lock, so RRef->tryDel() and Context::delUser(..) can acquire lock, and remove the deleted UserRRef from confirmedUsers_.

A more frequent call path to RRef->tryDel() is Python GC triggered UserRRef destructor, where the wrapping logic does not acquire lock at all.

After sending all delete user RRef messages, should wait for number of OwnerRRefs to reach 0 before moving on.

mrshenli · 2020-03-12T03:34:18Z

torch/csrc/distributed/rpc/rref_context.cpp

+    TORCH_INTERNAL_ASSERT(
+        iter != pendingChildren_.end(),
+        "Inconsistent states: attempt to delete a non-exist child fork.");
+    pendingChildren_.erase(iter);


cannot directly erase the UserRRef here, because that could trigger delUser which would then acquire lock on mutex_ to modify confirmedUsers_. As the lock is already acquired in this block, it would deadlock.

#34610 solved this problem by creating a temporary c10::intrusive_ptr<RRef> deletedUser to keep the deleted UserRRef alive in this block.

Thanks for explaining the subtlety here. Updating with comments.

mrshenli · 2020-03-12T03:34:24Z

torch/csrc/distributed/rpc/rref_context.cpp

+        std::piecewise_construct,
+        std::forward_as_tuple(forkId),
+        std::forward_as_tuple(iter->second));
+    pendingUsers_.erase(iter);


mrshenli

We are getting close. If tests pass after adding c10::intrusive_ptr<RRef> deletedUser;, we can land. :)

mrshenli · 2020-03-12T03:45:37Z

torch/csrc/distributed/rpc/rref_context.cpp

+  // First, wait for all pending UserRRefs to be confirmed,
+  // one kind is pendingUsers_, which are shared from Owner,
+  // the other kind pendingChildren_, which are shared from another User.
+  std::unique_lock<std::mutex> lock(mutex_);


looks like there is nothing that need to be shared across the three lock-unlock-lock blocks, we can use two lock_guard here to make the code more readable?

sounds good. updating.

mrshenli · 2020-03-12T03:46:10Z

torch/csrc/distributed/rpc/rref_context.cpp

+  // Note, there should be no new forkings in between, because it's assumed that
+  // this utility is called during graceful shutdown, where no new user RPCs can
+  // not be initiaited anymore.
+  auto tempConfirmedUsers = confirmedUsers_;


as erasing non-existing keys from unordered_map is fine, we can swap here?

Yeah, we can. It's more performant, like move semantics, but bi-directional.
Also, actually no one is expected to insert to confirmedUsers_ again, based on the assumption that it's called during graceful shutdown.

mrshenli · 2020-03-12T03:47:31Z

torch/csrc/distributed/rpc/rref_context.cpp

    const RRefId& rrefId,
    const TypePtr& type) {
-  std::lock_guard<std::mutex> lock(mutex_);
+  std::unique_lock<std::mutex> lock(mutex_);


This is one debugging vestige in #34610. I feel we don't need this in this PR. Let me test in #34610. If not necessary, let's revert these three lines.

mrshenli · 2020-03-12T03:48:13Z

torch/csrc/distributed/rpc/rref_impl.h

    return type_;
  }

+  virtual void tryDel() {}


Let's add comments to explain its behavior.

mrshenli

Could you please add a test for circular dependency? The example in #27096 should work.

xush6528 · 2020-03-12T05:29:56Z

Update: Use temporary ref to delay UserRRef destruction until lock released.

xush6528 · 2020-03-12T05:39:55Z

Added test case to show the "circular dependency" in #27096.
To be precise, it's because Python GC doesn't collect local circular dependent object, unless explicitly call gc.collect(). UserRRefs held by such Python objects will not be deleted in time.

…ng down" Stack from [ghstack](https://github.com/ezyang/ghstack): * **#31893 Delete all user forks tracked in RRefContext before shutting down** In order to resolve the issue summarized in #31325. The overal solution is to proactively send out delete fork messages from user nodes, before user nodes detecting rref leaks. As the first step, we want to have a weak ref tracker to track all user rrefs. Differential Revision: [D19292254](https://our.internmc.facebook.com/intern/diff/D19292254/) [ghstack-poisoned]

…xt before shutting down" Stack from [ghstack](https://github.com/ezyang/ghstack): * **#31893 Delete all user forks tracked in RRefContext before shutting down** In order to resolve the issue summarized in #31325. The overal solution is to proactively send out delete fork messages from user nodes, before user nodes detecting rref leaks. As the first step, we want to have a weak ref tracker to track all user rrefs. Differential Revision: [D19292254](https://our.internmc.facebook.com/intern/diff/D19292254/) [ghstack-poisoned]

… down Pull Request resolved: #31893 In order to resolve the issue summarized in #31325. The overal solution is to proactively send out delete fork messages from user nodes, before user nodes detecting rref leaks. As the first step, we want to have a weak ref tracker to track all user rrefs. ghstack-source-id: 100023142 Differential Revision: [D19292254](https://our.internmc.facebook.com/intern/diff/D19292254/)

mrshenli

Awesome!! Finally, we got all tests passed. Thanks a lot for spending so much time on this PR. LGTM! Let's land!

mrshenli · 2020-03-12T13:40:20Z

torch/csrc/distributed/rpc/init.cpp


 namespace {

+constexpr std::chrono::milliseconds kDeleteAllUsersTimeout(100000);


This is looks good to me for now. In future PRs we probably would like to expose this to users for configure?

Yes, I prefer not exposing to shutdown API at this moment.

Because I think users should only care to pass a top-level timeout, that includes
0) wait_for_all_workers, to flush any user RPC requests.

wait for pending,

send and wait delete user forks,

wait for owners to be removed.

wait for join().

This will require utils::Future to take timeout on it's wait() function, and require RpcAgent sync()/join() to take timeout as well.

facebook-github-bot · 2020-03-12T22:22:09Z

This pull request has been merged in 4e07c35.

…wner, instead of adding nothing to pending users (#34988) Summary: Pull Request resolved: #34988 In #31893, we introduced a confirmedUsers_ map in RRefContext. For the case that the fork is shared from the owner, there is no `pendingUsers_` intermediate phase for this fork, we should put this fork into `confirmedUsers_` immediately. Test Plan: ``` buck test mode/dev-nosan //caffe2/test/distributed/rpc:rpc_fork ``` ``` buck test mode/dev-nosan //caffe2/test/distributed/rpc/jit:rpc_fork ``` Differential Revision: D7735909 fbshipit-source-id: 14c36a16486f0cc9618dcfb111fe5223781b647d

xush6528 requested review from mrshenli, pritamdamania87 and zhaojuanmao as code owners January 6, 2020 20:14

xush6528 mentioned this pull request Jan 7, 2020

Implement backend-agnostic rpc._wait_all_workers() utility #31888

Closed

xush6528 changed the title ~~Add user forks tracker in RRefContext~~ Delete all user forks tracked in RRefContext before shutting down Jan 7, 2020

mrshenli reviewed Jan 7, 2020

View reviewed changes

torch/csrc/distributed/rpc/rref_context.h Outdated Show resolved Hide resolved

torch/csrc/distributed/rpc/rref_context.cpp Outdated Show resolved Hide resolved

mrshenli mentioned this pull request Jan 9, 2020

RRefs can leak when joining workers #27928

Closed

rohan-varma reviewed Jan 9, 2020

View reviewed changes

torch/csrc/distributed/rpc/rref_impl.cpp Outdated Show resolved Hide resolved

xush6528 mentioned this pull request Jan 15, 2020

Implement backend-agnostic rpc._wait_all_workers() utility #32190

Closed

xush6528 changed the title ~~Delete all user forks tracked in RRefContext before shutting down~~ [WIP, debugging] Delete all user forks tracked in RRefContext before shutting down Jan 15, 2020

xush6528 requested a review from osalpekar January 15, 2020 23:57

xush6528 changed the title ~~[WIP, debugging] Delete all user forks tracked in RRefContext before shutting down~~ Delete all user forks tracked in RRefContext before shutting down Jan 16, 2020

xush6528 mentioned this pull request Jan 16, 2020

RRef leak for other RPC Agents #31325

Closed

pritamdamania87 suggested changes Jan 16, 2020

View reviewed changes

torch/csrc/distributed/rpc/rref_context.h Outdated Show resolved Hide resolved

test/rpc_test.py Outdated Show resolved Hide resolved

torch/csrc/distributed/rpc/rref_impl.h Outdated Show resolved Hide resolved

torch/csrc/distributed/rpc/rref_impl.cpp Outdated Show resolved Hide resolved

mrshenli reviewed Jan 16, 2020

View reviewed changes

torch/csrc/distributed/rpc/rref_context.h Outdated Show resolved Hide resolved

mrshenli reviewed Jan 16, 2020

View reviewed changes

xush6528 changed the title ~~Delete all user forks tracked in RRefContext before shutting down~~ [WIP, debugging] Delete all user forks tracked in RRefContext before shutting down Jan 16, 2020

mrshenli reviewed Mar 12, 2020

View reviewed changes

mrshenli approved these changes Mar 12, 2020

View reviewed changes

facebook-github-bot closed this in 4e07c35 Mar 12, 2020

facebook-github-bot added the merged label Mar 12, 2020

mrshenli mentioned this pull request Mar 13, 2020

[Don't Review][Don't Mege] test RRef deadlock #34610

Closed

facebook-github-bot deleted the gh/xush6528/55/head branch March 16, 2020 14:16

This was referenced Mar 16, 2020

[1.5 Release][RPC Reliability] RRef Idempotency and RPC Retry enablement #33636

Closed

Nuke local UserRRefs when shutting down RPC #30105

Closed

xush6528 mentioned this pull request Mar 18, 2020

[RPC] Add to confirmed users immediately if the fork is shared from owner, instead of adding nothing to pending users #34988

Closed

mrshenli mentioned this pull request Apr 6, 2020

RFC: Proper timeout support with rpc.remote() API #36000

Closed

mruberry added the Merged label Oct 28, 2020


		namespace {

		constexpr std::chrono::milliseconds kDeleteAllUsersTimeout(100000);

Conversation

xush6528 commented Jan 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kostmo commented Jan 7, 2020 • edited by dr-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💊 CircleCI build failures summary and remediations

Uh oh!

Uh oh!

pritamdamania87 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

mrshenli left a comment

Choose a reason for hiding this comment

Uh oh!

xush6528 commented Mar 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrshenli commented Mar 11, 2020

Uh oh!

xush6528 commented Mar 11, 2020

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!

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!

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!

mrshenli left a comment

Choose a reason for hiding this comment

Uh oh!

xush6528 commented Mar 12, 2020

Uh oh!

xush6528 commented Mar 12, 2020 • 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!

xush6528 Mar 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

xush6528 commented Jan 6, 2020 •

edited

Loading

kostmo commented Jan 7, 2020 •

edited by dr-ci bot

Loading

xush6528 commented Mar 11, 2020 •

edited

Loading

xush6528 commented Mar 12, 2020 •

edited

Loading

xush6528 Mar 12, 2020 •

edited

Loading