Auto-enable op-tracing via per-model op_tracing_targets#1032
Conversation
Add an op_tracing_targets list to model registry entries so op-tracing turns on automatically for a given <EP>_<device> target (e.g. QNNExecutionProvider_npu) unless --op-tracing is explicitly set. Add 'disabled' choice to force it off.
DingmaomaoBJTU
left a comment
DingmaomaoBJTU
left a comment
There was a problem hiding this comment.
Reviewed the per-model auto-enable op-tracing change. The approach is clean overall: tracing runs as a separate additive pass so it doesn't skew the benchmark, and disabled gives an escape hatch. A few things worth addressing, the main one being the device=auto matching gap. Details inline.
… add tests - Centralize the <EP>_<device> key builder in registry (op_tracing_target_key) and reuse it in run_eval so there is a single source of truth. - Normalize op_tracing_targets on load so the registry can use the short EP alias (qnn_npu) consistent with the rest of the eval tooling; both alias and full name map to the canonical QNNExecutionProvider_npu. - Document that auto-enable requires explicit --ep and --device (default --device auto does not match a device-specific target). - Add unit tests for _resolve_op_tracing (disabled-overrides-optin, explicit levels, auto-device no-match, no-ep) and the key builder/normalizer.
DingmaomaoBJTU
left a comment
DingmaomaoBJTU
left a comment
There was a problem hiding this comment.
Re-reviewed the follow-up commit (bbef45c). Thanks for the quick turnaround — all four points are addressed well:
- device=auto gap — not silently ignored anymore: documented in the
_resolve_op_tracingdocstring, the--op-tracinghelp text, and pinned bytest_unset_device_auto_does_not_match. Reasonable to require explicit--ep/--devicerather than guessing. - Tests —
TestResolveOpTracing+TestOpTracingTargetKeycover disabled-overrides-optin, explicit levels, matching/non-matching, auto-device, and no-ep. I ran the file locally: 57 passed, ruff clean. - Naming — centralizing on
op_tracing_target_key/_canonical_ep_device_keyand accepting bothqnn_npuand the full form (JSON now uses the short alias, consistent with the pair catalog) resolves the two-conventions concern. Key builder and normalizer share one code path, so they can't drift. - Validation — normalization handles case/alias variance; one residual gap noted inline (non-blocking).
Looks good to merge once you've had a look at the inline note.
DingmaomaoBJTU
left a comment
DingmaomaoBJTU
left a comment
There was a problem hiding this comment.
Approving. All four review points from the prior round are addressed; op-tracing runs as a separate additive pass so it does not skew the benchmark, disabled gives a clean override, and the key builder/normalizer share one code path. Verified locally: 57 tests pass in tests/unit/eval/test_run_eval_script.py and ruff is clean. The remaining registry-validation note is non-blocking (every current target is qnn_npu and CI pins --device npu).
2 participants
Summary
Adds per-model auto-enable of op-tracing in the e2e eval runner. When
run_eval.pyruns without an explicit--op-tracing, a model whose registry entry opts into the current<EP>_<device>target now getsbasicop-tracing automatically.Changes
scripts/e2e_eval/utils/registry.py— addop_tracing_targets: list[str]field toModelEntryand load it inload_registry.scripts/e2e_eval/run_eval.py_op_tracing_target_key(ep, device)builds the match key, e.g.QNNExecutionProvider_npu._resolve_op_tracing(cli, entry, ep, device)resolves the effective level:disabledforces off (even for opted-in models);basic/detailused as-is; unset defaults tobasicwhen the target key is in the model'sop_tracing_targets, otherwise off.disabledto--op-tracingchoices with expanded help text.op_tracingper model in the run loop and pass it to bothrun_modelcall sites.scripts/e2e_eval/testsets/models_all.json— add"op_tracing_targets": ["QNNExecutionProvider_npu"]to the 10 profiled models.Behavior
--op-tracingop_tracing_targetsmatches)basicbasic/detaildisabled