Skip to content
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

Pattern IR cleanup #1475

Merged
merged 19 commits into from
May 2, 2024
Merged

Pattern IR cleanup #1475

merged 19 commits into from
May 2, 2024

Conversation

gramalingam
Copy link
Collaborator

@gramalingam gramalingam commented Apr 29, 2024

Continuation of the pattern cleanup. End goal is to have a small standalone pattern IR, and separate the matching algorithm (and other utilities like 'commute') out of the IR, allowing same IR to be used by different matching algorithms, which is yet to be done. This PR

  • Simplifies the core pattern constructs (primitives, used to match against strings, as well as attribute-patterns).
  • Removes names from primitives, attribute-patterns can have optional name used for binding in a match.
  • Introduce GraphPattern

TODO:

  • And other fields (like nodes) to GraphPattern (used by multi-output matcher, not single-output matcher).
  • Introduce pattern-builder object as parameter to function constructing target-graph-pattern
  • Reimplement commutes() externally (fixing a bug that exists there).
  • Support for tensor constants, as well as approximate-equality comparison for floats. (Switching defaults here.)

@gramalingam gramalingam marked this pull request as draft April 29, 2024 15:55
@property
def name(self) -> str:
return self._name
def matches(self, item: T) -> bool: ...

Check notice

Code scanning / CodeQL

Statement has no effect Note

This statement has no effect.
onnxscript/rewriter/pattern.py Fixed Show fixed Hide fixed
onnxscript/rewriter/pattern.py Fixed Show fixed Hide fixed
Copy link

codecov bot commented Apr 29, 2024

Codecov Report

Attention: Patch coverage is 70.53571% with 33 lines in your changes are missing coverage. Please review.

Project coverage is 76.80%. Comparing base (b81a38a) to head (033130f).

Files Patch % Lines
onnxscript/rewriter/pattern.py 70.53% 17 Missing and 16 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1475      +/-   ##
==========================================
- Coverage   76.85%   76.80%   -0.06%     
==========================================
  Files         209      209              
  Lines       22476    22416      -60     
  Branches     3817     3814       -3     
==========================================
- Hits        17275    17217      -58     
+ Misses       4483     4471      -12     
- Partials      718      728      +10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

github-actions bot commented Apr 29, 2024

Test Results

     30 files  ±     0      30 suites  ±0   3h 5m 34s ⏱️ - 7m 58s
  5 933 tests + 2 029   4 043 ✅ +1 621    1 886 💤 +   407   4 ❌ +1 
547 578 runs   - 16 784  86 705 ✅  - 1 357  460 815 💤  - 15 428  58 ❌ +1 

For more details on these failures, see this check.

Results for commit 71a7875. ± Comparison against base commit 03b55e3.

♻️ This comment has been updated with latest results.

onnxscript/rewriter/pattern.py Fixed Show fixed Hide fixed
onnxscript/rewriter/pattern.py Fixed Show fixed Hide fixed
onnxscript/rewriter/pattern.py Fixed Show fixed Hide fixed
onnxscript/rewriter/pattern.py Fixed Show fixed Hide fixed
onnxscript/rewriter/pattern.py Fixed Show fixed Hide fixed
onnxscript/rewriter/pattern.py Fixed Show fixed Hide fixed
@gramalingam gramalingam self-assigned this Apr 30, 2024
@gramalingam gramalingam changed the title [WIP] Pattern IR cleanup Pattern IR cleanup Apr 30, 2024
@gramalingam gramalingam marked this pull request as ready for review April 30, 2024 23:43
@justinchuby justinchuby self-assigned this May 1, 2024
onnxscript/rewriter/pattern.py Outdated Show resolved Hide resolved
onnxscript/rewriter/pattern.py Show resolved Hide resolved
onnxscript/rewriter/pattern.py Show resolved Hide resolved
onnxscript/rewriter/pattern.py Show resolved Hide resolved
onnxscript/rewriter/pattern.py Outdated Show resolved Hide resolved
onnxscript/rewriter/pattern.py Show resolved Hide resolved
onnxscript/rewriter/pattern.py Outdated Show resolved Hide resolved
onnxscript/rewriter/pattern.py Show resolved Hide resolved
onnxscript/rewriter/pattern.py Show resolved Hide resolved
onnxscript/rewriter/pattern.py Outdated Show resolved Hide resolved
@titaiwangms titaiwangms self-requested a review May 2, 2024 19:30
Comment on lines +221 to +224
# if isinstance(x, list):
# if all(isinstance(i, (int, float)) for i in x):
# return Constant(x)
# raise ValueError("Only lists of int/float can be used as a ValuePattern")

Check notice

Code scanning / CodeQL

Commented-out code Note

This comment appears to contain commented-out code.
@gramalingam gramalingam merged commit a722e60 into main May 2, 2024
33 of 41 checks passed
@gramalingam gramalingam deleted the rama/refactor3 branch May 2, 2024 22:23
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