Remove validated ORT core patches - #882
tzuhsuanwei wants to merge 1 commit into
Conversation
25c82fa to
e8e89ae
Compare
e8e89ae to
5de73e0
Compare
b84d224 to
6ff9209
Compare
6ff9209 to
c562df5
Compare
c562df5 to
1ac37c2
Compare
| # Keys are suite names and values map test case directory names to their reason. | ||
| # Keep this list deliberately narrow: it records the behavior formerly | ||
| # hard-coded in the upstream ORT test runner. | ||
| QNN_MODEL_TEST_EXCLUSIONS: Mapping[str, Mapping[str, str]] = { |
There was a problem hiding this comment.
The exclusion reasons describe these as QNN HTP limitations, but the node suite is currently executed with the QNN CPU backend in run_tests.sh, run_tests.ps1, and test_ort.py.
Because filter_model_test_suite() does not receive a backend, these cases are removed for every backend. This may silently reduce valid CPU coverage if the limitation is actually HTP-specific.
Could we clarify the intended scope?
- If this is a QNN EP-wide limitation, please change the reasons from “QNN HTP” to “QNN EP”.
- If it is HTP-only, please include the backend in the exclusion key/signature and add CPU-vs-HTP tests.
There was a problem hiding this comment.
Thanks. I validated the three RoiAlign opset-22 cases on V81 and made the exclusions backend-aware. With backend_type|cpu, all three cases report Not implemented. With HTP, roialign_aligned_false and roialign_aligned_true succeed, while roialign_mode_max remains unsupported. The filter now receives the backend from the Windows, Linux, and QDC/Appium call sites: CPU excludes the three unsupported opset-22 cases, while HTP excludes only mode=max. Unit coverage now verifies CPU, HTP, and unconfigured-backend behavior.
| "${runner_test_path}" 2>&1 | tee -a "${model_log}" | ||
| test_return_code=$? | ||
| set -e | ||
| if [ -n "${filtered_test_root}" ]; then |
There was a problem hiding this comment.
Could we guarantee cleanup of filtered_test_root on every exit path?
With set -e, a failure in model_test_filter.py, an interrupted runner, or another early return can exit before line 78 and leave the copied suite under the system temporary directory.
Please register cleanup immediately after mktemp, for example with a function-scoped RETURN/EXIT trap, and use rm -rf -- "${filtered_test_root}". A failure-path test would also help
prevent this from regressing.
There was a problem hiding this comment.
Done. Cleanup is registered immediately after mktemp with RETURN and EXIT traps and uses rm -rf --. The filter-failure path is handled explicitly, and cleanup failures cannot replace the model-runner result. I also added a Linux regression test that verifies cleanup on both a normal function return and an early shell exit.
| $TestPath | Tee-Object -FilePath $ModelLog | Write-Host | ||
| $RunnerTestPath | Tee-Object -FilePath $ModelLog -Append | Write-Host | ||
| $TestResult = $? | ||
| if ($null -ne $FilteredTestPath) { Remove-Item -Recurse -Force $FilteredTestPath } |
There was a problem hiding this comment.
Please wrap creation and use of $FilteredTestPath in try/finally.
At present, a terminating error from the filter or runner bypasses line 178, leaving a partially copied model suite in the system temp directory. This is especially likely for disk-full,
permission, or interrupted-job failures.
The finally block should remove the directory with -LiteralPath, while preserving the runner result for the existing failure handling.
There was a problem hiding this comment.
Done. Creation and use of the filtered suite are wrapped in try/finally. The finally block removes the directory with Remove-Item -LiteralPath -Recurse -Force. The runner result is captured before cleanup, and a cleanup failure emits a warning rather than overriding the existing test result.
472199c to
9cfdfe9
Compare
9cfdfe9 to
4532f44
Compare
Summary
This PR removes ORT-core patches 0002, 0004, and 0005 so the QNN plugin can build against a cleaner, unmodified upstream ORT source tree.
Patch 0002 previously changed ORT core solely to prevent the model runner from executing three QNN HTP RoiAlign test directories. The upstream plugin runner accepts a suite directory but has no per-case skip argument. Removing 0002 without replacement would make the normal full node-suite workflow execute est_roialign_mode_max, which is currently unsupported on QNN HTP and returns Not implemented.
Therefore this PR moves that test-selection policy out of ORT core and into a QNN-owned Python filter. Windows and Linux launchers call the filter before invoking the upstream plugin runner; Appium/QDC applies it before uploading the node suite to the device. The runner receives a temporary filtered suite, so ORT itself remains pristine and no longer needs a QNN-specific skip patch. This preserves the prior test workflow while keeping the ORT patch chain clean.
Final patch disposition
Validation