Migrate OwnerRRef value store to generic torch Future #38143
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
It's a followup of #32556, where an error handling boilerplate code path was added to the FutureMessage callback. However, I noticed that the FutureMessage could never be set with an error, because the FutureMessage is a member in OwnerRRef, - OwnerRRef does not have a setError method yet. - The FutureMessage is only used for signaling - The value of the RRef is contained in the `value_` field. With the Future being generalized, it could contain more value types, not limited to Message. This PR migrates the OwnerRRef value from the `value_` field to the generic Future. In a later PR, it will be super easy to add a `setError` method for OwnerRRef, which calls `future_.setError(..)`. (I decide to do it later. I think it's better to migrate the call sites together with adding the new `setError` method.) Also, this fixes the issue pointed out by https://github.com/pytorch/pytorch/pull/31086/files#r422256916. This PR was submitted as #32608. Differential Revision: [D5707692](https://our.internmc.facebook.com/intern/diff/D5707692/) [ghstack-poisoned]
xush6528
requested review from
mrshenli,
pritamdamania87 and
zhaojuanmao
as code owners
xush6528
added a commit
that referenced
this pull request
May 8, 2020
It's a followup of #32556, where an error handling boilerplate code path was added to the FutureMessage callback. However, I noticed that the FutureMessage could never be set with an error, because the FutureMessage is a member in OwnerRRef, - OwnerRRef does not have a setError method yet. - The FutureMessage is only used for signaling - The value of the RRef is contained in the `value_` field. With the Future being generalized, it could contain more value types, not limited to Message. This PR migrates the OwnerRRef value from the `value_` field to the generic Future. In a later PR, it will be super easy to add a `setError` method for OwnerRRef, which calls `future_.setError(..)`. (I decide to do it later. I think it's better to migrate the call sites together with adding the new `setError` method.) Also, this fixes the issue pointed out by https://github.com/pytorch/pytorch/pull/31086/files#r422256916. This PR was submitted as #32608. Differential Revision: [D5707692](https://our.internmc.facebook.com/intern/diff/D5707692/) ghstack-source-id: 103757743 Pull Request resolved: #38143
mrshenli
approved these changes
May 8, 2020
|
This pull request has been merged in 615235f. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Stack from ghstack:
It's a followup of #32556, where an error handling boilerplate code path was added to the FutureMessage callback.
However, I noticed that the FutureMessage could never be set with an error, because the FutureMessage is a member in OwnerRRef,
value_field.With the Future being generalized, it could contain more value types, not limited to Message.
This PR migrates the OwnerRRef value from the
value_field to the generic Future.In a later PR, it will be super easy to add a
setErrormethod for OwnerRRef, which callsfuture_.setError(..). (I decide to do it later. I think it's better to migrate the call sites together with adding the newsetErrormethod.)Also, this fixes the issue pointed out by https://github.com/pytorch/pytorch/pull/31086/files#r422256916.
This PR was submitted as #32608.
Differential Revision: D5707692