[ez][ET-VK] Small fix for choose_qparams_affine_impl #16186
Conversation
It seems that `choose_qparams_affine` has recently appended some arguments to the schema. This causes newly exported models to break because at runtime, the output arg can no longer be found. Fix by locating the output argument as the last entry in the args vector, rather than continuously incrementing the args index. Update quantize/dequantize ops as well since it seems quantized_decomposed namespace ops are subject to change in the future. Note that it would be good to do this for all operators in the Vulkan backend as a later refactor. Differential Revision: [D88887463](https://our.internmc.facebook.com/intern/diff/D88887463/) [ghstack-poisoned]
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/16186
Note: Links to docs will display an error until the docs builds have been completed. ❌ 1 New Failure, 1 Unrelated FailureAs of commit c9abd59 with merge base a0a6278 ( NEW FAILURE - The following job has failed:
UNSTABLE - The following job is marked as unstable, possibly due to flakiness on trunk:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
meta-cla
bot
added
the
CLA Signed
This PR needs a
|
It seems that `choose_qparams_affine` has recently appended some arguments to the schema. This causes newly exported models to break because at runtime, the output arg can no longer be found. Fix by locating the output argument as the last entry in the args vector, rather than continuously incrementing the args index. Update quantize/dequantize ops as well since it seems quantized_decomposed namespace ops are subject to change in the future. Note that it would be good to do this for all operators in the Vulkan backend as a later refactor. Differential Revision: [D88887463](https://our.internmc.facebook.com/intern/diff/D88887463/) [ghstack-poisoned]
| const std::vector<ValueRef>& args) { | ||
| int32_t arg_idx = 0; | ||
| size_t arg_idx = 0; | ||
| size_t last_arg_idx = args.size() - 1; |
There was a problem hiding this comment.
For the ones in between do you just rely on default behavior? What if they are serialized with values != default? Shouldnt you error out?
There was a problem hiding this comment.
yeah, that's a good point. I have a planned updated to improve arg checking for quantized_decomposed ops since there are currently a lot of unsupported input cases which are not accounted for - I will include this in that update.
The primary purpose of this PR as-is is to recover a currently broken CI signal, so I would prefer to keep it as simple as possible. In practice, not validating the args should be ok (for now) since the quantized_decomposed ops are inserted by a quantization workflow and Vulkan doesn't really work with non-supported quant workflows anyways 😛
Stack from [ghstack](https://github.com/ezyang/ghstack) (oldest at bottom): * __->__ #16187 * #16186 TSIA! Differential Revision: [D88802100](https://our.internmc.facebook.com/intern/diff/D88802100/) cc @manuelcandales @digantdesai @cbilgin --------- Co-authored-by: ssjia <ssjia@devvm1479.ncg0.facebook.com>
Stack from ghstack (oldest at bottom):
It seems that
choose_qparams_affinehas recently appended some arguments to the schema. This causes newly exported models to break because at runtime, the output arg can no longer be found.Fix by locating the output argument as the last entry in the args vector, rather than continuously incrementing the args index.
Update quantize/dequantize ops as well since it seems quantized_decomposed namespace ops are subject to change in the future.
Note that it would be good to do this for all operators in the Vulkan backend as a later refactor.
Differential Revision: D88887463