Unify the two pattern-matchers #1495
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| # if replacement is not None: | ||
| # TODO(Rama) | ||
| # assert len(replacement.new_outputs) == len(match_result.pattern_outputs), ( | ||
| # f"Not the same number of outputs, matched " | ||
| # f"outputs={match_result.pattern_outputs}, " | ||
| # f"got {replacement.new_outputs} in the applied pattern." |
Check notice
Code scanning / CodeQL
Commented-out code Note
| # def transpose_transpose_pattern(op, X, perm0, perm1): | ||
| # xt = op.Transpose(X, perm=perm0) | ||
| # Y = op.Transpose(xt, perm=perm1) | ||
| # return Y |
Check notice
Code scanning / CodeQL
Commented-out code Note
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1495 +/- ##
==========================================
- Coverage 76.87% 76.82% -0.05%
==========================================
Files 207 207
Lines 22345 22271 -74
Branches 3798 3775 -23
==========================================
- Hits 17177 17110 -67
+ Misses 4443 4440 -3
+ Partials 725 721 -4 ☔ View full report in Codecov by Sentry. |
Test Results 30 files ± 0 30 suites ±0 3h 12m 40s ⏱️ + 4m 17s For more details on these failures, see this check. Results for commit fd7731b. ± Comparison against base commit a1e46e4. This pull request removes 2027 tests.♻️ This comment has been updated with latest results. |
| @@ -46,7 +42,7 @@ def __init__( | |||
| self.pattern_outputs = pattern_outputs | |||
| self.kwargs: dict[str, Any] = {} | |||
|
|
|||
| matched_pattern_to_model_value: dict[ir.Value, ir.Value] = {} | |||
| matched_pattern_to_model_value: dict[str, ir.Value] = {} | |||
There was a problem hiding this comment.
Is it possible when the name is None, or if the names of two values collide?
There was a problem hiding this comment.
Good question. For now, there is an assert. May be better to make it conditional update to map. Eventually, this should be replaced by the pattern IR (instead of onnx IR).
There was a problem hiding this comment.
In the end, we care about binding only users want, for which they will give a name
|
|
||
| # There should be a better way. | ||
| sig = inspect.signature(match_pattern_function) | ||
| for i, p in enumerate(sig.parameters.values()): |
There was a problem hiding this comment.
| for i, p in enumerate(sig.parameters.values()): | |
| for i, parameter in enumerate(sig.parameters.values()): |
| if isinstance(outputs, Sequence): | ||
| value.name = outputs[0] | ||
| return value | ||
| values = self._tape.op_multi_output( |
There was a problem hiding this comment.
Q: Do we have multi-outputs with pattern.py now? If so, should we remove generic_pattern.py to avoid the potential confusion that users might have?
There was a problem hiding this comment.
No. This is different (it's about creating nodes with multiple outputs).
Update generic pattern matcher to support the unified API.
Still todo: