Review scheduler annotations, part 2 #6253
Conversation
| self._state: str = None # type: ignore | ||
| self.exception: str = None # type: ignore | ||
| self.exception_blame: TaskState = None # type: ignore | ||
| self._state = None # type: ignore |
There was a problem hiding this comment.
The cleanup of these non-None attributes forced to None is left to a future PR
| type(ws), | ||
| ws, | ||
| ) | ||
| if self.validate and ws is not None: | ||
| assert ws.address in self.workers |
There was a problem hiding this comment.
The top-level function decide_workers NEVER returns None in the unit tests.
Follow-up: #6254
|
All failures are unrelated |
crusaderky
force-pushed
the
scheduler_annotations
branch
from
df1cee3 to
a4ae52a
Compare
There was a problem hiding this comment.
Overall LGTM. I think there are a few more annotations we can drop while we're at it, but feel free to ignore these (as well as my nit-picking and meta-comment):
distributed/distributed/scheduler.py
Line 1133 in 2e9971d
distributed/distributed/scheduler.py
Line 7194 in 2e9971d
distributed/distributed/scheduler.py
Line 7266 in 2e9971d
| if finish == "memory" or finish == "erred": | ||
| ts: TaskState = self.scheduler.tasks.get(key) | ||
| ts = self.scheduler.tasks.get(key) |
There was a problem hiding this comment.
Just a comment outside of the scope of this review: ts appears to be a common abbreviation for task_state. When I hear ts I automatically think timestamp, I'm not sure if anybody else has this problem and if it creates friction. If so, it might be worth thinking about renaming ts variables to task_state.
There was a problem hiding this comment.
Yes, we ubiquitously intend ts as TaskState and ws as WorkerState
"ts" as timestamp should be avoided in distributed.
There was a problem hiding this comment.
Alright, so it'll just take some getting used to :)
|
|
||
| @property | ||
| def state(self) -> str: | ||
| """This task's current state. Valid states include ``released``, ``waiting``, |
There was a problem hiding this comment.
Thanks for this docstring! Without it, it has been extremely hard for me to understand what possible states a TaskState could be in.
There was a problem hiding this comment.
I'm going to replace str with a TaskStateState Literal in a later PR, like it's already done in worker_state_machine.py
Co-authored-by: Hendrik Makait <hendrik.makait@gmail.com>
Continues from #6132
Out of scope:
TaskState.__init__