Validate 4D input tensor memory layout against method metadata. - #22048
Validate 4D input tensor memory layout against method metadata.#22048ArchitAnant wants to merge 1 commit into
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/22048
Note: Links to docs will display an error until the docs builds have been completed.
|
|
|
meta-cla
Bot
added
the
CLA Signed
This PR needs a
|
There was a problem hiding this comment.
Pull request overview
Adds runtime-side validation in the Python bindings (portable mode) to ensure 4D tensor memory layout passed at execution matches the layout encoded in the exported method metadata, preventing silent numerical corruption when contiguous vs channels-last are mismatched.
Changes:
- Fetches input tensor
dim_orderfrom method metadata and validates incoming tensors against the expected contiguous vs channels-last layout. - Updates tensor conversion logic to reject layout mismatches with a runtime error.
Suppressed comments (2)
extension/pybindings/pybindings.cpp:846
- The error message "Input ... expects an unsupported memory format" is confusing/grammatically incorrect (the method metadata is what expects a layout). Consider rephrasing to clarify that the expected layout encoded in metadata is unsupported, and (optionally) mention what layouts are supported.
throw std::runtime_error(
"Input " + std::to_string(i) + " for method " + method_name +
" expects an unsupported memory format.");
extension/pybindings/pybindings.cpp:1251
- Same message clarity issue here: "Input ... expects an unsupported memory format" reads as if the input expects something. It would be clearer to say the method metadata encodes an unsupported expected layout (and mention supported layouts).
throw std::runtime_error(
"Input " + std::to_string(i) + " for method " + method_name +
" expects an unsupported memory format.");
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| dim != 4) { | ||
| throw std::runtime_error( | ||
| "Input " + std::to_string(i) + " for method " + method_name + | ||
| " expected channels-last memory layout but recvied a different layout."); |
Current runtime accepts either contiguous or channel_last tensor unconditionally. This produces silent numerical corruption if a method was exported assuming contiguous strides and a channels-last tensor is passed at runtime (and vice versa). This patch adds a 4D tensor layout validation check against the method metadata at runtime. If the input tensor layout does not match the expected layout, an error is raised. Fixes pytorch#21837 Signed-off-by: Archit Anant <architanant5@gmail.com>
408fe80 to
edc74e6
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.
Suppressed comments (3)
extension/pybindings/pybindings.cpp:801
- module_->method_meta(method_name) is invariant across inputs but is recomputed for every tensor input. Consider fetching MethodMeta once per run_method call (before the inputs loop) and reusing it to avoid repeated metadata lookups.
auto method_meta_res = module_->method_meta(method_name);
if (!method_meta_res.ok()) {
throw std::runtime_error(
"Failed to get metadata for method " + method_name);
}
extension/pybindings/pybindings.cpp:1251
- Same ambiguity as in PyModule::run_method: this error reads like the input is unsupported, but it’s actually the exported method’s expected dim_order that the Python bindings don’t handle. Clarify that only contiguous and channels-last layouts are supported by these bindings.
throw std::runtime_error(
"Input " + std::to_string(i) + " for method " + method_name +
" expects an unsupported memory format.");
extension/pybindings/pybindings.cpp:1235
- This change alters runtime behavior by rejecting mismatched contiguous vs channels-last inputs. Please add a regression unit test (e.g., in runtime/test/test_runtime.py) that exports once with contiguous and once with channels-last 4D inputs and asserts the opposite layout raises with the new error message (covering the issue #21837 repro path via Runtime.load_program(...).load_method(...).execute(...)).
if (expected_contiguous) {
if (!at_tensor.is_contiguous()) {
throw std::runtime_error(
"Input " + std::to_string(i) + " for method " + method_name +
" expected contiguous memory layout but received a different layout.");
| auto method_meta_res = module_->method_meta(method_name); | ||
| if (!method_meta_res.ok()) { | ||
| throw std::runtime_error( | ||
| "Failed to get metadata for method " + method_name); | ||
| } | ||
| auto tensor_meta_res = method_meta_res.get().input_tensor_meta(i); | ||
| if (!tensor_meta_res.ok()) { | ||
| throw std::runtime_error( | ||
| "Failed to get tensor metadata for input " + std::to_string(i)); | ||
| } |
| auto tensor_meta_res = method_->method_meta().input_tensor_meta(i); | ||
| if (!tensor_meta_res.ok()) { | ||
| throw std::runtime_error( | ||
| "Failed to get tensor metadata for input " + std::to_string(i)); | ||
| } |
| throw std::runtime_error( | ||
| "Input " + std::to_string(i) + " for method " + method_name + | ||
| " expects an unsupported memory format."); |
|
Thanks for picking this up, @ArchitAnant — I said I'd run the repro against the fix, so here Build: Your repro, both armsWrong numbers become an error, in both directions. Confirmed. Wider battery — both binding pathsThe patch changes two call sites, and the test plan above exercises one, so I ran everything
Both APIs agree on every row. Nothing that used to work stops working, the 2-D transposed One case worth knowing about (not a regression)A method exported from a C=1 channels-last tensor records With C=1 the two layouts tie on dims 1 and 3, and the stride sort breaks the tie the other Cost per callThe check does a
Roughly 0.05 us per extra input, which is inside the run-to-run spread and invisible next to One questionThe new check sits inside the Everything above is on portable kernels with |
|
@john-rocky Thank you so much for running this through such a rigorous battery of tests!
I would assume that as well, Just one question, in your initial test log:
From your tests I see the patch works fine. So, I am assuming it's just a typo? Just curious. |
Summary
Current runtime accepts either contiguous or channel_last tensor unconditionally. This produces silent numerical corruption if a method was exported assuming contiguous strides and a channels-last tensor is passed at runtime (and vice versa).
This patch adds a 4D tensor layout validation check against the method metadata at runtime. If the input tensor layout does not match the expected layout, an error is raised.
Fixes: #21837
cc @john-rocky
Test plan
Manually tested for both export configurations:
output: