Reduce size of scheduler_info #9045
Conversation
| @@ -1778,6 +1778,7 @@ def __init__( | |||
| self.task_groups = {} | |||
| self.task_prefixes = {} | |||
| self.task_metadata = {} | |||
| self.total_memory = 0 | |||
There was a problem hiding this comment.
It's almost overkill to maintain additional state for this but if we have to call identity often I want it to be constant time
There was a problem hiding this comment.
Also, I'm surprised we are not tracking this already. Seems like a useful metric for adaptive scaling or smth like that 🤷
| @@ -4075,16 +4076,22 @@ def _repr_html_(self) -> str: | |||
| tasks=self.tasks, | |||
| ) | |||
|
|
|||
| def identity(self) -> dict[str, Any]: | |||
| def identity(self, n_workers: int = -1) -> dict[str, Any]: | |||
There was a problem hiding this comment.
the default of -1 is to maintain backwards compat (no idea who is calling this in the wild). All callers in this code base are setting this explicitly
There was a problem hiding this comment.
Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.
Files not reviewed (1)
- distributed/widgets/templates/scheduler_info.html.j2: Language not supported
Comments suppressed due to low confidence (1)
distributed/client.py:4412
- [nitpick] There are varying conventions for n_workers across client calls (e.g. 0 in dashboard_link, 5 in scheduler_info, -1 for full info). Consider clarifying/documenting the intent behind each value to avoid confusion.
def scheduler_info(self, n_workers: int = 5, **kwargs: Any) -> SchedulerInfo:
| @@ -1298,7 +1298,7 @@ def dashboard_link(self): | |||
| try: | |||
| return self.cluster.dashboard_link | |||
| except AttributeError: | |||
| scheduler, info = self._get_scheduler_info() | |||
| scheduler, info = self._get_scheduler_info(n_workers=0) | |||
There was a problem hiding this comment.
Using n_workers=0 in this context results in an empty workers list. If the intention is to provide a default view for diagnostics, consider using a value such as -1 (to fetch all) or the same default limit (e.g. 5) used elsewhere.
| scheduler, info = self._get_scheduler_info(n_workers=0) | |
| scheduler, info = self._get_scheduler_info(n_workers=-1) |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
that's intentional since the workers info is not even accessed here
fjetter
force-pushed
the
reduce_size_scheduler_info
branch
from
b0e37f3 to
665431f
Compare
Unit Test ResultsSee test report for an extended history of previous test failures. This is useful for diagnosing flaky tests. 27 files ±0 27 suites ±0 11h 17m 19s ⏱️ + 1m 9s For more details on these failures and errors, see this check. Results for commit 41ce938. ± Comparison against base commit 16aa189. ♻️ This comment has been updated with latest results. |
|
I'll move forward with merging. Test failures appear to be unrelated and I think the benefits of this outweight possible minor UX things. I'm happy to follow up on this if there is feedback but I'd like to get the functional fix into the release today. |
See #9043
The scheduler_info is used for this view
I am introducing a limit to the number of workers being returned.
I cannot imagine that somebody is using this for actual diagnostics (but it's cute and we should leave it) and I just randomly hard coded this to
5. With this change the total numbers in the upper right corner is still the max and indicates that the below view is only a subset. I'm open to suggestions for making this prettier but that has only low priority.cc @jacobtomlinson I think you introduced this HTML view and might have an opinion about details or the hard coded number, etc.