-
Notifications
You must be signed in to change notification settings - Fork 751
Add Post to backend stage to recipes #14990
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/14990
Note: Links to docs will display an error until the docs builds have been completed. ❗ 1 Active SEVsThere are 1 currently active SEVs. If your PR is affected, please view them below: ❌ 22 New FailuresAs of commit 090baad with merge base 5cf193a ( NEW FAILURES - The following jobs have failed:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
It is a common pattern that passes require an exported program, which means that the pass needs to be constructed right before it is ran, with the correct exported program. Today there is no unified way this is done, which makes it tricky for general pass handling to figure out whether to pass an exported program to the constructor or not. This commit solves this by introducing a RequireExportProgram mixin, and makes a best effort to migirate passes to this system. Note that there are passes which require other arguments that still will break general pass handling. Signed-off-by: Erik Lundell <[email protected]> Change-Id: Ifaef3069fb04d6ab3df68b1d303aae7dc42585f2
After a discussion in pytorch#14588, it was decided to create an additional recipe stage to run passes after partitioning. This is meant for backends that convert ops directly instead of partitioning. Signed-off-by: Erik Lundell <[email protected]> Change-Id: Iddf74accb739d4dff16fa46c6fad88ffccfe2f3b
b9843c2 to
d72a730
Compare
|
Hi @abhinaykukkadapu and @digantdesai I think you commented on the original issue leading up to this change, is it OK to merge this? |
|
|
||
|
|
||
| class RequireExportedProgram: | ||
| """Mixin to require a pass to take an exported program, which is accessed by the exported_program property. |
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.
@Erik-Lundell @zingo Sorry for the late response and thanks for following up on this change, i've done some "standardization" when passes needs an ExportedProgram. QNN backend has exact issue where some passes wants ExportedProgram, i think we can follow similar pattern here too, here is the code ptr: https://github.com/pytorch/executorch/blob/main/backends/qualcomm/recipes/qnn_recipe_provider.py#L142
Basic idea is that the recipe will get a callable similar to aten_transform_passes and the callable can have a sequence of passes instantiated with or without EP. Let me know if you have any questions.
After you are done, i will probably take a stab at creating a single stage for all the pass related steps and then that stage shall be plugged in at various stages pre/post et stages.
Add PostToBackend stage to Recipes
After a discussion in #14588, it was decided to
create an additional recipe stage to run passes after partitioning.
This is meant for backends that convert ops directly instead of
partitioning.
Add RequireExportProgram mixin for passes
It is a common pattern that passes require an exported program,
which means that the pass needs to be constructed right before
it is ran, with the correct exported program. Today there is no
unified way this is done, which makes it tricky for general pass handling
to figure out whether to pass an exported program to the constructor or not.
This commit solves this by introducing a RequireExportProgram mixin,
and makes a best effort to migirate passes to this system.
cc @freddan80 @per @zingo @oscarandersson8218 @digantdesai