-
Notifications
You must be signed in to change notification settings - Fork 143
Fix FusionOptimizer bug #1630
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
Fix FusionOptimizer bug #1630
Conversation
92595ba
to
7c6eeb4
Compare
7c6eeb4
to
ecc5d23
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I approved too fast, it looks like you broke scan :D
Oh well we don't lose much anyway |
ecc5d23
to
9ff40d0
Compare
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (94.11%) is below the target coverage (100.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #1630 +/- ##
==========================================
- Coverage 81.67% 81.67% -0.01%
==========================================
Files 232 232
Lines 53132 53132
Branches 9410 9407 -3
==========================================
- Hits 43396 43395 -1
- Misses 7283 7284 +1
Partials 2453 2453
🚀 New features to boost your workflow:
|
good news: it works now. bad news: mypy ... |
continue | ||
|
||
# Finished exploring this subgraph | ||
all_subgraphs_bitset |= subgraph_bitset |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was what was causing the scan tests to fail. If we add the subgraph_bitset of failed nodes to all_subgraphs_bitset it then causes a failure in the sorting of the subgraphs below, because it was trying to sort wrt to these ones that don't matter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was the original logic of including it? Or was it just a mistake
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
original this variable was meant more as a collection of "variable that were claimed or known to not be fuseable", so it would fulfill the second goal. But that info was not used anywhere in the latest iteration. Instead we only use this for the sorting bit at the end.
): | ||
break # bingo | ||
else: # no-break | ||
raise RuntimeError( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would have raised in the failing scan test, showing our sorting failed
When a subgraph with multiple outputs is "implicitly" claimed, it can change the dependencies of remaining nodes. A node that depended only on a subset of the subgraph outputs now depends on all of them. Not taking this into account could lead to circular dependent Composites
9ff40d0
to
ca797da
Compare
First commit is just small refactor, second fixes the issue.
Reported by @jessegrabowski, where pymc model would hang before sampling. We need to update the ancestors_bitset when we implicitly "claim" a subgraph.