[BE][DTensor] fix DTensor equal op #99014
Closed
Conversation
This file contains 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
[ghstack-poisoned]
XilunWu
requested review from
mrshenli,
zhaojuanmao,
rohan-varma,
H-Huang,
awgu,
kwen2501,
wanchaol and
fegin
as code owners
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/99014
Note: Links to docs will display an error until the docs builds have been completed. ❌ 3 FailuresAs of commit f259a47: NEW FAILURES - The following jobs have failed:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
[ghstack-poisoned]
XilunWu
added a commit
that referenced
this pull request
Apr 13, 2023
ghstack-source-id: 2ce007350bf07010b33a78c535c513024220a5e2 Pull Request resolved: #99014
[ghstack-poisoned]
[ghstack-poisoned]
XilunWu
added a commit
that referenced
this pull request
Apr 13, 2023
ghstack-source-id: b5445b8b47af08c6d68506b9e43dadc1d1a28f60 Pull Request resolved: #99014
wanchaol
reviewed
Apr 13, 2023
[ghstack-poisoned]
XilunWu
added a commit
that referenced
this pull request
Apr 13, 2023
ghstack-source-id: 65ee5fb45a49bce3ed1ffb70c9ab23fecb926bad Pull Request resolved: #99014
|
@pytorchbot rebase |
|
@pytorchbot successfully started a rebase job. Check the current status here |
## What problem this PR solves? #97170 fixed `equal` operator return type (old: Tensor, now: bool) by giving it the correct sharding propagation. This is consistent with the `aten::equal` op. However, the correctness only stays at the local result level: * `equal` op returns True if the local copy of dtensor A equals to the the local copy of dtensor B This is not the correct semantic of `equal` which should return True if all local copies of A are equal to the corresponding local copies of B. ## What is this PR? 1. For non-participating ranks, if the return type is scalar, `local_results` is set to `None` which means the default value is a reduced result of participating ranks only. 2. For all ranks, if the return type is scalar and the `op_call` is `aten::equal`(because `aten::equal` is the only function that returns scalar value and needs communication), all gather the `local_results` within the `default pg` and reduce on them with `operator.and_`. The result will be the new `local_result`. ## Result/Impact For non-participating ranks and the return type is scalar: 1. op is `aten::equal`, the return value is same with all other ranks 2. op is not `aten::equal`, the return value is None. Before this PR, this will raise "NotImplementedError" but has not been tested. For participating ranks and the return type is scalar: 1. op is `aten::equal`, the return value is the equality of two dtensor operands - True if all copies are equal, False otherwise. 2. op is not `aten::equal`, simply the local computation result. [ghstack-poisoned]
|
Successfully rebased |
wanchaol
approved these changes
Apr 17, 2023
## What problem this PR solves? #97170 fixed `equal` operator return type (old: Tensor, now: bool) by giving it the correct sharding propagation. This is consistent with the `aten::equal` op. However, the correctness only stays at the local result level: * `equal` op returns True if the local copy of dtensor A equals to the the local copy of dtensor B This is not the correct semantic of `equal` which should return True if all local copies of A are equal to the corresponding local copies of B. ## What is this PR? 1. For non-participating ranks, if the return type is scalar, `local_results` is set to `None` which means the default value is a reduced result of participating ranks only. 2. For all ranks, if the return type is scalar and the `op_call` is `aten::equal`(because `aten::equal` is the only function that returns scalar value and needs communication), all gather the `local_results` within the `default pg` and reduce on them with `operator.and_`. The result will be the new `local_result`. ## Result/Impact For non-participating ranks and the return type is scalar: 1. op is `aten::equal`, the return value is same with all other ranks 2. op is not `aten::equal`, the return value is None. Before this PR, this will raise "NotImplementedError" but has not been tested. For participating ranks and the return type is scalar: 1. op is `aten::equal`, the return value is the equality of two dtensor operands - True if all copies are equal, False otherwise. 2. op is not `aten::equal`, simply the local computation result. [ghstack-poisoned]
|
@pytorchbot rebase |
|
@pytorchbot successfully started a rebase job. Check the current status here |
|
Tried to rebase and push PR #99014, but it was already up to date |
## What problem this PR solves? #97170 fixed `equal` operator return type (old: Tensor, now: bool) by giving it the correct sharding propagation. This is consistent with the `aten::equal` op. However, the correctness only stays at the local result level: * `equal` op returns True if the local copy of dtensor A equals to the the local copy of dtensor B This is not the correct semantic of `equal` which should return True if all local copies of A are equal to the corresponding local copies of B. ## What is this PR? 1. For non-participating ranks, if the return type is scalar, `local_results` is set to `None` which means the default value is a reduced result of participating ranks only. 2. For all ranks, if the return type is scalar and the `op_call` is `aten::equal`(because `aten::equal` is the only function that returns scalar value and needs communication), all gather the `local_results` within the `default pg` and reduce on them with `operator.and_`. The result will be the new `local_result`. ## Result/Impact For non-participating ranks and the return type is scalar: 1. op is `aten::equal`, the return value is same with all other ranks 2. op is not `aten::equal`, the return value is None. Before this PR, this will raise "NotImplementedError" but has not been tested. For participating ranks and the return type is scalar: 1. op is `aten::equal`, the return value is the equality of two dtensor operands - True if all copies are equal, False otherwise. 2. op is not `aten::equal`, simply the local computation result. [ghstack-poisoned]
XilunWu
added a commit
that referenced
this pull request
Apr 17, 2023
ghstack-source-id: eced00444301025c602f49362f542fbac2439d79 Pull Request resolved: #99014
|
@pytorchmergebot merge -f "CI failure not related to PR" |
Merge startedYour change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes). Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What problem this PR solves?
#97170 fixed
equaloperator return type (old: Tensor, now: bool) by giving it the correct sharding propagation. This is consistent with theaten::equalop. However, the correctness only stays at the local result level:equalop returns True if the local copy of dtensor A equals to the the local copy of dtensor BThis is not the correct semantic of
equalwhich should return True if all local copies of A are equal to the corresponding local copies of B.What is this PR?
local_resultsis set toNonewhich means the default value is a reduced result of participating ranks only.op_callisaten::equal(becauseaten::equalis the only function that returns scalar value and needs communication), all gather thelocal_resultswithin thedefault pgand reduce on them withoperator.and_. The result will be the newlocal_result.Result/Impact
For non-participating ranks and the return type is scalar:
aten::equal, the return value is same with all other ranksaten::equal, the return value is None. Before this PR, this will raise "NotImplementedError" but has not been tested.For participating ranks and the return type is scalar:
aten::equal, the return value is the equality of two dtensor operands - True if all copies are equal, False otherwise.aten::equal, simply the local computation result.Stack from ghstack (oldest at bottom):