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

Add test case for unsupported input-variable reuse in multi-output pattern matcher #1731

Merged
merged 5 commits into from
Jul 16, 2024

Conversation

gramalingam
Copy link
Collaborator

The multi-output pattern does not support a useful case, where the pattern's input variables are reused elsewhere.

This PR adds a test-case illustrating the scenario. I was hoping that some changes in the in-progress PR #1636 by Xavier might fix this. But I don't think it does. Adding the test-case for now. Need to figure out how to support this.

Copy link

codecov bot commented Jul 16, 2024

Codecov Report

Attention: Patch coverage is 11.76471% with 15 lines in your changes missing coverage. Please review.

Project coverage is 74.83%. Comparing base (581e998) to head (268dc3b).

Files Patch % Lines
onnxscript/rewriter/generic_pattern_test.py 11.76% 15 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1731      +/-   ##
==========================================
- Coverage   74.88%   74.83%   -0.05%     
==========================================
  Files         242      242              
  Lines       25869    25886      +17     
  Branches     4665     4666       +1     
==========================================
+ Hits        19371    19373       +2     
- Misses       5628     5643      +15     
  Partials      870      870              

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

@@ -281,6 +281,42 @@ def apply_pattern(op, x, **_):
self.assertEqual(len(graph.node), 2)
self.assertEqual(graph.node[0].op_type, "SinCos")

@unittest.skip("Input variable reuse not supported yet")
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can xfail this so we know when it is fixed

Copy link

github-actions bot commented Jul 16, 2024

Test Results

     24 files  ±     0      24 suites  ±0   1h 49m 53s ⏱️ + 5m 30s
 13 870 tests +     1  10 560 ✅  -     1    3 300 💤 +     1  10 ❌ +1 
275 456 runs  +17 582  68 370 ✅ +2 567  207 068 💤 +15 014  18 ❌ +1 

For more details on these failures, see this check.

Results for commit 268dc3b. ± Comparison against base commit 581e998.

♻️ This comment has been updated with latest results.

@gramalingam gramalingam merged commit fb7dea4 into main Jul 16, 2024
28 of 43 checks passed
@gramalingam gramalingam deleted the rama/multi-pattern-input-reuse branch July 16, 2024 17:14
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.

2 participants