fix unknow pattern case by fangyangci · Pull Request #1162 · microsoft/winml-cli

fangyangci · 2026-07-21T08:04:04Z

related check results and rules parquet:
https://github.com/gim-home/ModelKitArtifacts/pull/156

DingmaomaoBJTU

Reviewed the fix for the "unknown pattern" cases. The overall approach is sound: broaden the RTR transpose input-generator to cover 3-D outputs and merged_transpose_dim=5, and make the Gemm generator treat only scalar biases as value-constraints so vector/tensor biases stay shape-constrained (instead of emitting value-based rules that real models never match). I hand-checked the added transpose combinations — they're all element-count consistent, and the PixelShuffle case does resolve to merged_transpose_dim=5.

A few things to look at (details inline):

The Gemm-side behavioral change (_is_scalar_constraint_value / C_is_value_constraint) has no unit test — only the transpose generator got new tests.
pattern: Pattern lost its type annotation in GemmPatternInputGenerator; that looks unrelated to this fix and now contradicts the comment directly above it.
The new transpose tests assert the hard-coded fixture rather than the derivation → rule-matching path that actually produced "unknown".
End-to-end: the matcher loads precomputed rule tables (parquet/JSON pulled from a release, per rules/runtime_check_rules/README.md), so these generator changes won't change real analyses until the runtime-check rules are regenerated and re-released. Is that regeneration planned as part of landing this?

Note: I couldn't run the suite locally (ARM64 host vs. the AMD64/linux lockfile), so please rely on CI to confirm the new tests pass.

…ngyangci/fix_unknow_patterns

DingmaomaoBJTU

Overall solid fix for unknown patterns with good test coverage. A few concerns below, primarily about the Attention domain and the base.py shape change.

DingmaomaoBJTU · 2026-07-28T02:45:38Z

+# Copyright (c) Microsoft Corporation. All rights reserved.
+# Licensed under the MIT License.
+# --------------------------------------------------------------------------
+"""Input generator for ai.onnx Attention operator (opset 23+).


The docstring says ai.onnx Attention operator (opset 23+) but available_ops_all.json lists Attention with start_version: 99999 and valid_for_gene: false - this is a com.microsoft contrib op, not a standard ai.onnx op.

Should this class set op_domain = "com.microsoft" so it registers under the domain-qualified key com.microsoft::Attention (same pattern as the Gelu generator)? Without it, the op registers under the default ai.onnx domain which seems incorrect.

DingmaomaoBJTU · 2026-07-28T02:45:38Z

+            {
+                "Q": InputShapeConstraint((2, 4, 8, 16)),
+                "K": InputShapeConstraint((2, 4, 8, 16)),
+                "V": InputShapeConstraint((2, 4, 8, 16)),


scale: 1.0 / 16 is a raw float mixed with InputShapeConstraint entries in the dict. Worth adding a comment clarifying that non-InputShapeConstraint keys are forwarded as op attributes, since the other entries are all shapes and it's not immediately obvious.

DingmaomaoBJTU · 2026-07-28T02:45:38Z


-            output_tensor = helper.make_tensor_value_info(output_name, elem_type, None)
+            # Keep shape present for ONNX checker while leaving dimensions unknown.
+            output_tensor = helper.make_tensor_value_info(output_name, elem_type, [None])


This changes the output shape from None (unknown rank) to [None] (rank-1 with one unknown dim). For patterns whose output is multi-dimensional (e.g., matmul -> 2-D, conv -> 4-D), a hardcoded rank-1 shape declaration is semantically incorrect and may cause ONNX checker warnings.

Consider using [None] * expected_output_rank derived from pattern metadata, or documenting why rank-1 is always safe here.

DingmaomaoBJTU · 2026-07-28T02:45:38Z

            },
+            {
+                "data": InputShapeConstraint((1, 256, 256, 96)),
+                "transpose_shape": (1, 32, 8, 32, 8, 96),


Nit: This method grew from 2 entries to 20+. Many entries share the same perm=(0,1,3,2,4,5) and only vary spatial/hidden dims and output shape. Consider generating these programmatically from a compact (spatial, hidden_size, output_dims) spec to reduce maintenance burden.

DingmaomaoBJTU · 2026-07-28T02:45:38Z

+        # Distinguish scalar literal constraints from shape-based constraints.
+        # Vector/tensor constants should remain shape-constrained (not value-constrained).
+        c_value = item.get("C_value")
+        # Keep the shape gate as a defensive consistency check in case value/shape


👍 Nice fix - the old check incorrectly treated vector constants as value-constrained. The new scalar-only gate with the C_dim == 0 defensive check is correct and well-tested.

DingmaomaoBJTU · 2026-07-28T02:45:38Z

        None,
    )
-
+    new_nodes = list(model.graph.node)


Nit: new_nodes = list(model.graph.node) is the same expression that was already inlined. The extra local variable doesn't seem to serve any purpose - was there an intent to mutate it before passing?

DingmaomaoBJTU · 2026-07-28T02:45:38Z

-        assert len(get_registered_operators()) == 120
+        registered_ops = get_registered_operators()
+
+        # Keep a lower bound so additive registrations do not break this test.


👍 Good change - >= 120 lower bound + uniqueness check makes this test resilient to additive registrations without masking accidental removals.

fangyangci · 2026-07-28T03:32:18Z

Tracking issue: #1236

fangyangci requested a review from a team as a code owner July 21, 2026 08:04

DingmaomaoBJTU reviewed Jul 22, 2026

View reviewed changes

fangyangci force-pushed the fangyangci/fix_unknow_patterns branch 3 times, most recently from 3bfb39d to a2c09c0 Compare July 27, 2026 02:22

fix unknown patterns

95acbe5

fangyangci force-pushed the fangyangci/fix_unknow_patterns branch from a2c09c0 to 95acbe5 Compare July 27, 2026 03:00

fangyangci added 10 commits July 27, 2026 11:04

Merge branch 'main' of https://github.com/microsoft/winml-cli into fa…

0240d14

…ngyangci/fix_unknow_patterns

Add regression tests for Gemm bias and RTR query conditions

993eed3

Clarify defensive C_dim gate for Gemm bias constraint

6bc5182

fix lint

d239c9e

Fix GemmPatternInputGenerator pattern type for mypy

abc5a75

fix lint

0cceb53

Fix SingleGelu universal pattern validation and registry count

cdc702b

Make operator registry test resilient to additive registrations

9e1599d

chore: retrigger PR checks

1ad39c4

Merge origin/main into fangyangci/fix_unknow_patterns

02e0ca3

DingmaomaoBJTU reviewed Jul 28, 2026

View reviewed changes

fangyangci mentioned this pull request Jul 28, 2026

跟踪: fix unknow pattern case #1236

Open

2 tasks

Uh oh!

Conversation

fangyangci commented Jul 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DingmaomaoBJTU left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DingmaomaoBJTU left a comment

Choose a reason for hiding this comment

Uh oh!

DingmaomaoBJTU Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

DingmaomaoBJTU Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

DingmaomaoBJTU Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

DingmaomaoBJTU Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

DingmaomaoBJTU Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

DingmaomaoBJTU Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

DingmaomaoBJTU Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

fangyangci commented Jul 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

fangyangci commented Jul 21, 2026 •

edited

Loading