-
Couldn't load subscription status.
- Fork 700
[et] generate debug handle before opeartor decomposition #11997
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[et] generate debug handle before opeartor decomposition #11997
Conversation
This diff update the debug handle generation, from each node in the edge program having a individual debug handle, to all nodes having a same ancestor in export graph sharing a same debug handle, which update the start point of tracing our node transformation from edge graph to exported graph. Differential Revision: [D76860368](https://our.internmc.facebook.com/intern/diff/D76860368/) [ghstack-poisoned]
This diff update the debug handle generation, from each node in the edge program having a individual debug handle, to all nodes having a same ancestor in export graph sharing a same debug handle, which update the start point of tracing our node transformation from edge graph to exported graph. Differential Revision: [D76860368](https://our.internmc.facebook.com/intern/diff/D76860368/) ghstack-source-id: 292801936 Pull Request resolved: #11997
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/11997
Note: Links to docs will display an error until the docs builds have been completed. ✅ You can merge normally! (2 Unrelated Failures)As of commit dba90f5 with merge base b342f83 ( BROKEN TRUNK - The following jobs failed but were present on the merge base:👉 Rebase onto the `viable/strict` branch to avoid these failures
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
|
This pull request was exported from Phabricator. Differential Revision: D76860368 |
This diff update the debug handle generation, from each node in the edge program having a individual debug handle, to all nodes having a same ancestor in export graph sharing a same debug handle, which update the start point of tracing our node transformation from edge graph to exported graph. Differential Revision: [D76860368](https://our.internmc.facebook.com/intern/diff/D76860368/) [ghstack-poisoned]
…ed graph instead of edge program Pull Request resolved: #11997 This diff update the debug handle generation, from each node in the edge program having a individual debug handle, to all nodes having a same ancestor in export graph sharing a same debug handle, which update the start point of tracing our node transformation from edge graph to exported graph generated from nn.Module, by using `from_node` as source. N6213836 is an example of how `from_node` infra works. Before this diff: {F1979833525} After this diff: {F1979833595} and if quantized: {F1979833708} ghstack-source-id: 293136004 @exported-using-ghexport Differential Revision: [D76860368](https://our.internmc.facebook.com/intern/diff/D76860368/)
|
This pull request was exported from Phabricator. Differential Revision: D76860368 |
This diff update the debug handle generation, from each node in the edge program having a individual debug handle, to all nodes having a same ancestor in export graph sharing a same debug handle, which update the start point of tracing our node transformation from edge graph to exported graph. Differential Revision: [D76860368](https://our.internmc.facebook.com/intern/diff/D76860368/) [ghstack-poisoned]
…ed graph instead of edge program Pull Request resolved: #11997 This diff update the debug handle generation, from each node in the edge program having a individual debug handle, to all nodes having a same ancestor in export graph sharing a same debug handle, which update the start point of tracing our node transformation from edge graph to exported graph generated from nn.Module, by using `from_node` as source. N6213836 is an example of how `from_node` infra works. After this diff every node sharing the same ancestor node in export graph will have same debug handle. Before this diff: {F1979833525} After this diff: {F1979833595} and if quantized: {F1979833708} ghstack-source-id: 293582394 @exported-using-ghexport Differential Revision: [D76860368](https://our.internmc.facebook.com/intern/diff/D76860368/)
|
This pull request was exported from Phabricator. Differential Revision: D76860368 |
This diff update the debug handle generation, from each node in the edge program having a individual debug handle, to all nodes having a same ancestor in export graph sharing a same debug handle, which update the start point of tracing our node transformation from edge graph to exported graph. Differential Revision: [D76860368](https://our.internmc.facebook.com/intern/diff/D76860368/) [ghstack-poisoned]
…ed graph instead of edge program Pull Request resolved: #11997 This diff update the debug handle generation, from each node in the edge program having a individual debug handle, to all nodes having a same ancestor in export graph sharing a same debug handle, which update the start point of tracing our node transformation from edge graph to exported graph generated from nn.Module, by using `from_node` as source. N6213836 is an example of how `from_node` infra works. After this diff every node sharing the same ancestor node in export graph will have same debug handle. Before this diff: {F1979833525} After this diff: {F1979833595} and if quantized: {F1979833708} ghstack-source-id: 293597605 @exported-using-ghexport Differential Revision: [D76860368](https://our.internmc.facebook.com/intern/diff/D76860368/)
|
This pull request was exported from Phabricator. Differential Revision: D76860368 |
| """ | ||
|
|
||
| node_source = node.meta[FROM_NODE_KEY] | ||
| node_source = node_source[-1] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is only populated for unlifted graph when .module is called? if so how is it guranteed that this is present
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no it is not only populate that .module is explicitly called. In our case it is guarantee generated after run_decompostition so we will have that when generating debug handle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is guarantee generated after run_decompostition so we will have that when generating debug handle.
There in my question around to_edge_transform_and_lower. although that specifically is unrelated to this pr
| debug_handle = ( | ||
| len(source_node_id_to_debug_handle) + 1 | ||
| if greatest_ancestor_node_id not in source_node_id_to_debug_handle | ||
| else source_node_id_to_debug_handle[greatest_ancestor_node_id] | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok one of my concern is this: is this gonna impact any existing debug utils that rely on debug handle? earlier each node had its own debug handle, but not we are giving a bunch of nodes that might belong to the same original node (e.g. full aten graph nodes?).
Other question is how does this work for delegates that use to_edge_tranform_and_lower, if this pass is run only after to_edge?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For first question, yes like devtool and some other infra or utils are depending on the debgu handle and in the following diffs i will do the migration. There're some testcases rely on debug handle for check and I've talked with the owners and they are ok with the update. Now ci is all clean.
For second question, not sure if I fully understand your concern, but our current debug handle generation pass also locates at the end of to_edge. Our update does not change the location. But we do have concern about whether or not the change of debug handle generation may impact the correctness of the debug handels in the downstream e.g. in delegate, though ci is all clean. And we will pay attention to it when working on the numeric descrepancy dtector on delegated model.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually we should discuss this second paragraph further. THe issue is that to_edge_tranform_and_lower happens before we run decomposition, is that right? If so you dont have debug handles generated no? If no debug handles then no debug information. I would like to be either wrong about my assumption or we need to fix it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the first paragraph, do you forsee any disruption in existing user flows for debugging? That would be one thing that would make me block this PR, so having an answer on that would be nice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue is that to_edge_tranform_and_lower happens before we run decomposition, is that right?
No operator decomposition should be happened inside to_edge_transform_and_lower. to_edge stage happened in side to_edge_transform_and_lower and operator decompostition happened inside to_edge stage. https://github.com/pytorch/executorch/blob/main/exir/program/_program.py#L1129
forsee any disruption in existing user flows for debugging
no. One heart break context is no debug tool is actually using debug handle right now. The numerical discrepancy detector will be the first one.
| # One debug handle should only be associated with one node. We are in the middle of migrating debug handle generation | ||
| # from graph after to_edge to graph after torch.export, one every debug handle in exported graph may be associated with multiple nodes in to_edge |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put before and after state
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i've updated the comment to reflect the function behavior before, during and after the migration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the most part looks good. I left a couple of comments, one specifically around relation of to_edge_lower_and_transform to debug handle (not related ot this diff)
This diff update the debug handle generation, from each node in the edge program having a individual debug handle, to all nodes having a same ancestor in export graph sharing a same debug handle, which update the start point of tracing our node transformation from edge graph to exported graph. Differential Revision: [D76860368](https://our.internmc.facebook.com/intern/diff/D76860368/) [ghstack-poisoned]
…ed graph instead of edge program Pull Request resolved: #11997 This diff update the debug handle generation, from each node in the edge program having a individual debug handle, to all nodes having a same ancestor in export graph sharing a same debug handle, which update the start point of tracing our node transformation from edge graph to exported graph generated from nn.Module, by using `from_node` as source. N6213836 is an example of how `from_node` infra works. After this diff every node sharing the same ancestor node in export graph will have same debug handle. Before this diff: {F1979833525} After this diff: {F1979833595} and if quantized: {F1979833708} ghstack-source-id: 293914705 @exported-using-ghexport Differential Revision: [D76860368](https://our.internmc.facebook.com/intern/diff/D76860368/)
|
This pull request was exported from Phabricator. Differential Revision: D76860368 |
This diff update the debug handle generation, from each node in the edge program having a individual debug handle, to all nodes having a same ancestor in export graph sharing a same debug handle, which update the start point of tracing our node transformation from edge graph to exported graph. Differential Revision: [D76860368](https://our.internmc.facebook.com/intern/diff/D76860368/) [ghstack-poisoned]
…ed graph instead of edge program Pull Request resolved: #11997 This diff update the debug handle generation, from each node in the edge program having a individual debug handle, to all nodes having a same ancestor in export graph sharing a same debug handle, which update the start point of tracing our node transformation from edge graph to exported graph generated from nn.Module, by using `from_node` as source. N6213836 is an example of how `from_node` infra works. After this diff every node sharing the same ancestor node in export graph will have same debug handle. Before this diff: {F1979833525} After this diff: {F1979833595} and if quantized: {F1979833708} ghstack-source-id: 294639957 @exported-using-ghexport Differential Revision: [D76860368](https://our.internmc.facebook.com/intern/diff/D76860368/)
|
This pull request was exported from Phabricator. Differential Revision: D76860368 |
This diff update the debug handle generation, from each node in the edge program having a individual debug handle, to all nodes having a same ancestor in export graph sharing a same debug handle, which update the start point of tracing our node transformation from edge graph to exported graph. Differential Revision: [D76860368](https://our.internmc.facebook.com/intern/diff/D76860368/) [ghstack-poisoned]
…ed graph instead of edge program Pull Request resolved: #11997 This diff update the debug handle generation, from each node in the edge program having a individual debug handle, to all nodes having a same ancestor in export graph sharing a same debug handle, which update the start point of tracing our node transformation from edge graph to exported graph generated from nn.Module, by using `from_node` as source. N6213836 is an example of how `from_node` infra works. After this diff every node sharing the same ancestor node in export graph will have same debug handle. Before this diff: {F1979833525} After this diff: {F1979833595} and if quantized: {F1979833708} Beyond that, placeholder and output node will no longer have debug handle. ghstack-source-id: 294751869 @exported-using-ghexport Differential Revision: [D76860368](https://our.internmc.facebook.com/intern/diff/D76860368/)
This diff update the debug handle generation, from each node in the edge program having a individual debug handle, to all nodes having a same ancestor in export graph sharing a same debug handle, which update the start point of tracing our node transformation from edge graph to exported graph. Differential Revision: [D76860368](https://our.internmc.facebook.com/intern/diff/D76860368/) [ghstack-poisoned]
…ed graph instead of edge program Pull Request resolved: #11997 This diff update the debug handle generation, from each node in the edge program having a individual debug handle, to all nodes having a same ancestor in export graph sharing a same debug handle, which update the start point of tracing our node transformation from edge graph to exported graph generated from nn.Module, by using `from_node` as source. N6213836 is an example of how `from_node` infra works. After this diff every node sharing the same ancestor node in export graph will have same debug handle. Before this diff: {F1979833525} After this diff: {F1979833595} and if quantized: {F1979833708} Beyond that, placeholder and output node will no longer have debug handle. ghstack-source-id: 294784602 @exported-using-ghexport Differential Revision: [D76860368](https://our.internmc.facebook.com/intern/diff/D76860368/)
|
This pull request was exported from Phabricator. Differential Revision: D76860368 |
9216209
into
gh/gasoonjia/17/base
This PR was created by the merge bot to help merge the original PR into the main branch. ghstack PR number: #11997 by @Gasoonjia ^ Please use this as the source of truth for the PR details, comments, and reviews ghstack PR base: https://github.com/pytorch/executorch/tree/gh/gasoonjia/17/base ghstack PR head: https://github.com/pytorch/executorch/tree/gh/gasoonjia/17/head Merge bot PR base: https://github.com/pytorch/executorch/tree/main Merge bot PR head: https://github.com/pytorch/executorch/tree/gh/gasoonjia/17/orig @diff-train-skip-merge Co-authored-by: gasoonjia <[email protected]>
This PR was created by the merge bot to help merge the original PR into the main branch. ghstack PR number: pytorch#11997 by @Gasoonjia ^ Please use this as the source of truth for the PR details, comments, and reviews ghstack PR base: https://github.com/pytorch/executorch/tree/gh/gasoonjia/17/base ghstack PR head: https://github.com/pytorch/executorch/tree/gh/gasoonjia/17/head Merge bot PR base: https://github.com/pytorch/executorch/tree/main Merge bot PR head: https://github.com/pytorch/executorch/tree/gh/gasoonjia/17/orig @diff-train-skip-merge Co-authored-by: gasoonjia <[email protected]>
Stack from ghstack (oldest at bottom):
This diff update the debug handle generation, from each node in the edge program having a individual debug handle, to all nodes having a same ancestor in export graph sharing a same debug handle, which update the start point of tracing our node transformation from edge graph to exported graph.
Differential Revision: D76860368