fix(perf): support multiple QNN EP context partitions - #1361
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
a follow up #1362 |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Qiong Wu (qiowu) (DingmaomaoBJTU)
left a comment
Qiong Wu (qiowu) (DingmaomaoBJTU)
left a comment
There was a problem hiding this comment.
Found two actionable regressions in this change.
| "hvx_threads": max( | ||
| sample["metadata"]["hvx_threads"] for sample in partition_samples | ||
| ), | ||
| "accel_execute_cycles": sum( |
There was a problem hiding this comment.
Could we normalize each partition's metadata before aggregating it? _parse_artifacts() explicitly accepts QNN numeric values such as "120000.7" via _to_int(), but this new sum() runs first. With multiple partitions, summing those strings raises TypeError, the monitor reports parse_failed, and winml perf --op-tracing exits 4. A regression test combining multiple partitions with float-string metadata would catch this.
There was a problem hiding this comment.
Fixed. QNN metadata normalization is now shared by partition coalescing and downstream conversion, including float-string values. Added a multi-partition float-string regression test.
| trace_data = json.loads(trace_files[0].read_text(encoding="utf-8")) | ||
| report_data = json.loads(output_path.read_text(encoding="utf-8")) | ||
| trace_data = report_data["hw_monitor"]["ep_proof"] | ||
| assert not list(tmp_path.glob("*_op_trace.json")) |
There was a problem hiding this comment.
This behavior change also leaves tests/e2e/test_perf_e2e.py::TestPerfOnnx::test_op_tracing_basic_qnn_npu expecting and reading perf_op_tracing_qnn_npu_op_trace.json (lines 805/823). Since perf.py no longer writes that artifact, the QNN hardware E2E now fails. Please update that E2E to read output_file["hw_monitor"]["ep_proof"] and assert the standalone trace file is absent, as this test does.
There was a problem hiding this comment.
Updated the existing QNN NPU E2E to read hw_monitor.ep_proof from the main perf JSON and assert the standalone _op_trace.json is absent. The enabled hardware E2E passes.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
2 participants
Summary