Let RpcAgent::send() return JitFuture #49906

mrshenli · 2020-12-28T23:27:55Z

Stack from ghstack:

Apply clang-format to rpc cpp files #50236 Apply clang-format to rpc cpp files
Completely remove FutureMessage type #50029 Completely remove FutureMessage type
Completely Remove FutureMessage from RPC agents #50028 Completely Remove FutureMessage from RPC agents
Completely Remove FutureMessage from RPC cpp tests #50027 Completely Remove FutureMessage from RPC cpp tests
Remove FutureMessage from RPC request callback logic #50026 Remove FutureMessage from RPC request callback logic
Completely remove FutureMessage from FaultyProcessGroupAgent #50025 Completely remove FutureMessage from FaultyProcessGroupAgent
Remove FutureMessage from sender TensorPipeAgent #50024 Remove FutureMessage from sender TensorPipeAgent
Remove FutureMessage from sender ProcessGroupAgent #50023 Remove FutureMessage from sender ProcessGroupAgent
Completely remove FutureMessage from distributed autograd #50020 Completely remove FutureMessage from distributed autograd
Completely remove FutureMessage from RPC TorchScript implementations #50005 Completely remove FutureMessage from RPC TorchScript implementations
Completely remove FutureMessage from RRef Implementations #50004 Completely remove FutureMessage from RRef Implementations
Replace FutureMessage with ivalue::Future in RpcAgent retry logic #49995 Replace FutureMessage with ivalue::Future in RpcAgent retry logic
Replace FutureMessage with ivalue::Future in RRefContext #49960 Replace FutureMessage with ivalue::Future in RRefContext
Replace FutureMessage with ivalue::Future in distributed/autograd/utils.* #49927 Replace FutureMessage with ivalue::Future in distributed/autograd/utils.*
Let RpcAgent::send() return JitFuture #49906 Let RpcAgent::send() return JitFuture

This commit modifies RPC Message to inherit from torch::CustomClassHolder,
and wraps a Message in an IValue in RpcAgent::send().

Differential Revision: D25719518

This commit modifies RPC Message to inherit from `torch::CustomClassHolder`, and wraps a Message in an IValue in `RpcAgent::send()`. [ghstack-poisoned]

facebook-github-bot · 2020-12-28T23:28:12Z

💊 CI failures summary and remediations

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

1/1 failures possibly* introduced in this PR
- 1/1 non-CircleCI failure(s)

ci.pytorch.org: 1 failed

Failed: pr/caffe2-pytorch-linux-bionic-rocm3.10-py3.6-test

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 to the (internal) Dr. CI Users group.

This comment has been revised 75 times.

This commit modifies RPC Message to inherit from `torch::CustomClassHolder`, and wraps a Message in an IValue in `RpcAgent::send()`. [ghstack-poisoned]

This commit modifies RPC Message to inherit from `torch::CustomClassHolder`, and wraps a Message in an IValue in `RpcAgent::send()`. ghstack-source-id: 49f0b3caaef1245efe1e2c7afa8dd61fac873caf Pull Request resolved: #49906

torch/csrc/distributed/rpc/rpc_agent.h

pritamdamania87

Left a few comments on the internal diff.

This commit modifies RPC Message to inherit from `torch::CustomClassHolder`, and wraps a Message in an IValue in `RpcAgent::send()`. Differential Revision: [D25719518](https://our.internmc.facebook.com/intern/diff/D25719518) [ghstack-poisoned]

This commit modifies RPC Message to inherit from `torch::CustomClassHolder`, and wraps a Message in an IValue in `RpcAgent::send()`. ghstack-source-id: 58569c04274c229e5aca69b4abdce8312d4feb98 Pull Request resolved: #49906

mrshenli · 2020-12-29T16:47:54Z

torch/csrc/distributed/rpc/init.cpp

+  // NB: need to call torch::class_ to register Message in the map returned by
+  // c10::getCustomClassTypeMap(). Otherwise, Message cannot be wrapped within
+  // an IValue.
+  static const auto message = torch::class_<Message>("rpc", "_Message");


Hey @wanchaol, are there plans to extract the custom type registration logic from torch::class_ to convert it into a utility function, so that c++ only IValue custom types no longer need to be bound here?

Sorry for the late reply. is this still a valid ask? I think if this is only for C++ used, we don't need to register/bound the class into python or TorchScript, just make the C++ class inherit torch::CustomClassHolder is enough

Hey @wanchaol, thanks for the comments. Yep, this is still relevant. I found that if I don't call torch::class_, IValue(c10::make_intrusive<Message>(std::move(msg))) would crash. I traced the code and found that it hits this line:

pytorch/aten/src/ATen/core/ivalue.h

Line 963 in 71ca600

throw c10::Error("Can't find class id in custom class type map", "");

I did a quick search, and looks like torch::class_ is only place that inserts new types:

pytorch/torch/custom_class.h

Lines 88 to 91 in 00a3add

c10::getCustomClassTypeMap().insert(

{std::type_index(typeid(c10::intrusive_ptr<CurClass>)), classTypePtr});

c10::getCustomClassTypeMap().insert(

{std::type_index(typeid(c10::tagged_capsule<CurClass>)), classTypePtr});

Please let me know if there are better suggestions. Thanks!

This commit modifies RPC Message to inherit from `torch::CustomClassHolder`, and wraps a Message in an IValue in `RpcAgent::send()`. Differential Revision: [D25719518](https://our.internmc.facebook.com/intern/diff/D25719518) [ghstack-poisoned]

This commit modifies RPC Message to inherit from `torch::CustomClassHolder`, and wraps a Message in an IValue in `RpcAgent::send()`. ghstack-source-id: da01047db164bd38c277f47884c8d8bd54d6468a Pull Request resolved: pytorch#49906

This commit modifies RPC Message to inherit from `torch::CustomClassHolder`, and wraps a Message in an IValue in `RpcAgent::send()`. Differential Revision: [D25719518](https://our.internmc.facebook.com/intern/diff/D25719518) [ghstack-poisoned]

mrshenli

This PR has internal changes

lw

The OSS changes look good to me. Although I'm not qualified on the JIT custom classes, so let's get a review from someone else too.

lw · 2021-01-05T10:12:49Z

torch/csrc/distributed/rpc/rpc_agent.h

@@ -157,7 +157,7 @@ class TORCH_API RpcAgent {
  // If ``message.isRequest()`` is true, the ``FutureMessage`` will be
  // completed when the response arrives. For other message types, the Future
  // should be ignored by the caller.
-  virtual std::shared_ptr<FutureMessage> send(
+  virtual std::shared_ptr<JitFuture> send(


I've mostly seen JitFuture being passed around through an intrusive_ptr, rather than a shared_ptr. I guess that's somewhat more efficient (not sure how much). Should we consider it here too?

@wanchaol @gmagogsfm is there any perf penalty if we use ivalue::Future with std::shared_ptr?

There shouldn't be too much perf penalty, it's just that shared_ptr store the refcount in a separate memory space instead of embedded in the object, so from cache locality prospective intrusive_ptr should be a bit faster. We passed around JitFuture through intrusive_ptr also for the reason of making it to be able to held by IValue so that it's easier to bind to TorchScript if needed.

I see, thanks for the explanation!

torch/csrc/distributed/rpc/message.cpp

This commit modifies RPC Message to inherit from `torch::CustomClassHolder`, and wraps a Message in an IValue in `RpcAgent::send()`. Differential Revision: [D25719518](https://our.internmc.facebook.com/intern/diff/D25719518) [ghstack-poisoned]

facebook-github-bot · 2021-01-08T03:51:19Z

@mrshenli merged this pull request in 84e3237.

Summary: Pull Request resolved: pytorch#49906 This commit modifies RPC Message to inherit from `torch::CustomClassHolder`, and wraps a Message in an IValue in `RpcAgent::send()`. Test Plan: Imported from OSS Reviewed By: lw Differential Revision: D25719518 Pulled By: mrshenli fbshipit-source-id: 694e40021e49e396da1620a2f81226522341550b

Let RpcAgent::send() return JitFuture

bd9ca7d

This commit modifies RPC Message to inherit from `torch::CustomClassHolder`, and wraps a Message in an IValue in `RpcAgent::send()`. [ghstack-poisoned]

mrshenli requested review from mingzhe09088, pritamdamania87, rohan-varma and zhaojuanmao as code owners December 28, 2020 23:27

facebook-github-bot added cla signed oncall: distributed Add this issue/PR to distributed oncall triage queue labels Dec 28, 2020

Update on "Let RpcAgent::send() return JitFuture"

e23cdc3

This commit modifies RPC Message to inherit from `torch::CustomClassHolder`, and wraps a Message in an IValue in `RpcAgent::send()`. [ghstack-poisoned]

Update on "Let RpcAgent::send() return JitFuture"

70651b3

This commit modifies RPC Message to inherit from `torch::CustomClassHolder`, and wraps a Message in an IValue in `RpcAgent::send()`. [ghstack-poisoned]

rohan-varma reviewed Dec 28, 2020

View reviewed changes

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

pritamdamania87 reviewed Dec 29, 2020

View reviewed changes

mrshenli mentioned this pull request Dec 29, 2020

Replace FutureMessage with ivalue::Future in distributed/autograd/utils.* #49927

Closed

mrshenli commented Dec 29, 2020

View reviewed changes

mrshenli mentioned this pull request Dec 30, 2020

Replace FutureMessage with ivalue::Future in RRefContext #49960

Closed

mrshenli mentioned this pull request Jan 1, 2021

Replace FutureMessage with ivalue::Future in RpcAgent retry logic #49995

Closed

mrshenli added 5 commits January 1, 2021 14:52

This was referenced Jan 3, 2021

Completely remove FutureMessage from RRef Implementations #50004

Closed

Completely remove FutureMessage from RPC TorchScript implementations #50005

Closed

mrshenli commented Jan 4, 2021

View reviewed changes

lw approved these changes Jan 5, 2021

View reviewed changes

mrshenli added 2 commits January 6, 2021 12:19

mrshenli mentioned this pull request Jan 7, 2021

Apply clang-format to rpc cpp files #50236

Closed

facebook-github-bot closed this in 84e3237 Jan 8, 2021

facebook-github-bot added the Merged label Jan 8, 2021

facebook-github-bot deleted the gh/mrshenli/268/head branch January 11, 2021 15:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Let RpcAgent::send() return JitFuture #49906

Let RpcAgent::send() return JitFuture #49906

mrshenli commented Dec 28, 2020 •

edited

Loading

facebook-github-bot commented Dec 28, 2020 •

edited

Loading

pritamdamania87 left a comment

mrshenli Dec 29, 2020

wanchaol Jan 5, 2021

mrshenli Jan 5, 2021

mrshenli left a comment

lw left a comment

lw Jan 5, 2021

mrshenli Jan 6, 2021

wanchaol Jan 6, 2021

mrshenli Jan 6, 2021

facebook-github-bot commented Jan 8, 2021

	c10::getCustomClassTypeMap().insert(
	{std::type_index(typeid(c10::intrusive_ptr<CurClass>)), classTypePtr});
	c10::getCustomClassTypeMap().insert(
	{std::type_index(typeid(c10::tagged_capsule<CurClass>)), classTypePtr});

Let RpcAgent::send() return JitFuture #49906

Let RpcAgent::send() return JitFuture #49906

Conversation

mrshenli commented Dec 28, 2020 • edited Loading

facebook-github-bot commented Dec 28, 2020 • edited Loading

💊 CI failures summary and remediations

ci.pytorch.org: 1 failed

pritamdamania87 left a comment

Choose a reason for hiding this comment

mrshenli Dec 29, 2020

Choose a reason for hiding this comment

wanchaol Jan 5, 2021

Choose a reason for hiding this comment

mrshenli Jan 5, 2021

Choose a reason for hiding this comment

mrshenli left a comment

Choose a reason for hiding this comment

lw left a comment

Choose a reason for hiding this comment

lw Jan 5, 2021

Choose a reason for hiding this comment

mrshenli Jan 6, 2021

Choose a reason for hiding this comment

wanchaol Jan 6, 2021

Choose a reason for hiding this comment

mrshenli Jan 6, 2021

Choose a reason for hiding this comment

facebook-github-bot commented Jan 8, 2021

mrshenli commented Dec 28, 2020 •

edited

Loading

facebook-github-bot commented Dec 28, 2020 •

edited

Loading