You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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
This fix solves the issue only in the context of the ONNX exporter but this issue happens in other context.
The bug happens when method run_decompositions is called. The failing pattern is assumed to be view(transpose(x, ...)). This pattern is replaced by view(flatten(transpose(x, ..))). By changing the dimensions, the strides are updated as well and run_decompositions does not fail anymore. It would be inefficient on a 1D tensor but then transpose would not be used. The extra node appears in the final onnx graph but is removed after optimization. The final onnx graph should not be impacted and no performance loss should be observed for the onnx model.
Is this intended to be merged in? If we can address the root cause that would be the best imo, since we have had issues with too many FX passes where we don’t know which one to look when an error is introduced. I can run a git bisect today to try to pin down the commit.
If we do decide to merge this as a temporary fix, I recommend moving the logic to _fx_passes, where all fx passes live.
The fix is just one pass on the fx graph. It is done inplace. It should be very fast. I can move the function to _fx_passes.py if that's what you mean.
The fix is just one pass on the fx graph. It is done inplace. It should be very fast. I can move the function to _fx_passes.py if that's what you mean.
I am more concerned about complexity and robustness over speed. But since this should be a temporary fix, it should be useful to unblock us for now. I would just make it very clear that it should be removed when the issue is resolved and should be removed before the 2.6 release
withgraph.inserting_after(node):
new_node=graph.call_function(torch.ops.aten.contiguous.default, args=(node,))
node.replace_all_uses_with(new_node)
# new_node is replaced as well so we manually revert the replacementnew_node.update_arg(0, node)
node.users= {new_node: None}
justinchuby
changed the title
[ONNX] Insert flatten node between transpose and view before calling run_decompositions
[ONNX] Insert contiguous node between transpose and view before calling run_decompositions
Oct 7, 2024
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.
Works around #136543.
This fix solves the issue only in the context of the ONNX exporter but this issue happens in other context.
The bug happens when method
run_decompositions
is called. The failing pattern is assumed to beview(transpose(x, ...))
. This pattern is replaced byview(flatten(transpose(x, ..)))
. By changing the dimensions, the strides are updated as well andrun_decompositions
does not fail anymore. It would be inefficient on a 1D tensor but then transpose would not be used. The extra node appears in the final onnx graph but is removed after optimization. The final onnx graph should not be impacted and no performance loss should be observed for the onnx model.