Skip to content

Client story based on #6095#1

Merged
sjperkins merged 5 commits intostimulus-id-kwargsfrom
client-story-6095
Apr 13, 2022
Merged

Client story based on #6095#1
sjperkins merged 5 commits intostimulus-id-kwargsfrom
client-story-6095

Conversation

@sjperkins
Copy link
Owner

No description provided.

(f.key, "executing", "memory", "memory", {}),
],
ordered_timestamps=False,
)
Copy link
Owner Author

Choose a reason for hiding this comment

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

@mrocklin Here is the test case including both scheduler and worker events. Testing of stimuli remains to be added.

Choose a reason for hiding this comment

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

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?

Choose a reason for hiding this comment

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

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").

Copy link
Owner Author

Choose a reason for hiding this comment

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

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?

Choose a reason for hiding this comment

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

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?

Choose a reason for hiding this comment

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

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.

Copy link
Owner Author

Choose a reason for hiding this comment

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

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-commenter
Copy link

Codecov Report

Merging #1 (739470a) into stimulus-id-kwargs (0a0f4bc) will increase coverage by 0.40%.
The diff coverage is 100.00%.

@@                  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     
Impacted Files Coverage Δ
distributed/client.py 94.65% <100.00%> (+0.38%) ⬆️
distributed/scheduler.py 94.05% <100.00%> (-0.13%) ⬇️
distributed/utils_comm.py 98.28% <100.00%> (+0.23%) ⬆️
distributed/utils_test.py 88.96% <100.00%> (+0.20%) ⬆️
distributed/worker.py 93.75% <100.00%> (-0.04%) ⬇️
distributed/comm/ws.py 92.85% <0.00%> (-0.80%) ⬇️
distributed/utils.py 87.58% <0.00%> (-0.27%) ⬇️
... and 23 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0a0f4bc...739470a. Read the comment docs.

@sjperkins sjperkins merged commit 746b569 into stimulus-id-kwargs Apr 13, 2022
@sjperkins
Copy link
Owner Author

sjperkins commented Apr 13, 2022

Merged this into dask#6095

@sjperkins sjperkins deleted the client-story-6095 branch April 13, 2022 10:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants