-
Notifications
You must be signed in to change notification settings - Fork 84
Expose match functionality of rewrite-rule by extracting base classes #2447
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
Conversation
…tern matching Co-authored-by: gramalingam <10075881+gramalingam@users.noreply.github.com>
Co-authored-by: gramalingam <10075881+gramalingam@users.noreply.github.com>
This change addresses @gramalingam's feedback to return the match object (which includes success/failure status) instead of always returning None when the initial pattern match fails. This provides more consistent API behavior and makes failure information available when applicable. - Changed PatternImpl.match() to return match object on line 161 - Updated RewriteRule.try_rewrite() to use "if not match:" instead of "if match is None:" - Added test case to verify both None and failed MatchResult are handled correctly - Backward compatible: None still returned for GenericPatternMatcher, failed MatchResult for SimplePatternMatcher Co-authored-by: gramalingam <10075881+gramalingam@users.noreply.github.com>
Co-authored-by: gramalingam <10075881+gramalingam@users.noreply.github.com>
…g failure Co-authored-by: gramalingam <10075881+gramalingam@users.noreply.github.com>
❌ 7 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
Co-authored-by: gramalingam <10075881+gramalingam@users.noreply.github.com>
Agreed! I've renamed |
*, | ||
verbose: int | None = None, | ||
check_nodes_are_removable: bool = True, | ||
tracer: _basics.MatchingTracer | None = None, |
Check failure
Code scanning / CodeQL
Module-level cyclic import Error
onnxscript.rewriter._basics
onnxscript.rewriter._rewrite_rule
definition
import
Copilot Autofix
AI 3 months ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
Signed-off-by: Ganesan Ramalingam <grama@microsoft.com>
Co-authored-by: justinchuby <11205048+justinchuby@users.noreply.github.com>
Co-authored-by: gramalingam <10075881+gramalingam@users.noreply.github.com>
LGTM - I think a follow up is needed: The way "Pattern" and "PatternBase" are related can be confusing. It isn't clear if they are related at all even though they have similar names. |
Yes. This can be addressed later, but I think it stems from a similar confusion between RewriteRuleClassBase and RewriteRule (the latter being the original, and the former becoming preferred for more complex rules). |
This PR extracts the pattern matching functionality from rewrite rules into standalone base classes, allowing users to use pattern matching without needing the replacement functionality.
Changes
New Base Classes
PatternImpl
: Core pattern matching functionality_target_pattern
,_matcher
, and_condition_function
match()
method that returnsMatchResult
orNone
PatternBase
: Base class for class-based pattern definitionpattern()
method for defining patternscheck()
method for condition functionscreate_pattern_impl()
method to generatePatternImpl
instancesUpdated Classes
RewriteRule
: Now inherits fromPatternImpl
match()
method intry_rewrite()
RewriteRuleClassBase
: Now inherits fromPatternBase
rule()
class method to createRewriteRule
instancesUsage Examples
Standalone Pattern Matching
Class-Based Pattern Definition
Existing Functionality Preserved
Backward Compatibility
All existing functionality is preserved. The changes are purely additive - existing code using
RewriteRule
andRewriteRuleClassBase
will continue to work without modification.Testing
Fixes #2446.
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.