fix unknow pattern case#1162
Conversation
DingmaomaoBJTU
left a comment
There was a problem hiding this comment.
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: Patternlost its type annotation inGemmPatternInputGenerator; 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.
| from .op_input_gen import InputShapeConstraint, InputValueConstraint | ||
|
|
||
|
|
||
| def _is_scalar_constraint_value(value: Any) -> bool: |
There was a problem hiding this comment.
This scalar-vs-vector bias logic is the core behavioral change on the Gemm side, but there's no unit test for it in this PR (only the transpose generator got new tests). Please add coverage asserting C_is_value_constraint is True for a scalar bias and False for a 1-D/N-D vector constant and for None — that's exactly the distinction this fix hinges on, and it'd lock the regression in.
|
|
||
| # Typed as the base Pattern so subclasses can set a different concrete pattern. | ||
| pattern: Pattern = ReshapeGemmReshapePattern() | ||
| pattern = ReshapeGemmReshapePattern() |
There was a problem hiding this comment.
This drops the explicit : Pattern annotation, but the comment right above still says "Typed as the base Pattern so subclasses can set a different concrete pattern." Without the annotation the attribute falls back to the base Pattern | None type, and this change looks unrelated to the unknown-pattern fix. Suggest restoring pattern: Pattern = ReshapeGemmReshapePattern() (or dropping the now-inaccurate comment).
| c_value = item.get("C_value") | ||
| item["C_is_value_constraint"] = _is_scalar_constraint_value(c_value) and item[ | ||
| "C_dim" | ||
| ] == 0 |
There was a problem hiding this comment.
Is and item["C_dim"] == 0 adding anything beyond _is_scalar_constraint_value(c_value)? A scalar / 0-d value already implies C_shape == () → C_dim == 0, so this second clause looks redundant. If it's intentional defensive coding for rows where value and shape disagree, a one-line comment would help; otherwise it can be dropped to keep the intent clear.
| "data": InputShapeConstraint((1, 256, 256, 96)), | ||
| "transpose_shape": (1, 32, 8, 32, 8, 96), | ||
| "perm": (0, 1, 3, 2, 4, 5), | ||
| "output_shape": (1024, 64, 96), |
There was a problem hiding this comment.
Good — this is the missing 3-D output coverage (and the later case adds merged_transpose_dim=5). One thing to confirm before this actually fixes anything for users: the runtime matcher loads precomputed rule tables (parquet/JSON from a release, per rules/runtime_check_rules/README.md), not the generators directly. So these new combinations only change classification once the runtime-check rules are regenerated and re-released. Is that regeneration planned as part of this change, or does it happen out-of-band?
| class TestRTRPatternInputGeneratorCoverage: | ||
| """Regression tests for RTR pattern input-generation coverage.""" | ||
|
|
||
| def test_output_dim_coverage_includes_3d_and_4d(self) -> None: |
There was a problem hiding this comment.
These tests assert that the generator returns the combinations that were just hard-coded above — useful as a guard against accidental removal, but they don't exercise the path that actually produced the "unknown pattern" (property derivation → rule matching in runtime_checker_query). Consider adding a regression test that runs a representative previously-unknown subgraph through the matcher and asserts it's now classified, so the tests protect the behavior rather than the fixture.
c81a272 to
92bc5a9
Compare
9a94882 to
3bfb39d
Compare
No description provided.