Add a DagOptimizer test #745
Conversation
|
@ttim can you take a look? |
Codecov Report
@@ Coverage Diff @@
## develop #745 +/- ##
==========================================
+ Coverage 72.23% 72.34% +0.1%
==========================================
Files 154 154
Lines 3742 3742
Branches 209 209
==========================================
+ Hits 2703 2707 +4
+ Misses 1039 1035 -4
Continue to review full report at Codecov.
|
|
okay, this now fails for me (both the fanOut and the idempotency test). cc @non |
|
actually, I can't get idempotency to fail now... maybe it is just fanOut being broken being the issue, which some of the rules use... |
|
Okay, I don't see a bug actually. It was an issue with the test actually, and an inconsistency between how fanOut was defined in Dependants and ExpressionDag. This test actually seems to pass for me now. I'll try to find any counter example next week, but so far, I guess everything looks correct still. |
|
okay, idempotency failure: I'll try to reproduce that failure and fix. |
There was a subtle bug in ExpressionDag.fanOut which caused non-idempotency, and underapplication of rules in some cases. The problem was in computing fanOut in Id space, where there is actually no 1:1 function between Ids and N nodes. The solution is to compute fanOut directly in N space which is what is meaningful anyway. This seems to fix the bug even with rather large numbers of trials
|
This seems to fix the issue. The bug was with fanOut. It was working in the If you jump back to the Along the way I cleaned things up slightly. Can you take a look @ttim |
|
had a storm flake. Restarted a 2.12 build. |
|
I'd like to cherry pick this when we merge and publish a version of Also, I'm considering breaking this code out into a standalone library. I copied it into scalding, but it is inconvenient to have to publish these large projects to update this totally generic DAG rewriting tool. |
|
|
||
| implicit val generatorDrivenConfig = | ||
| PropertyCheckConfig(minSuccessful = 1000, maxDiscarded = 1000) // the producer generator uses filter, I think | ||
| //PropertyCheckConfig(minSuccessful = 100, maxDiscarded = 1000) // the producer generator uses filter, I think |
|
|
||
| } | ||
|
|
||
| test("test some idempotency specific past failures") { |
There was a problem hiding this comment.
maybe nice to add a comment above on the details of the past failure? might be hard for folks reading the code to know?
There was a problem hiding this comment.
I don't know what to say. This was a hand minimized example (it took me about an hour) from a failure case found by scalacheck. Since the failures were quite rare, it took a long time to even find a failure with scalacheck, so once I found it, I wanted to test that failure every time.
That's what I mean by "specific past failures".
I understand (somewhat) why this failed now, but I couldn't easily generate another that would also show the bug.
Can you suggest some specific text you would like to see me add?
There was a problem hiding this comment.
Yeah I guess reading the title it doesn't give you a sense of what the issue is and what it's testing. Would it make sense to add a tldr of your understanding of why it failed?
|
|
We are using the DagOptimizer at stripe before planning to reduce the size of some online graphs (went from 115 storm bolts or so to 69 in one example).
However, even in the case where we reach 69, there are rules that don't seem to be fully applied and I have not yet found out why.
Anyway, more test coverage never hurts.