Completely Remove FutureMessage from RPC agents #50028
Conversation
[ghstack-poisoned]
mrshenli
requested review from
mingzhe09088,
pritamdamania87,
rohan-varma and
zhaojuanmao
as code owners
facebook-github-bot
added
cla signed
oncall: distributed
💊 CI failures summary and remediationsAs of commit 1de8a22 (more details on the Dr. CI page):
ci.pytorch.org: 1 failedThis 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 17 times. |
ghstack-source-id: 34cc2d66b97bd0c956e635b38277426eacd6fb24 Pull Request resolved: #50028
Differential Revision: [D25753887](https://our.internmc.facebook.com/intern/diff/D25753887) [ghstack-poisoned]
| auto futureResponse = | ||
| std::make_shared<JitFuture>(at::AnyClassType::get()); |
There was a problem hiding this comment.
These lines are different than the ones they're replacing: before we were creating an empty (null) shared_ptr, here we create a "full" shared_ptr that points to an uncompleted future. It also appears that immediately after we re-set this shared_ptr to a new value, hence destroying that uncompleted future. I think it makes sense to avoid creating it in the first place.
| if (!error) { | ||
| if (!futureResponseMessage->hasError()) { | ||
| Message&& responseMessage = | ||
| std::move(*futureResponseMessage->value().toCustomClass<Message>()); |
There was a problem hiding this comment.
I wonder: does it make sense to add a moveValue() method to ivalue::Future? For cases like this where we don't care about the future anymore and just want its content?
(Personally I think the "idiomatic" way would be to have a &&-qualified value() method but I've seen that in PyTorch that's not always the convention...)
There was a problem hiding this comment.
Sure, I can add that. cc @wanchaol if there are concerns for adding value() &&.
| const c10::optional<utils::FutureError> error = | ||
| futureResponseMessage->error(); | ||
| Message&& responseMessage = std::move(*futureResponseMessage).moveValue(); | ||
| responseMessage.setId(messageId); |
There was a problem hiding this comment.
Where did this line go? Is it not needed?
There was a problem hiding this comment.
Yep, I don't think this is necessary as request cbs have already set the id for the response message. But let me add it back for safety.
| @@ -572,12 +570,14 @@ void TensorPipeAgent::respond(std::shared_ptr<tensorpipe::Pipe>& pipe) { | |||
| << " is running request #" << messageId << " from " | |||
| << pipe->getRemoteName() << " in thread pool"; | |||
|
|
|||
| std::shared_ptr<FutureMessage> futureResponseMessage; | |||
| auto futureResponseMessage = | |||
| std::make_shared<JitFuture>(at::AnyClassType::get()); | |||
There was a problem hiding this comment.
Same as above: we're allocating an uncompleted JitFuture just to immediately replacing it (and thus deallocating it). It's probably better to leave the shared_ptr unset.
Differential Revision: [D25753887](https://our.internmc.facebook.com/intern/diff/D25753887) [ghstack-poisoned]
Differential Revision: [D25753887](https://our.internmc.facebook.com/intern/diff/D25753887) [ghstack-poisoned]
Summary: Pull Request resolved: pytorch#50028 Test Plan: Imported from OSS Reviewed By: lw Differential Revision: D25753887 Pulled By: mrshenli fbshipit-source-id: 40718349c2def262a16aaa24c167c0b540cddcb1
Stack from ghstack:
Differential Revision: D25753887