-
Notifications
You must be signed in to change notification settings - Fork 536
Support multi-module exports in Inspector and ETRecord #8336
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
base: main
Are you sure you want to change the base?
Support multi-module exports in Inspector and ETRecord #8336
Conversation
- Added optional module_name and method_name parameters to Inspector initialization - Updated _consume_etrecord method to handle multi-module export scenarios - Modified gen_graphs_from_etrecord to support custom graph keys (for each module/method) - Updated inspector_cli to accept module and method name arguments - Improved error handling for multi-module export use cases
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/8336
Note: Links to docs will display an error until the docs builds have been completed. ❌ 5 New Failures, 2 Cancelled JobsAs of commit becb19a with merge base d99970b ( NEW FAILURES - The following jobs have failed:
CANCELLED JOBS - The following jobs were cancelled. Please retry:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
@JacobSzwejbka, @tarun292, @Gasoonjia can you take a look please? |
raise RuntimeError( | ||
f"Unsupported type of edge_dialect_program passed in {type(edge_dialect_program)}." | ||
) | ||
if export_modules is None: |
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'm not sure i fully understand why you needed to add this check here?
"--module_name", | ||
required=False, | ||
default=None, | ||
help="Module Name to inspect (used with multi-module exports)", |
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.
What is module name intended to be here?
@cptspacemanspiff any update on this? I'm seeing a couple open questions to the code still |
So, this is a prototype of changes required to get performance data out of a multi-method export with ETRecords and ETDump.
I do not have a good understanding of if this is correct/the best way, and my understanding of the performance analysis stuff is limited.
What this does:
FWIW I was able to get the statistics table with node labeling with these changes.
From the sounds of this this might be used internally and there is probably a better way to handle this?
related to #8030