Arm backend: Preserve output order #13454
Conversation
This change allows us to preserve output order after export. Change-Id: I7ee55c2877ca1b247f10d2e90da3ba38dc727b6f Signed-off-by: Elena Zhelezina <[email protected]>
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/13454
Note: Links to docs will display an error until the docs builds have been completed. ⏳ No Failures, 130 PendingAs of commit 71d2b8d with merge base 41730fa ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
meta-cla
bot
added
the
CLA Signed
wwwind
added
partner: arm
|
The dl3 model tests seem to have got problems, I rerun test to see if it's random or a 100% thing. |
|
Unfortunately not :( I wonder if we assumed some order or where just lucky before. |
|
Hi @zingo I will run this test locally and check the stability for it. Thank you! |
| spec = TosaSpecification.create_from_string("TOSA-1.0+INT") | ||
| compile_spec = ArmCompileSpecBuilder().tosa_compile_spec(tosa_spec=spec).build() | ||
| # Setup quantizer | ||
| quantizer = TOSAQuantizer(compile_spec) | ||
| quantizer.set_global( | ||
| get_symmetric_quantization_config(is_qat=True, is_per_channel=False) | ||
| ) | ||
| # Trace the model | ||
| dummy = torch.randn(batch_size, 1, 28, 28) | ||
| fx_mod = torch.export.export_for_training(model, (dummy,)).module() | ||
| model = prepare_pt2e(fx_mod, quantizer) | ||
| model(dummy) | ||
| model = convert_pt2e(model) |
There was a problem hiding this comment.
can we use FP profile and avoid quantization in this test? Just to simplify
There was a problem hiding this comment.
the order of inputs has happened only for INT profile, it is not repro in FP.
this test fails without this fix
There was a problem hiding this comment.
Is it because we don't run the same test in FP? I am failing to see a output order connection with the TOSA profiling? Is there a pass we run only in INT profile which shuffles the order or something?
There was a problem hiding this comment.
it just happens here that for FP profile the order is what we want.
this test for INT does not fail in the debugger, for example, and that is why it was impossible for me to find out where exactly it fails during partioning but it fails when we run as a pytest.
the order of outputs is not deterministic. this change makes sure that we re-order according to the initial order.
the reason of these flakiness can be in usage of set, etc inside of Python code.
we need this fix for our project and this is a clean and working solution to make sure that order is as original
There was a problem hiding this comment.
it just happens here that for FP profile the order is what we want.
this test for INT does not fail in the debugger, for example, and that is why it was impossible for me to find out where exactly it fails during partioning but it fails when we run as a pytest.
Weird. Tracking the output order after each pass might lead to something. You can add a print in the base class for ExportPass or something.
the order of outputs is not deterministic.
This is surprising TBH. export() does have this guarantee (if flattened and back then perhaps with preserve_module_call_signature arg). Also, ExportGraphSignature also same, but it adds more stuff if you do buffer modifications.
Inputs = [*parameters_buffers_constant_tensors, *flattened_user_inputs]
Outputs = [*mutated_inputs, *flattened_user_outputs]
See - https://docs.pytorch.org/docs/stable/export.html#torch.export.graph_signature.ExportGraphSignature
we need this fix for our project and this is a clean and working solution to make sure that order is as original
I get this and am also OK with landing this as a TOSA level solution.
That said, I would like to understand the root cause a bit better and see what's the right place to fix this.
There was a problem hiding this comment.
Ok, I create a ticket for us to investigate further. I close this PR for now then
digantdesai
left a comment
digantdesai
left a comment
There was a problem hiding this comment.
@wwwind - can you create an issue to further investigate the root case. Main concern is if this is needed by all the backends etc. But stamping to unblock you. I didn't mean to block you in the first place. Thanks.
| exported_program=edge_program | ||
| ) | ||
|
|
||
| # Re-shuffle output nodes to preserve author's order |
There was a problem hiding this comment.
so IIUC the order was correct before we ran passes (i.e. for the incoming edge_program) but then got switched up? If yes, did we find if some pass(es) are injecting things out of order in output_node.arg[0]?
There was a problem hiding this comment.
yes, the order was correct here. after we run passes, it changed.
because in the debugger this test was working, I was not able to get to the true reason.
|
@wwwind this unfortunally seem to need a rebase before merge, hopefully that fixes most of the the CI fails also. |
Change-Id: I61cbbb515fd1bfd2f92cf6ac0c110452ca9f624b
|
i resolved conflicts locally and dont understand what is going on here with Resolve conflicts being grayed out |
|
Replaced with #13997 due to this grayed out Resolve conflicts button |
3 participants
This change allows us to preserve output order after export.
Change-Id: I7ee55c2877ca1b247f10d2e90da3ba38dc727b6f
Signed-off-by: Elena Zhelezina [email protected]
cc @digantdesai @freddan80 @per @zingo @oscarandersson8218