Refactor wrap_fx_proxy_cls, no symbolic_shapes test anymore #103303
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
…eems risky. Signed-off-by: Edward Z. Yang <ezyang@meta.com> [ghstack-poisoned]
ezyang
added a commit
that referenced
this pull request
Jun 9, 2023
…eems risky. Signed-off-by: Edward Z. Yang <ezyangmeta.com> ghstack-source-id: 5a81b5b661bf0c7615c5d44ae3ed08eddbe0c58e Pull Request resolved: #103303
github-actions
bot
requested review from
albanD,
antoniojkim,
bdhirsh,
jbschlosser,
miladm,
SherlockNoMad,
voznesenskym and
wconstab
…cls. This seems risky." Signed-off-by: Edward Z. Yang <ezyangmeta.com> cc voznesenskym penguinwu anijain2305 EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx ipiszy aakhundov [ghstack-poisoned]
ezyang
added a commit
that referenced
this pull request
Jun 12, 2023
Signed-off-by: Edward Z. Yang <ezyangmeta.com> ghstack-source-id: 62e0cdc7373ecfd7185e0a014864eab542d6695c Pull Request resolved: #103303
ezyang
changed the title
ezyang
changed the title
Signed-off-by: Edward Z. Yang <ezyangmeta.com> cc voznesenskym penguinwu anijain2305 EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx ipiszy aakhundov [ghstack-poisoned]
ezyang
added a commit
that referenced
this pull request
Jun 12, 2023
Signed-off-by: Edward Z. Yang <ezyangmeta.com> ghstack-source-id: 5738a2141baa95f6ccb7e431da961beb2a80d5ea Pull Request resolved: #103303
| @@ -1059,7 +1059,10 @@ def _dataclasses_fields_lambda(obj): | |||
| return TupleVariable(items).add_options(obj) | |||
|
|
|||
|
|
|||
| def wrap_fx_proxy(tx, proxy, example_value=None, **options): | |||
| no_example = object() | |||
|
Obsoleted by #103438 |
ezyang
added a commit
that referenced
this pull request
Jul 9, 2023
Per #103303 we cannot universally allow tracing in all functions that return int, as the graph breaks appear to be load bearing in some cases. However, allowing for torch.sym_int to be traced in even if the result is statically known is fine; this can happen in case of a SymBool to int conversion. This PR is not exhaustive but e.g., I fixed size/stride/numel handling in #103438 The biggest risk is that arithmetic operations on sizes end up getting constant-ified (this appears to have happened in practice for modulus, which is why it's in this list.) If we don't care about spewing useless computation into the graph, a more aggressive version of this PR would be to greatly expand the list of allowed to specialize to int targets and then undo #103438 Signed-off-by: Edward Z. Yang <ezyang@meta.com> [ghstack-poisoned]
ezyang
added a commit
that referenced
this pull request
Jul 9, 2023
Per #103303 we cannot universally allow tracing in all functions that return int, as the graph breaks appear to be load bearing in some cases. However, allowing for torch.sym_int to be traced in even if the result is statically known is fine; this can happen in case of a SymBool to int conversion. This PR is not exhaustive but e.g., I fixed size/stride/numel handling in #103438 The biggest risk is that arithmetic operations on sizes end up getting constant-ified (this appears to have happened in practice for modulus, which is why it's in this list.) If we don't care about spewing useless computation into the graph, a more aggressive version of this PR would be to greatly expand the list of allowed to specialize to int targets and then undo #103438 Signed-off-by: Edward Z. Yang <ezyangmeta.com> ghstack-source-id: d63980a89a67822f69a606df60f6f17f3cc186bb Pull Request resolved: #104837
pytorchmergebot
pushed a commit
that referenced
this pull request
Jul 9, 2023
Per #103303 we cannot universally allow tracing in all functions that return int, as the graph breaks appear to be load bearing in some cases. However, allowing for torch.sym_int to be traced in even if the result is statically known is fine; this can happen in case of a SymBool to int conversion. This PR is not exhaustive but e.g., I fixed size/stride/numel handling in #103438 The biggest risk is that arithmetic operations on sizes end up getting constant-ified (this appears to have happened in practice for modulus, which is why it's in this list.) If we don't care about spewing useless computation into the graph, a more aggressive version of this PR would be to greatly expand the list of allowed to specialize to int targets and then undo #103438 Signed-off-by: Edward Z. Yang <ezyang@meta.com> Pull Request resolved: #104837 Approved by: https://github.com/voznesenskym
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.
Stack from ghstack (oldest at bottom):
Originally, wrap_fx_proxy_cls had a thicket of conditions handling a lot of cases that, frankly, didn't make any sense to me. This refactor attempts to make sense out of the madness. Some important basic knowledge:
example_value is Nonewas previously overloaded for two conditions: (1) please compute the example value by runningget_fake_valueand (2) my example value is literally None please give me a ConstantVariable(None)The refactor proceeds as follows:
example_valuewithno_examplesingleton object, so we can distinguish it from None. This allows us to remove the attention return None special case. There are a few sites in code where None is explicitly passed in asexample_valuewhen they shouldn't be; they're updated to rely on the default argument.One thing I didn't fix is the special case for TupleVariable
target_cls. I think this is just a straight up bug but I didn't feel like fixing that corner of the codebase, so leaving a mess for now.Signed-off-by: Edward Z. Yang ezyang@meta.com
cc @voznesenskym @penguinwu @anijain2305 @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @Xia-Weiwen @wenzhe-nrv @jiayisunx @ipiszy @aakhundov