Make e2e eval recipe/precision expansion NPU-only#1163
Conversation
Recipes and multi-precision expansion now apply only when --device is npu. On NPU, a model without a recipe falls back to winml config expanded into two jobs (w8a8 + w8a16); an explicit per-model precision overrides this. CPU/GPU (and auto) ignore recipes and build a single winml config fallback. Update _build_jobs to take the device, add EvalJob.fallback_precision, adjust _build_for_job, refresh docs and tests.
The fallback build path invoked `winml build -c <config> --device <d>` without `--precision`. Because `--device` is always passed, winml build re-resolves quant/compile from device+precision and overwrites the quant baked into the config by `winml config`. With precision defaulting to auto, an NPU build fell back to w8a16, so a w8a8 fallback job and a w8a16 fallback job produced the same cached artifact (identical config hash) and perf reported w8a16 for both. Forward the resolved precision to the build call as well, mirroring the existing `winml config` invocation, so distinct-precision fallback jobs build distinct artifacts. Add regression tests asserting --precision is passed to both config and build, distinct precisions stay distinct, and the flag is omitted when precision is None.
DingmaomaoBJTU
left a comment
DingmaomaoBJTU
left a comment
There was a problem hiding this comment.
CPU/GPU also need recipes for winml eval. Please just ignore quantized version for them and add fallbacks
The e2e eval runner previously ignored authored recipes entirely off-NPU and always built a single 'winml config' fallback, which also dropped the accuracy eval/dataset config the recipes carry. '_build_jobs' now discovers recipe variants on every device and filters out quantized variants (delegating to winml's 'is_quantized_precision') when the target is not NPU. Off-NPU builds the non-quantized recipe variants (e.g. fp16) and falls back to 'winml config' only when no applicable variant exists. NPU behavior (fp16 + quantized variants, w8a8+w8a16 fallback) is unchanged. Update the README, module docstring, and tests to match.
DingmaomaoBJTU
left a comment
DingmaomaoBJTU
left a comment
There was a problem hiding this comment.
Reviewed the NPU-only recipe/precision expansion. The core change is sound and the --precision forwarding fix is well-reasoned and well-tested. A few observations below — one edge case worth a look (NPU + VitisAI), plus two low-severity notes.
| variants = [v for v in variants if not _is_quantized_precision(v.precision)] | ||
| if variants: | ||
| jobs.extend(EvalJob(entry, variant) for variant in variants) | ||
| elif npu and entry.precision is None: |
There was a problem hiding this comment.
On NPU paired with a skip-quant EP (VitisAI — the documented vitisai_npu combo), a recipe-less model with no per-model precision still expands here into two jobs (w8a8 + w8a16). But _resolve_precision forces precision to None for _EPS_SKIP_WINML_QUANT EPs, so both jobs build the same unquantized artifact and write two eval_result.json (…__w8a8 and …__w8a16) with identical perf — the same "two precisions collapse onto one artifact" class of bug this PR fixes elsewhere. Pre-PR this was a single fallback job, so run_eval.py --device npu --ep vitisai on a recipe-less model regresses to duplicate/mislabeled results.
Since _build_jobs only receives device (not ep), consider also threading ep through and skipping the expansion when _should_skip_winml_quant(ep) (emit a single fallback instead). Not the default path (--ep defaults to None), so low severity, but it's a supported invocation.
There was a problem hiding this comment.
Good catch — fixed in 1f3fc30. _build_jobs now takes ep and skips the recipe-less NPU w8a8+w8a16 expansion when _should_skip_winml_quant(ep) is true, emitting a single winml config fallback instead. Added test_npu_skip_quant_ep_no_recipe_single_fallback covering --device npu --ep vitisai, and noted the behavior in the README.
| if not npu: | ||
| # Off-NPU still uses recipes for their eval config, but drops | ||
| # quantized variants -- quantization is an NPU-only concern here. | ||
| variants = [v for v in variants if not _is_quantized_precision(v.precision)] |
There was a problem hiding this comment.
Off-NPU, a model whose recipe has only quantized variants (e.g. a w8a16 recipe with no fp16) is filtered to an empty list and falls through to the winml config fallback (variant=None → recipe_meta=None). That silently drops the recipe's eval/dataset config for accuracy runs — which is the very reason the PR now consults recipes off-NPU. Likely rare (most recipes ship an fp16 variant), but worth confirming it's acceptable that such models lose their authored accuracy config on CPU/GPU rather than, say, warning.
There was a problem hiding this comment.
Confirmed it's acceptable for now. I scanned all recipes under examples/recipes: across 82 (model, task) groups, none is quantized-only — every group ships a non-quantized (fp16/fp32) variant, so no model loses its authored eval/dataset config off-NPU today. Leaving the current fall-through behavior as-is rather than adding a warning for a case that can't currently trigger; happy to revisit if a quantized-only recipe lands later.
| """Recipe precision, else the pinned fallback precision, else None.""" | ||
| if self.variant is not None: | ||
| return self.variant.precision | ||
| return self.fallback_precision |
There was a problem hiding this comment.
Minor / pre-existing, but the PR reworks this property so worth aligning: for a recipe-less NPU model with an explicit per-model precision (e.g. fp16), fallback_precision is None, so this returns None. Yet _build_for_job builds with job.fallback_precision or job.entry.precision = entry.precision. The result-dir slug, display label, and build_eval_result(precision=…) then record no precision even though the artifact is actually fp16. Returning self.fallback_precision or self.entry.precision would keep the label/slug/recorded precision in sync with what's built (note: that would add a precision suffix to those models' output dirs, so it's a deliberate behavior change).
There was a problem hiding this comment.
Fixed in 1f3fc30. EvalJob.precision now returns self.fallback_precision or self.entry.precision, so a recipe-less job with an explicit per-model precision keeps its slug/label/recorded precision in sync with the built artifact. This does add a precision suffix to those models' output dirs, as you noted — intentional. Updated test_npu_no_recipe_honors_explicit_precision accordingly.
Address PR review feedback: - Suppress the recipe-less NPU w8a8+w8a16 expansion for skip-quant EPs (VitisAI): _resolve_precision forces the flag off for these EPs, so both jobs would build the same unquantized artifact under distinct precision slugs. Thread ep into _build_jobs and emit a single fallback instead. - EvalJob.precision now falls back to entry.precision so a recipe-less job with an explicit per-model precision records a slug/label matching the built artifact.
2 participants
Why
In the e2e eval harness, recipe-driven builds and multi-precision job expansion were applied on every device. That only makes sense on the NPU: recipes target NPU quantization, and expanding a model into several quantized precisions has no meaning on CPU/GPU. This change scopes that behavior to
--device npuand gives NPU models without a recipe a sensible default matrix.What changed
_build_jobstakes the targetdevice. Recipes are only consulted whendevice == "npu". On CPU/GPU/auto, recipes are ignored and each model builds as a singlewinml configfallback job (prior behavior).precisionstill wins and produces a single job.--precisiontowinml build. The fallback path invokedwinml build -c <config> --device npuwithout--precision. Because--deviceis always passed,winml buildre-resolves quant from device+precision and overwrites the quant baked into the config bywinml config. With precision defaulting to auto, the NPU build reverted to w8a16, so a w8a8 job and a w8a16 job produced the same cached artifact (identical config hash) and perf reported w8a16 for both. The build call now forwards the resolved precision, mirroring thewinml configinvocation.Notes for reviewers
device == "npu"match (auto does not count), consistent with_resolve_precision._run_recipe_build) passes neither--devicenor--precision, so the config's quant stays authoritative there. It was never affected by the overwrite issue.--precisionoverwrite mechanism predates this PR and was latent on main (NPU fallback only ever resolved to w8a16, which matched the build's auto default). Introducing the w8a8 variant is what surfaced it; the fix also closes the latent case where a per-modelfp16entry built on NPU would be silently coerced to w8a16.--liststill prints one line per model and does not preview expanded jobs. That is pre-existing behavior and unchanged here.Docs (
scripts/e2e_eval/README.md) and unit tests are updated to cover the NPU-only expansion, the w8a8 + w8a16 fallback, and the--precisionforwarding.