Don't consider scheduler idle while executing Scheduler.update_graph
#8877
Conversation
Unit Test ResultsSee test report for an extended history of previous test failures. This is useful for diagnosing flaky tests. 25 files ± 0 25 suites ±0 10h 22m 24s ⏱️ - 3m 32s For more details on these failures, see this check. Results for commit 40f3a35. ± Comparison against base commit 80b3af5. ♻️ This comment has been updated with latest results. |
hendrikmakait
changed the title
Scheduler.update_graphScheduler.update_graph
There was a problem hiding this comment.
Nice, thanks @hendrikmakait. I think this will help when dealing with large graphs
There was a problem hiding this comment.
Thanks @hendrikmakait, this LGTM. I left a couple of non-blocking comments. Feel free to include or ignore.
| @@ -4913,8 +4916,11 @@ async def update_graph( | |||
| # (which may not have been added to who_wants yet) | |||
| client=client, | |||
| ) | |||
| end = time() | |||
| self.digest_metric("update-graph-duration", end - start) | |||
| finally: | |||
There was a problem hiding this comment.
Just noting that this finally block is being used to ensure we always decrement the _active_graph_updates counter
| class UpdateGraphTrackerPlugin(SchedulerPlugin): | ||
| def start(self, scheduler): | ||
| self.scheduler = scheduler | ||
| self.idle_during_update_graph = None |
There was a problem hiding this comment.
[Non-blocking nit] This is totally fine as is but maybe don't even assign self.idle_during_update_graph until inside the update_graph plugin hook? assert plugin.idle_during_update_graph is None could mean that the update_graph hook didn't run at all.
| self.idle_during_update_graph = None |
There was a problem hiding this comment.
I've slightly adjusted this such that idle_during_update_graph is initialized to None and set to True or False during the hook.
Co-authored-by: James Bourbeau <[email protected]>
Co-authored-by: James Bourbeau <[email protected]>
Partially addresses #8876
pre-commit run --all-files