Skip to content

Remove validated ORT core patches - #882

Open
tzuhsuanwei wants to merge 1 commit into
mainfrom
dev/tzuhwei/remove-ort-core-patches-0002-0005
Open

tzuhsuanwei wants to merge 1 commit into
mainfrom
dev/tzuhwei/remove-ort-core-patches-0002-0005

Conversation

@tzuhsuanwei

@tzuhsuanwei tzuhsuanwei commented Sep 29, 2026 •

Copy link
Copy Markdown
Collaborator

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.

  • Remove 0002, 0004, and 0005 from the FetchContent patch chain and delete their patch files.
  • Re-home 0002's former RoiAlign exclusions in the QNN test harness, not ORT core.

Final patch disposition

Patch Decision Evidence / removal gate
0001 plugin EP runner Keep QNN still depends on the local plugin-EP runner implementation.
0002 RoiAlign HTP exclusions Remove Upstream runner has no individual-case skip CLI. QNN filter explicitly excludes the three former cases before runner invocation and logs reasons.
0003 arm64ReproDir Keep Upstream arm64x.cmake unconditionally overwrites arm64ReproDir; outer QNN CMake cannot fix it after ORT configures. Keep until upstream supplies a cache variable/override hook and ARM64X repro paths validate.
0004 monolithic LSTM option Remove Plugin runner forwards arbitrary key/value provider options; explicit enumeration is redundant.
0005 MSVC C4875 Remove Pinned ORT v1.29 contains upstream GSL C4875 root-cause fix.
0006 No action No patch file or application reference exists.
0007 Android QNN_LIB_FILES copy Keep Android Java/AAR path can still invoke cmake -E copy with an empty source list.
0008 rank-5/6 HandleReshapeSplit Keep QNN SpaceToDepth/ChannelShuffle fusions need hardening for the optimizer-produced form.

Validation

  • Fresh ARM64 source build after moving prior build tree aside.
  • Fresh ort_core patch stamp omits 0002; fresh ORT TestCase.cc/main.cc contain no RoiAlign exclusions.
  • Fresh ARM64 runner and matching DLLs deployed to V81.
  • V81 HTP run with backend_type|htp and htp_arch|81: Models 1, Succeeded 1, Not implemented 0, Failed 0.
  • Filter staging suite excludes test_roialign_aligned_false, test_roialign_aligned_true, and test_roialign_mode_max while retaining test_abs.
  • Python 3.8 filter smoke test passed; Windows launcher PowerShell syntax passed.
  • Repository lint was run before the PR reduction/reinstatement; this PR's restored filter code is the same validated implementation.

@tzuhsuanwei
tzuhsuanwei force-pushed the dev/tzuhwei/remove-ort-core-patches-0002-0005 branch 4 times, most recently from 25c82fa to e8e89ae Compare September 29, 2026 08:46
@qti-yuduo
qti-yuduo force-pushed the dev/tzuhwei/remove-ort-core-patches-0002-0005 branch from e8e89ae to 5de73e0 Compare September 29, 2026 16:20
@tzuhsuanwei
tzuhsuanwei force-pushed the dev/tzuhwei/remove-ort-core-patches-0002-0005 branch 2 times, most recently from b84d224 to 6ff9209 Compare October 1, 2026 05:27
Comment thread cmake/external/onnxruntime_external_deps.cmake
@tzuhsuanwei
tzuhsuanwei force-pushed the dev/tzuhwei/remove-ort-core-patches-0002-0005 branch from 6ff9209 to c562df5 Compare October 1, 2026 08:41
@tzuhsuanwei
tzuhsuanwei force-pushed the dev/tzuhwei/remove-ort-core-patches-0002-0005 branch from c562df5 to 1ac37c2 Compare October 1, 2026 08:59
Comment thread qcom/scripts/all/model_test_filter.py Outdated
# 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]] = {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread qcom/scripts/windows/run_tests.ps1 Outdated
$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 }

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@tzuhsuanwei
tzuhsuanwei force-pushed the dev/tzuhwei/remove-ort-core-patches-0002-0005 branch 2 times, most recently from 472199c to 9cfdfe9 Compare October 1, 2026 09:55
@tzuhsuanwei
tzuhsuanwei force-pushed the dev/tzuhwei/remove-ort-core-patches-0002-0005 branch from 9cfdfe9 to 4532f44 Compare October 1, 2026 10:08

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants