read meta["val"] from arg for place holders instead of generating a fake tensor using fake_tensor_mode.from_real_tensor #6550
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
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/6550
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit 4d2d511 with merge base f0463c4 ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
facebook-github-bot
added
the
CLA Signed
Contributor
|
This pull request was exported from Phabricator. Differential Revision: D65120377 |
laithsakka
changed the title
laithsakka
force-pushed
the
export-D65120377
branch
from
856d5e0 to
6b1de41
Compare
laithsakka
added a commit
to laithsakka/executorch
that referenced
this pull request
Oct 30, 2024
…ake tensor using fake_tensor_mode.from_real_tensor (pytorch#6550) Summary: re-generating the inputs (the fake tensors from real tensors) can change some dimension from static to dynamic, this does not sounds to me to be part of the intention of those custom passes . But this can have complication, for details of the issue read below, what i suggest here is just read the meta['val'] from the input graph instead of regenerating a new fake tensor. a failure here https://www.internalfb.com/diff/D65023233 that is related to executorch. 0) one of the passes changes the dimension of the one of the inputs from [2, 2, 3, 1] to f32[s0, 2, 7 - s2] (because calling from fake_tensor_mode.from_real_tensor can do that) 1) after running passes execuotorch revert the changes of the meta['val] values for all inputs to match the old graph values before the pass did run. (https://www.internalfb.com/code/fbsource/[92156447ae2c]/xplat/executorch/exir/pass_base.py?lines=688) 2) But executorch only do that for inputs and not for the internal nodes of the graph. 3) in this case that one of the inputs from: ``` def forward(self, p_conv1_bias: "f32[2]", p_conv2_bias: "f32[2]", `b__frozen_param0: "i8[2, 2, s2, 1]", b__frozen_param1: "i8[2, 2, s2, 1]", x: "f32[s0, 2, 4]"):``` to ```def forward(self, p_conv1_bias: "f32[2]", p_conv2_bias: "f32[2]", b__frozen_param0: "i8[2, 2, 3, 1]", b__frozen_param1: "i8[2, 2, 3, 1]", x: "f32[s0, 2, 4]"):``` replacing every s2 with 3 in the place holders only **but executorch do not apply the changes back to internal nodes** we we still have i8[s0, 2, 7 - s2, 1] in the graph (s2 coming from no where now!) 5) executorch runs more passes on this (ruined graph) 6) we get an error in one internal node operation that checks that the a dimension have only one dynamic shape. quantized_decomposed_quantize_per_tensor_default_4: "i8[s0, 2, 7 - s2, 1]" = executorch_exir_dialects_edge__ops_quantized_decomposed_quantize_per_tensor_default(aten_unsqueeze_copy_default_1, 0.012759864330291748, 55, -128, 127, torch.int8); aten_unsqueeze_copy_default_1 = None which is right since s2 was replaced with 3, but the change was not propagated to all internal nodes. Differential Revision: D65120377
Contributor
|
This pull request was exported from Phabricator. Differential Revision: D65120377 |
laithsakka
added a commit
to laithsakka/executorch
that referenced
this pull request
Oct 30, 2024
…ake tensor using fake_tensor_mode.from_real_tensor (pytorch#6550) Summary: re-generating the inputs (the fake tensors from real tensors) can change some dimension from static to dynamic, this does not sounds to me to be part of the intention of those custom passes . But this can have complication, for details of the issue read below, what i suggest here is just read the meta['val'] from the input graph instead of regenerating a new fake tensor. a failure here https://www.internalfb.com/diff/D65023233 that is related to executorch. 0) one of the passes changes the dimension of the one of the inputs from [2, 2, 3, 1] to f32[s0, 2, 7 - s2] (because calling from fake_tensor_mode.from_real_tensor can do that) 1) after running passes execuotorch revert the changes of the meta['val] values for all inputs to match the old graph values before the pass did run. (https://www.internalfb.com/code/fbsource/[92156447ae2c]/xplat/executorch/exir/pass_base.py?lines=688) 2) But executorch only do that for inputs and not for the internal nodes of the graph. 3) in this case that one of the inputs from: ``` def forward(self, p_conv1_bias: "f32[2]", p_conv2_bias: "f32[2]", `b__frozen_param0: "i8[2, 2, s2, 1]", b__frozen_param1: "i8[2, 2, s2, 1]", x: "f32[s0, 2, 4]"):``` to ```def forward(self, p_conv1_bias: "f32[2]", p_conv2_bias: "f32[2]", b__frozen_param0: "i8[2, 2, 3, 1]", b__frozen_param1: "i8[2, 2, 3, 1]", x: "f32[s0, 2, 4]"):``` replacing every s2 with 3 in the place holders only **but executorch do not apply the changes back to internal nodes** we we still have i8[s0, 2, 7 - s2, 1] in the graph (s2 coming from no where now!) 5) executorch runs more passes on this (ruined graph) 6) we get an error in one internal node operation that checks that the a dimension have only one dynamic shape. quantized_decomposed_quantize_per_tensor_default_4: "i8[s0, 2, 7 - s2, 1]" = executorch_exir_dialects_edge__ops_quantized_decomposed_quantize_per_tensor_default(aten_unsqueeze_copy_default_1, 0.012759864330291748, 55, -128, 127, torch.int8); aten_unsqueeze_copy_default_1 = None which is right since s2 was replaced with 3, but the change was not propagated to all internal nodes. Differential Revision: D65120377
laithsakka
force-pushed
the
export-D65120377
branch
from
6b1de41 to
b0c1e51
Compare
Contributor
|
This pull request was exported from Phabricator. Differential Revision: D65120377 |
…ake tensor using fake_tensor_mode.from_real_tensor (pytorch#6550) Summary: re-generating the inputs (the fake tensors from real tensors) can change some dimension from static to dynamic, this does not sounds to me to be part of the intention of those custom passes . But this can have complication, for details of the issue read below, what i suggest here is just read the meta['val'] from the input graph instead of regenerating a new fake tensor. a failure here https://www.internalfb.com/diff/D65023233 that is related to executorch. 0) one of the passes changes the dimension of the one of the inputs from [2, 2, 3, 1] to f32[s0, 2, 7 - s2] (because calling from fake_tensor_mode.from_real_tensor can do that) 1) after running passes execuotorch revert the changes of the meta['val] values for all inputs to match the old graph values before the pass did run. (https://www.internalfb.com/code/fbsource/[92156447ae2c]/xplat/executorch/exir/pass_base.py?lines=688) 2) But executorch only do that for inputs and not for the internal nodes of the graph. 3) in this case that one of the inputs from: ``` def forward(self, p_conv1_bias: "f32[2]", p_conv2_bias: "f32[2]", `b__frozen_param0: "i8[2, 2, s2, 1]", b__frozen_param1: "i8[2, 2, s2, 1]", x: "f32[s0, 2, 4]"):``` to ```def forward(self, p_conv1_bias: "f32[2]", p_conv2_bias: "f32[2]", b__frozen_param0: "i8[2, 2, 3, 1]", b__frozen_param1: "i8[2, 2, 3, 1]", x: "f32[s0, 2, 4]"):``` replacing every s2 with 3 in the place holders only **but executorch do not apply the changes back to internal nodes** we we still have i8[s0, 2, 7 - s2, 1] in the graph (s2 coming from no where now!) 5) executorch runs more passes on this (ruined graph) 6) we get an error in one internal node operation that checks that the a dimension have only one dynamic shape. quantized_decomposed_quantize_per_tensor_default_4: "i8[s0, 2, 7 - s2, 1]" = executorch_exir_dialects_edge__ops_quantized_decomposed_quantize_per_tensor_default(aten_unsqueeze_copy_default_1, 0.012759864330291748, 55, -128, 127, torch.int8); aten_unsqueeze_copy_default_1 = None which is right since s2 was replaced with 3, but the change was not propagated to all internal nodes. Differential Revision: D65120377
laithsakka
force-pushed
the
export-D65120377
branch
from
b0c1e51 to
4d2d511
Compare
Contributor
|
This pull request was exported from Phabricator. Differential Revision: D65120377 |
larryliu0820
approved these changes
Oct 30, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
Summary:
re-generating the inputs (the fake tensors from real tensors) can change some dimension from static to dynamic, this does not sounds to me to be part of the intention of those custom passes .
But this can have complication, for details of the issue read below, what i suggest here is just read the meta['val'] from the input graph instead of regenerating a new fake tensor.
a failure here https://www.internalfb.com/diff/D65023233 that is related to executorch.
one of the passes changes the dimension of the one of the inputs from [2, 2, 3, 1] to f32[s0, 2, 7 - s2] (because calling from fake_tensor_mode.from_real_tensor can do that)
after running passes execuotorch revert the changes of the meta['val] values for all inputs to match the old graph values before the pass did run. (https://www.internalfb.com/code/fbsource/[92156447ae2c]/xplat/executorch/exir/pass_base.py?lines=688)
But executorch only do that for inputs and not for the internal nodes of the graph.
in this case that one of the inputs from:
def forward(self, p_conv1_bias: "f32[2]", p_conv2_bias: "f32[2]", `b__frozen_param0: "i8[2, 2, s2, 1]", b__frozen_param1: "i8[2, 2, s2, 1]", x: "f32[s0, 2, 4]"):to
def forward(self, p_conv1_bias: "f32[2]", p_conv2_bias: "f32[2]", b__frozen_param0: "i8[2, 2, 3, 1]", b__frozen_param1: "i8[2, 2, 3, 1]", x: "f32[s0, 2, 4]"):replacing every s2 with 3 in the place holders only but executorch do not apply the changes back to internal nodes we we still have i8[s0, 2, 7 - s2, 1] in the graph (s2 coming from no where now!)
5) executorch runs more passes on this (ruined graph)
6) we get an error in one internal node operation that checks that the a dimension have only one dynamic shape.
quantized_decomposed_quantize_per_tensor_default_4: "i8[s0, 2, 7 - s2, 1]" = executorch_exir_dialects_edge__ops_quantized_decomposed_quantize_per_tensor_default(aten_unsqueeze_copy_default_1, 0.012759864330291748, 55, -128, 127, torch.int8); aten_unsqueeze_copy_default_1 = None
which is right since s2 was replaced with 3, but the change was not propagated to all internal nodes.
Differential Revision: D65120377