Client story based on #6095#1
Conversation
| (f.key, "executing", "memory", "memory", {}), | ||
| ], | ||
| ordered_timestamps=False, | ||
| ) |
There was a problem hiding this comment.
@mrocklin Here is the test case including both scheduler and worker events. Testing of stimuli remains to be added.
There was a problem hiding this comment.
In general I dislike the assert_story tests. They seem really brittle to me. This also doesn say to me "this closes the circle". Does it say that to you? If we failed in closing the circle is there something that we would immediately notice without having to understand this code exactly?
There was a problem hiding this comment.
Or maybe I should ask "what does it mean to close the circle?" Can we define that somehow? Currently this test says to me "this task transitions from released to waiting and then to processing and then to memory and then to compute-task and then to waiting and then to ready and then to executing and then to memory" which doesn't actually tell me much. I view tests like these as more of a liability (I'm afraid that I'll have to spend a lot of time in the future to understand it when touching some other part of the code) rather than an asset (something that would tell me "this closes the circle").
There was a problem hiding this comment.
Ah, the confusion might be that I haven't pushed up the stimulus_id comparisons. Just testing locally.
Would that close the circle do you think?
There was a problem hiding this comment.
Do you think that it closes the circle? If so, why? How would you describe it to someone who didn't intimately know the internals of the system?
There was a problem hiding this comment.
For example, to me it means that we can trace any event downstream to a root stimulus. By "trace" I mean that I can connect stimulus IDs to other stimulus IDs until I get back to some root source.
I don't care about the values of those stimulus IDs (and so I wouldn't want them in the test) but I would want to create some function like
def can_trace_to_source(...):
...And apply it to various events and verify that they cross from the scheduler and worker coherently. I absolutely wouldn't want to test the exact story that they tell (because this is certainly going to change over the next few weeks, and I want my test to provide value for more than a few weeks).
I don't know stimulus IDs well enough to say that this is correct though. I'm hoping that you do. I think that we need to lift up a little bit from highly specific "this is exactly what happened" tests to something that tests a broader invariant.
I'm also happy to switch to a live call tomorrow if that's easier. I suspect that we're speaking past each other a little here.
There was a problem hiding this comment.
And apply it to various events and verify that they cross from the scheduler and worker coherently.
I'm also happy to switch to a live call tomorrow if that's easier. I suspect that we're speaking past each other a little here.
Thanks, I'd appreciate that, I've been mostly testing with the existing story utilities in mind. It'll also help me to think about your suggestion with a fresh head.
Codecov Report
@@ Coverage Diff @@
## stimulus-id-kwargs #1 +/- ##
======================================================
+ Coverage 91.92% 92.32% +0.40%
======================================================
Files 130 130
Lines 24192 24214 +22
======================================================
+ Hits 22238 22356 +118
+ Misses 1954 1858 -96
Continue to review full report at Codecov.
|
9cc7fa5 to
aab88b4
Compare
|
Merged this into dask#6095 |
No description provided.