Fix inductor <> ddp_optimizer issue #108081
Conversation
[ghstack-poisoned]
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/108081
Note: Links to docs will display an error until the docs builds have been completed. ❗ 1 Merge Blocking SEVsThere is 1 active merge blocking SEVs. Please view them below:
If you must merge, use ✅ No FailuresAs of commit c33fed0 with merge base a16b0aa ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
ghstack-source-id: 3404582932e2237cabc5c88c46eb896a11c81a0e Pull Request resolved: #108081
github-actions
bot
requested review from
ezyang,
miladm,
SherlockNoMad,
voznesenskym and
wconstab
|
Sounds good - let me try to make a simpler tests (should be doable just by using inductor + inference graph + input mutations + layout optimization) |
|
Oh I'll also add an assertion that we don't go beyond the number of graph outputs (which would also have caught this issue instead of silently not preserving strides for certain outputs) |
|
Hmm I tried creating a repro by taking the example from the linked issue, removing the DDP code, and manually inserting a graph break at the same place that the ddp optimizer would. I confirmed that between the original repro and my modified version the AOTAutograd graphs that we send to inductor are identical, but for some reason I can't trigger the issue. I'll try inspecting the generating inductor code and see if it's any different |
|
hmm @shunting314 - will pointed out to me that the issue you linked only affects code that's run under |
|
@bdhirsh so my point is that for the manually injected graph breaks, downstream graph already get the output of OPTIMIZED upstream graph, whether we succeed to decide the correct output tensors to keep their strides does not matter. Even if we the compiler change the output tensor's stride, the downstream graph will get tensors with those updated stride. Just want to explain why you could not repro earlier and I think we have to use DDPOptimizer to repro the issue. |
|
i'm not sure how brian did the repro, but i would expect you could repro without DDPOptimizer and without graph-breaks, just by
Why is DDPoptimizer needed for the repro? I do understand that if graph-breaks are introduced by dynamo, dynamo works around compilation, ensuring it will match expectations. |
cc shunting314 this is a tentative fix for #107362. More discussion at #107362 (comment). wconstab pointed out that inductor found a graph with 6 input mutations and only 1 output, and seemed to be (incorrectly) chopping off the first "6" outputs from the graph (even though there is only 1). It looks like this is because: (1) AOTAutograd has special handling for input mutations in inference vs. training graphs. In a training graph, whenever AOTAutograd sees an input mutation, it will add an **extra** output to the graph, corresponding to the updated input (and then at runtime, it will grab the updated input, and perform the actual mutation outside of the graph). In inference, AOTAutograd is smarter and can leave the input mutations directly in the graph for inductor to optimize (doing this in training is harder). In inference, AOTAutograd will **not** add any extra graph outputs for input mutations. It looks like inductor was unconditionally assuming that input mutations counted as extra outputs in the graph, which is wrong for the inference case. cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 ipiszy ngimel yf225 chenyang78 kadeng muchulee8 aakhundov [ghstack-poisoned]
test/dynamo/test_repros.py
Outdated
| @@ -3270,6 +3270,29 @@ def f(): | |||
|
|
|||
| self.assertEqual(f(), _GLOBAL_CPU_TENSOR + _GLOBAL_CPU_TENSOR) | |||
|
|
|||
| @torch._inductor.config.patch("layout_optimization", True) | |||
There was a problem hiding this comment.
yea, we may disable layout optimization on the fly if certain patterns are found.
cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 ipiszy ngimel yf225 chenyang78 kadeng muchulee8 aakhundov anijain2305 shunting314 this is a tentative fix for #107362. More discussion at #107362 (comment). wconstab pointed out that inductor found a graph with 6 input mutations and only 1 output, and seemed to be (incorrectly) chopping off the first "6" outputs from the graph (even though there is only 1). It looks like this is because: (1) AOTAutograd has special handling for input mutations in inference vs. training graphs. In a training graph, whenever AOTAutograd sees an input mutation, it will add an **extra** output to the graph, corresponding to the updated input (and then at runtime, it will grab the updated input, and perform the actual mutation outside of the graph). In inference, AOTAutograd is smarter and can leave the input mutations directly in the graph for inductor to optimize (doing this in training is harder). In inference, AOTAutograd will **not** add any extra graph outputs for input mutations. It looks like inductor was unconditionally assuming that input mutations counted as extra outputs in the graph, which is wrong for the inference case. cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 ipiszy ngimel yf225 chenyang78 kadeng muchulee8 aakhundov [ghstack-poisoned]
cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 ipiszy ngimel yf225 chenyang78 kadeng muchulee8 aakhundov anijain2305 shunting314 this is a tentative fix for #107362. More discussion at #107362 (comment). wconstab pointed out that inductor found a graph with 6 input mutations and only 1 output, and seemed to be (incorrectly) chopping off the first "6" outputs from the graph (even though there is only 1). It looks like this is because: (1) AOTAutograd has special handling for input mutations in inference vs. training graphs. In a training graph, whenever AOTAutograd sees an input mutation, it will add an **extra** output to the graph, corresponding to the updated input (and then at runtime, it will grab the updated input, and perform the actual mutation outside of the graph). In inference, AOTAutograd is smarter and can leave the input mutations directly in the graph for inductor to optimize (doing this in training is harder). In inference, AOTAutograd will **not** add any extra graph outputs for input mutations. It looks like inductor was unconditionally assuming that input mutations counted as extra outputs in the graph, which is wrong for the inference case. cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 ipiszy ngimel yf225 chenyang78 kadeng muchulee8 aakhundov [ghstack-poisoned]
…stride=False together (#108235) From talking to @wconstab, we agreed that because of the way DDPOptimizer is written, it is (sort of) incompatible with inductor's `keep_output_stride=False` optimizations (and will cause silent correctness problems if you use them ogether). Added an assertion. Pull Request resolved: #108235 Approved by: https://github.com/wconstab ghstack dependencies: #108081
Fixes #105327. The problem is that `lift_fresh_copy()`'s functionalization implementation currently assumes that the input is always not functional. This is apparently too limiting: when you have "user" code like this (which can potentially come from exporting a model and then running compile on the resulting graph): ``` tensor_constant0 = torch.tensor(2) lift_fresh = torch.ops.aten.lift_fresh_copy.default(tensor_constant0) ``` When we run this through AOTAutograd, the first call (torch.tensor(2)) will **already** be lifted into a functional tensor wrapper - so the `lift_fresh_copy` call doesn't need to do any "lifting" anymore - it just needs to do a clone. Pull Request resolved: #108243 Approved by: https://github.com/albanD ghstack dependencies: #108081, #108235
cc @voznesenskym @penguinwu @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @Xia-Weiwen @wenzhe-nrv @jiayisunx @peterbell10 @ipiszy @ngimel @yf225 @chenyang78 @kadeng @muchulee8 @aakhundov @anijain2305 @shunting314 this is a tentative fix for #107362. More discussion at #107362 (comment).
@wconstab pointed out that inductor found a graph with 6 input mutations and only 1 output, and seemed to be (incorrectly) chopping off the first "6" outputs from the graph (even though there is only 1). It looks like this is because:
(1) AOTAutograd has special handling for input mutations in inference vs. training graphs. In a training graph, whenever AOTAutograd sees an input mutation, it will add an extra output to the graph, corresponding to the updated input (and then at runtime, it will grab the updated input, and perform the actual mutation outside of the graph).
In inference, AOTAutograd is smarter and can leave the input mutations directly in the graph for inductor to optimize (doing this in training is harder). In inference, AOTAutograd will not add any extra graph outputs for input mutations.
It looks like inductor was unconditionally assuming that input mutations counted as extra outputs in the graph, which is wrong for the inference case.
Stack from ghstack (oldest at bottom):
cc @voznesenskym @penguinwu @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @Xia-Weiwen @wenzhe-nrv @jiayisunx @peterbell10 @ipiszy @ngimel @yf225 @chenyang78 @kadeng @muchulee8 @aakhundov