[dynamo] Rehaul the autograd.Function support#166788
[dynamo] Rehaul the autograd.Function support#166788anijain2305 wants to merge 51 commits intogh/anijain2305/940/basefrom
Conversation
[ghstack-poisoned]
cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 kadeng chauhang amjames Lucaskabela [ghstack-poisoned]
cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 kadeng chauhang amjames Lucaskabela [ghstack-poisoned]
cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 kadeng chauhang amjames Lucaskabela [ghstack-poisoned]
cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 kadeng chauhang amjames Lucaskabela [ghstack-poisoned]
cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 kadeng chauhang amjames Lucaskabela [ghstack-poisoned]
cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 kadeng chauhang amjames Lucaskabela [ghstack-poisoned]
cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 kadeng chauhang amjames Lucaskabela [ghstack-poisoned]
cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 kadeng chauhang amjames Lucaskabela [ghstack-poisoned]
We make a rehaul because (1) we want to support non-proxyable outputs in the fwd method (2) we saw general softness in the support. I have put lot of comments in the code. Follow up * Graph break on backward stride dependent computation. This is BC breaking, so needs care. * Use DynamoAutogradFunctionTraceHelper for backward tracer. * Add test cases for module input, pytree input/outputs, pg groups * Consider unifying `automatic` and `automatic_with_forced_placeholders` * Better error messages - especially nonstrict trace for stride dependent backward computation. Pull Request resolved: pytorch#166788 Approved by: https://github.com/zou3519
…d graph tracing (pytorch#169399)" This reverts commit f874542. Reverted pytorch#169399 on behalf of https://github.com/huydhn due to Sorry for reverting your change but I need to revert this to revert pytorch#166788 ([comment](pytorch#169399 (comment)))
This reverts commit a84798e. Reverted pytorch#166788 on behalf of https://github.com/huydhn due to Sorry for reverting your change but it seems to cause some numerical error in trunk ([comment](pytorch#166788 (comment)))
We make a rehaul because (1) we want to support non-proxyable outputs in the fwd method (2) we saw general softness in the support. I have put lot of comments in the code. Follow up * Graph break on backward stride dependent computation. This is BC breaking, so needs care. * Use DynamoAutogradFunctionTraceHelper for backward tracer. * Add test cases for module input, pytree input/outputs, pg groups * Consider unifying `automatic` and `automatic_with_forced_placeholders` * Better error messages - especially nonstrict trace for stride dependent backward computation. Pull Request resolved: #166788 Approved by: https://github.com/zou3519
…d graph tracing (#169399)" This reverts commit f874542. Reverted #169399 on behalf of https://github.com/huydhn due to Sorry for reverting your change but I need to revert this to revert #166788 ([comment](#169399 (comment)))
This reverts commit a84798e. Reverted #166788 on behalf of https://github.com/huydhn due to Sorry for reverting your change but it seems to cause some numerical error in trunk ([comment](#166788 (comment)))
ghstack-source-id: fa8ce98 Pull Request resolved: pytorch/pytorch#166788
ghstack-source-id: 62b4540 Pull Request resolved: pytorch/pytorch#166788
|
@zou3519 I am debugging why Observations
On
So my recommendation is to skip the test for now for this PR, because this is an existing issue. And handle that as a separate issue, opened here - #170160 |
|
@anijain2305 can you add a test case that defends against the source bug? I've seen this issue actually but wasn't able to repro it. It can just be test_partitioner_recomputes_factory_ones_like_sin_op with a fullgraph=True and cleaned up. I'm curious about what the tlparse looks like for the original PR - like, was the autograd.Function captured in whole? If it was captured in whole, did it run into the problem? Was the source bug introduced later? I'm not completely sure I'd classify this as "a preexisting problem" without that analysis. This PR does increase the memory usage of that autograd.Function because inductor fails to reinplace the custom op in the backward, it trades-off the graph break for additional memory. That being said, it's pretty rare for someone to use ones_like to allocate a tensor for a kernel, so I'm fine if you want to merge this. It's also a good time to yolo merge this (it's after the branch cut) |
|
Starting merge as part of PR stack under #169399 |
1 similar comment
|
Starting merge as part of PR stack under #169399 |
…tracing (#169399) Pull Request resolved: #169399 Approved by: https://github.com/tugsbayasgalan, https://github.com/zou3519 ghstack dependencies: #166788
We make a rehaul because (1) we want to support non-proxyable outputs in the fwd method (2) we saw general softness in the support. I have put lot of comments in the code. Follow up * Graph break on backward stride dependent computation. This is BC breaking, so needs care. * Use DynamoAutogradFunctionTraceHelper for backward tracer. * Add test cases for module input, pytree input/outputs, pg groups * Consider unifying `automatic` and `automatic_with_forced_placeholders` * Better error messages - especially nonstrict trace for stride dependent backward computation. Pull Request resolved: pytorch#166788 Approved by: https://github.com/zou3519
…tracing (pytorch#169399) Pull Request resolved: pytorch#169399 Approved by: https://github.com/tugsbayasgalan, https://github.com/zou3519 ghstack dependencies: pytorch#166788
We make a rehaul because (1) we want to support non-proxyable outputs in the fwd method (2) we saw general softness in the support. I have put lot of comments in the code. Follow up * Graph break on backward stride dependent computation. This is BC breaking, so needs care. * Use DynamoAutogradFunctionTraceHelper for backward tracer. * Add test cases for module input, pytree input/outputs, pg groups * Consider unifying `automatic` and `automatic_with_forced_placeholders` * Better error messages - especially nonstrict trace for stride dependent backward computation. Pull Request resolved: pytorch#166788 Approved by: https://github.com/zou3519
…tracing (pytorch#169399) Pull Request resolved: pytorch#169399 Approved by: https://github.com/tugsbayasgalan, https://github.com/zou3519 ghstack dependencies: pytorch#166788
Stack from ghstack (oldest at bottom):
We make a rehaul because
(1) we want to support non-proxyable outputs in the fwd method
(2) we saw general softness in the support.
I have put lot of comments in the code.
Follow up
automaticandautomatic_with_forced_placeholderscc @voznesenskym @penguinwu @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @wenzhe-nrv @jiayisunx @ipiszy @kadeng @muchulee8 @amjames @chauhang @aakhundov @coconutruben @jataylo @Lucaskabela @chenyang78