Review annotations and docstrings in scheduler.py, part 1 #6132
Conversation
| .. attribute:: client_key: str | ||
| #: A unique identifier for this client. This is generally an opaque | ||
| #: string generated by the client itself. | ||
| client_key: str |
There was a problem hiding this comment.
I added :members: to worker-memory.rst. This means that attributes, methods, and properties that have a docstring and don't start with _ are now automatically documented.
| @@ -196,21 +195,26 @@ def _to_dict_no_nest(self, *, exclude: "Container[str]" = ()) -> dict: | |||
|
|
|||
| class MemoryState: | |||
| """Memory readings on a worker or on the whole cluster. | |||
| See :doc:`worker-memory`. | |||
There was a problem hiding this comment.
This class is now documented in Sphinx. It was already referenced in public docs in several places.
There was a problem hiding this comment.
Historically we've kept a newline between the title line of a docstring and the body. Thoughts on this? Any objection to continuing with it?
I don't care so much about this particular instance (please don't block on a comment this small), but more broadly I'd love to align on this if it's easy.
| will run them eventually, depending on its execution resources | ||
| (but see :doc:`work-stealing`). | ||
| local_directory: str | ||
| services: dict[str, int] |
There was a problem hiding this comment.
These attributes were undocumented before and remain it now.
Note that our definition of "public" is "documented in Sphinx". This makes these attributes implicitly private.
There should be a proper pass later on where we document these attributes and decide wether we want to prepend them with _ to hide them from sphinx again - and make it clear that they're private.
| #: History of the last 30 seconds' worth of unmanaged memory. Used to differentiate | ||
| #: between "old" and "new" unmanaged memory. | ||
| #: Format: ``[(timestamp, bytes), (timestamp, bytes), ...]`` | ||
| _memory_unmanaged_history: deque[tuple[float, int]] |
There was a problem hiding this comment.
These two attributes were undocumented (and thus private before), and I felt they should remain private, so I prepended them with _.
| @@ -569,7 +584,7 @@ def identity(self) -> dict: | |||
| **self.extra, | |||
| } | |||
|
|
|||
| def _to_dict_no_nest(self, *, exclude: "Container[str]" = ()) -> dict: | |||
| def _to_dict_no_nest(self, *, exclude: Container[str] = ()) -> dict[str, Any]: | |||
There was a problem hiding this comment.
The thorough review ends here. Beyond this point it's either auto-tweaks by pyupgrade or fixes to keep things working.
Unit Test Results 16 files + 4 16 suites +4 7h 20m 21s ⏱️ + 1h 50m 24s For more details on these failures, see this check. Results for commit 380a1cc. ± Comparison against base commit 2ab4438. ♻️ This comment has been updated with latest results. |
| @@ -196,21 +195,26 @@ def _to_dict_no_nest(self, *, exclude: "Container[str]" = ()) -> dict: | |||
|
|
|||
| class MemoryState: | |||
| """Memory readings on a worker or on the whole cluster. | |||
| See :doc:`worker-memory`. | |||
There was a problem hiding this comment.
Historically we've kept a newline between the title line of a docstring and the body. Thoughts on this? Any objection to continuing with it?
I don't care so much about this particular instance (please don't block on a comment this small), but more broadly I'd love to align on this if it's easy.
|
|
||
| This worker's unique key. This can be its connected address | ||
| (such as ``'tcp://127.0.0.1:8891'``) or an alias (such as ``'alice'``). | ||
| """A simple object holding information about a worker.""" |
There was a problem hiding this comment.
Hrm, we're losing most of the docstring here, which I'm a little sad about (although not too sad). I guess the reason not to have a solid docstring here is because then we're keeping two copies of the attributes and types around (and honestly we're probably going to struggle to do even that). I guess that this makes sense.
There was a problem hiding this comment.
The Sphinx documentation is still fully generated for the attributes:
https://distributed--6132.org.readthedocs.build/en/6132/scheduling-state.html#worker-state
I found many examples of the docstring falling out of sync with the attributes - I find keeping the documentation of the attribute together with its annotation much more robust. It also lets you spot on the fly the undocumented ones.
|
|
||
| @property | ||
| def has_what(self) -> "Set[TaskState]": | ||
| def has_what(self) -> Set[TaskState]: |
| @@ -1704,7 +1719,7 @@ def _transition(self, key, finish: str, *args, **kwargs): | |||
|
|
|||
| finish2 = ts._state | |||
| # FIXME downcast antipattern | |||
| scheduler = pep484_cast(Scheduler, self) | |||
| scheduler = cast(Scheduler, self) | |||
There was a problem hiding this comment.
I'm curious why this is necessary
There was a problem hiding this comment.
transition_log and validate are attributes/methods of Scheduler, which are invoked from methods of SchedulerState which is a parent class.
There was a problem hiding this comment.
Now that we're no longer doing Cythonization, we should merge these two. Presumably this goes away then 🎉
No problem, fixed |
Follow-up to #6104
Incomplete work (I reached WorkerState, included). It can be merged as-is; more PRs will follow when time allows.