Skip to content

Improve checks in GenericPattern matcher #2155

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

shubhambhokare1
Copy link
Contributor

@shubhambhokare1 shubhambhokare1 commented Apr 1, 2025

  • Support None inputs
  • Add support for allow_extra_inputs
  • TODO: Add support for allow_extra_attributes

Copy link

codecov bot commented Apr 1, 2025

❌ 8 Tests Failed:

Tests completed Failed Passed Skipped
5771 8 5763 2816
View the top 3 failed test(s) by shortest run time
onnxscript.rewriter.generic_pattern_test.GenericPatternTest_0::test_transpose_transpose_onnxscript
Stack Traces | 0.001s run time
onnxscript/rewriter/generic_pattern_test.py:594: in test_transpose_transpose_onnxscript
    rule.apply_to_model(ir_model)
onnxscript/rewriter/pattern.py:1426: in apply_to_model
    return RewriteRuleSet([self], commute=commute).apply_to_model(
onnxscript/rewriter/pattern.py:1799: in apply_to_model
    count = self._apply_to_graph_or_function(
onnxscript/rewriter/pattern.py:1702: in _apply_to_graph_or_function
    delta = rule.try_rewrite(
onnxscript/rewriter/pattern.py:1372: in try_rewrite
    match = self._matcher.match(
onnxscript/rewriter/generic_pattern.py:707: in match
    return _to_match_result(PatternMatchResult(self.pattern, matched_nodes))
onnxscript/rewriter/generic_pattern.py:63: in __init__
    self._bind_attributes(pattern_node, graph_node)
E   AttributeError: 'PatternMatchResult' object has no attribute '_bind_attributes'
onnxscript.backend.onnx_export_test.TestOnnxBackEnd::test_export2python_produces_correct_onnx_script_model_0647_test_min_example
Stack Traces | 0.003s run time
onnxscript\backend\onnx_export_test.py:137: in extract_functions
    mod = importlib.import_module(import_name)
C:\hostedtoolcache\windows\Python\3.10.11\x64\lib\importlib\__init__.py:126: in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
E   ModuleNotFoundError: No module named 'tests.onnx_backend_test_code.test_min_example'

The above exception was the direct cause of the following exception:
.nox\test\lib\site-packages\parameterized\parameterized.py:620: in standalone_func
    return func(*(a + p.args), **p.kwargs, **kw)
onnxscript\backend\onnx_export_test.py:271: in test_export2python_produces_correct_onnx_script_model
    functions = extract_functions(backend_test.name, code, self.test_folder)
onnxscript\backend\onnx_export_test.py:139: in extract_functions
    raise AssertionError(
E   AssertionError: Unable to import 'tests.onnx_backend_test_code.test_min_example' (e=No module named 'tests.onnx_backend_test_code.test_min_example') (file: 'D:\\a\\onnxscript\\onnxscript\\tests\\onnx_backend_test_code\\test_min_example.py', absolute path: 'D:\\a\\onnxscript\\onnxscript\\tests\\onnx_backend_test_code\\test_min_example.py', current folder: D:\a\onnxscript\onnxscript
E   ---- CONTENT --
E   import numpy
E   from onnx import TensorProto
E   from onnx.helper import make_tensor
E   from onnxscript import script, external_tensor
E   from onnxscript.values import Opset
E   from onnxscript.onnx_types import FLOAT
E   from onnxscript.onnx_opset import opset13
E   
E   @script()
E   def bck_test_min_example(data_0: FLOAT[3], data_1: FLOAT[3], data_2: FLOAT[3]) -> (FLOAT[3]):
E       result = opset13.Min(data_0, data_1, data_2)
E       return result
onnxscript.rewriter.generic_pattern_test.GenericPatternTest_0::test_rotary_embedding_onnxscript
Stack Traces | 0.003s run time
onnxscript/rewriter/generic_pattern_test.py:455: in test_rotary_embedding_onnxscript
    rule.apply_to_model(ir_model)
onnxscript/rewriter/pattern.py:1426: in apply_to_model
    return RewriteRuleSet([self], commute=commute).apply_to_model(
onnxscript/rewriter/pattern.py:1799: in apply_to_model
    count = self._apply_to_graph_or_function(
onnxscript/rewriter/pattern.py:1702: in _apply_to_graph_or_function
    delta = rule.try_rewrite(
onnxscript/rewriter/pattern.py:1372: in try_rewrite
    match = self._matcher.match(
onnxscript/rewriter/generic_pattern.py:707: in match
    return _to_match_result(PatternMatchResult(self.pattern, matched_nodes))
onnxscript/rewriter/generic_pattern.py:63: in __init__
    self._bind_attributes(pattern_node, graph_node)
E   AttributeError: 'PatternMatchResult' object has no attribute '_bind_attributes'

To view more test analytics, go to the Test Analytics Dashboard
📋 Got 3 mins? Take this short survey to help us improve Test Analytics.

@@ -53,6 +58,9 @@
)
for a, b in zip(graph_node.outputs, pattern_node.outputs):
self._bind(b, a)

Check warning

Code scanning / lintrunner

EDITORCONFIG-CHECKER/editorconfig Warning

Trailing whitespace
@@ -53,6 +58,9 @@
)
for a, b in zip(graph_node.outputs, pattern_node.outputs):
self._bind(b, a)

Check warning

Code scanning / lintrunner

RUFF/W293 Warning

@justinchuby
Copy link
Collaborator

Is there a plan to merge GenericPattern with Pattern?

@gramalingam
Copy link
Collaborator

Any unit tests?

@justinchuby justinchuby requested a review from Copilot April 11, 2025 15:14
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (2)

onnxscript/rewriter/generic_pattern.py:42

  • In the init method, the check requires graph_node.inputs to be strictly greater than pattern_node.inputs when allow_other_inputs is true, while _match_backward only checks if it is not less. Consider aligning these conditions (e.g., using '>=' in both places) to ensure consistent behavior.
if pattern_node.allow_other_inputs:

onnxscript/rewriter/generic_pattern.py:42

  • The code uses the property 'allow_other_inputs' while the PR description refers to 'allow_extra_inputs'. Consider using a single term for clarity across the code and documentation.
if pattern_node.allow_other_inputs:

@shubhambhokare1
Copy link
Contributor Author

Closing for now, as we are not focusing on improving the generic pattern matcher

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

3 participants