[dynamo][invoke_subgraph] Faster aliasing checks #148997
Conversation
[ghstack-poisoned]
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/148997
Note: Links to docs will display an error until the docs builds have been completed. ❌ 2 New Failures, 2 Unrelated FailuresAs of commit d66dde3 with merge base dcc502f ( NEW FAILURES - The following jobs have failed:
FLAKY - The following job failed but was likely due to flakiness present on trunk:
BROKEN TRUNK - The following job failed but was 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. |
anijain2305
added
ciflow/trunk
| unimplemented("NYI: invoke_subgraph with mutated inputs") | ||
|
|
||
| if not supports_aliasing and subtracer.has_aliasing(): | ||
| unimplemented("NYI: invoke_subgraph with aliasing") |
There was a problem hiding this comment.
can I get an unimplemented_v2 lol
There was a problem hiding this comment.
Big YES!! Will move over.
| should_flatten_outputs=False, | ||
| under_activation_checkpoint=False, | ||
| supports_input_mutation=True, | ||
| supports_aliasing=True, |
There was a problem hiding this comment.
Can the default for this be False? Also ideally we would go through all HOPs that use the _has_potential_branch_input_alias check and update them to use this (could you file an issue for that?)
There was a problem hiding this comment.
I am not sure if the default should be False. For example, for activation checkpointing, it might be ok with aliasing? I will open an issue to track for sure.
There was a problem hiding this comment.
activation checkpointing is OK with it. The way I'd pitch this is: "supports_aliasing=False" is a safer default, because otherwise, someone writing a new HOP may run into silent incorrectness. For example, if someone is writing invoke_subgraph2, if they set supports_aliasing=True but do not handle it separately, they will get silen tincorrectness.
cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 kadeng chauhang amjames [ghstack-poisoned]
cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 kadeng chauhang amjames [ghstack-poisoned]
cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 kadeng chauhang amjames [ghstack-poisoned]
cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 kadeng chauhang amjames [ghstack-poisoned]
cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 kadeng chauhang amjames [ghstack-poisoned]
|
Squashed in #148953 for easier merge. Will manually port over Richard's comment to that PR. |
ghstack-source-id: 4d85ff1 Pull Request resolved: pytorch/pytorch#148953 [dynamo][invoke_subgraph] Faster aliasing checks ghstack-source-id: 4d85ff1 Pull Request resolved: pytorch/pytorch#148997
Stack from ghstack (oldest at bottom):
cc @voznesenskym @penguinwu @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @wenzhe-nrv @jiayisunx @chenyang78 @kadeng @chauhang @amjames