-
Notifications
You must be signed in to change notification settings - Fork 538
Fix pre-autograd transforms not getting persisted during xnnpack export #9118
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
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/9118
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit 9978148 with merge base cf8ce89 ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
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.
Lets keep exported_program as the only source of truth
extension/llm/export/builder.py
Outdated
@@ -394,9 +415,12 @@ def export_to_edge(self) -> "LLMEdgeManager": | |||
return_value=False, | |||
) | |||
|
|||
# Prior to export, persist the changes to the pre autograd | |||
# graph module back to the source-of-truth ExportedProgram. | |||
self.export(self.pre_autograd_graph_module) |
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 think we should keep exported_program up-to-date. Thus shouldnt do this here but rather wherever we extract graph_module
and apply any transformations. Thus we should not keep self.pre_autograd_graph_module
at all. Only source of truth would be exported_program
f5506bf
to
9978148
Compare
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.
Looks good. We discussed to follow up to answer " what should be the source of truth, graph_module or EP"
Summary
After moving to
to_edge_transform_and_lower
for the XNNPack export route in #8624, we were discarding all of the transforms made to the pre-autograd graph module stored inLLMEdgeManager
, since the newto_edge_transform_and_lower
took in anExportedProgram
instead of ann.Module
as an argument. To solve this, we re-run export for training right before eachLLMEdgeManager
API that runs the full non-autograd safetorch.export()
.Test plan
Tested manually on Llama3.2 1B export:
Before ops (contains permute_copy):
After ops (no permute_copy):