NXP backend: Improve test result gathering#20024
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/20024
Note: Links to docs will display an error until the docs builds have been completed. ❌ 4 New FailuresAs of commit f3bd95d with merge base 9898bc5 ( NEW FAILURES - The following jobs have failed:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
|
| "tag1_neutron.et.tflite" | ||
| ), "Converted Neutron model not found in working directory, export in NeutronBackend failed." | ||
| shutil.copy("tag1_pure.et.tflite", test_dir) | ||
| shutil.copy("tag1_neutron.et.tflite", test_dir) |
There was a problem hiding this comment.
How does this work when there are more delegated partitions? The file names suggest it only works with 1 such partition.
There was a problem hiding this comment.
I counted with single partition, as multiple partitions means there is a problem. But we also need to take these cases into account. As Robert pointed out the copy-solution unfortunate, so I changed it to pass test_dir to Neutron backend via CompileSpec and store all partitions directly there.
| remove_quant_io_ops=remove_quant_io_ops, | ||
| ) | ||
|
|
||
| # Copy converter Neutron model and Neutron IR model to test_dir |
There was a problem hiding this comment.
This is a quite impractical workaround for the code in executorch/backends/nxp/nxp_backend.py::NeutronBackend::preprocess() which stores the debug files into current working directory.
Now you have:
- logic in
lower_run_compare, which knows where to properly store the test files - derived from test ID fromrequest - logic in
NeutronBackend::preprocessstoring them into current working directory, what e.g. Arm handles with passing the intermediates folder incompile_spec, what is acceptable, see https://github.com/pytorch/executorch/blob/main/backends/arm/scripts/aot_arm_compiler.py#L511 - logic in this function (
_run_delegated_executorch_program) which stores the pytorch/executorch visualized graphs (.json files) into thetest_dirwhat is kind of OK as it is called fromlower_run_comparebut unflexible:
Unflexible because: the folder name for intermediate artefacts is created in lower_run_compare based on request fixture. The lower_run_compare is quite low level function to know about request [ https://en.wikipedia.org/wiki/Principle_of_least_privilege ] . Why not to move the test_dir name creation logic into test itself and pass it into utility functions. Such a change also opens the possibility to store intermediate artefacts also from the tests not calling the lower_run_compare , like assert_not_delegated tests.
And as Martin pointed out, you must rely on specific tag name to avoid mistakenly copying garbage.
There was a problem hiding this comment.
I agree the copy-solution is unfortunate, so I changed it to pass test_dir to Neutron backend via CompileSpec and store all partitions directly there. Now the storing logic is in one place. I made the test_dir param inside our pipeline optional, so the default store dir is still cwd.
I also wouldn't move the test_name extraction outside of lower_run_compare as I consider it as the one compact API for NSYS test. Other type of tests typically don't generate such test results and the test_name can still be be extracted from request in those tests.
ceed080 to
9b79072
Compare
|
Provided additional information to some of my comments. Otherwise LGTM |
9b79072 to
f3bd95d
Compare
Summary
Only last 2 commits are part of this PR. Requires upgrade to NC 3.1.2 PR.
Feature for creating improved test results for reporting when test log-level is set to DEBUG. Adds IR models to .outputs/test_dir, result tensor differences, text variant of input and result tensors, summary with test info.
Test plan
Test provided. All tests that use NSYS.
cc @robert-kalmar