Refactor gather_dep #6388
Refactor gather_dep #6388
Conversation
crusaderky
force-pushed
the
WSMR/gather_dep
branch
from
8a3d7d9
to
270cb82
Compare
Unit Test Results 15 files + 15 15 suites +15 6h 54m 37s ⏱️ + 6h 54m 37s For more details on these failures and errors, see this check. Results for commit d985e6b. ± Comparison against base commit bde90af. ♻️ This comment has been updated with latest results. |
crusaderky
force-pushed
the
WSMR/gather_dep
branch
from
270cb82
to
d988cbe
Compare
crusaderky
force-pushed
the
WSMR/gather_dep
branch
from
d988cbe
to
43fef59
Compare
crusaderky
force-pushed
the
WSMR/gather_dep
branch
from
43fef59
to
6e7281c
Compare
crusaderky
force-pushed
the
WSMR/gather_dep
branch
from
6e7281c
to
08debe9
Compare
crusaderky
force-pushed
the
WSMR/gather_dep
branch
from
08debe9
to
5246162
Compare
crusaderky
force-pushed
the
WSMR/gather_dep
branch
from
5246162
to
442bf17
Compare
crusaderky
force-pushed
the
WSMR/gather_dep
branch
from
7df31ed
to
d985e6b
Compare
Unit Test ResultsSee test report for an extended history of previous test failures. This is useful for diagnosing flaky tests. 15 files + 15 15 suites +15 6h 32m 18s ⏱️ + 6h 32m 18s For more details on these failures, see this check. Results for commit de0adb0. ± Comparison against base commit bde90af. |
crusaderky
force-pushed
the
WSMR/gather_dep
branch
from
7b7024f
to
146467b
Compare
crusaderky
force-pushed
the
WSMR/gather_dep
branch
from
146467b
to
9128b8e
Compare
|
|
@fjetter Ready for review! |
| else: | ||
| refetch.add(ts) | ||
|
|
||
| return merge_recs_instructions( |
There was a problem hiding this comment.
Do you think we'll eventually be able to refactor all these merge_recs_instructions out, or is this here to stay because of the pattern of using the helper functions and ensure_communicating?
There was a problem hiding this comment.
It's here to stay. The alternative would be to pass recommendations, transitions to the helper functions and let them write back into them.
Unit Test ResultsSee test report for an extended history of previous test failures. This is useful for diagnosing flaky tests. 15 files + 15 15 suites +15 6h 27m 52s ⏱️ + 6h 27m 52s For more details on these failures and errors, see this check. Results for commit b31c3fc. ± Comparison against base commit 059798a. ♻️ This comment has been updated with latest results. |
Co-authored-by: Gabe Joseph <gjoseph92@gmail.com>
distributed/worker.py
Outdated
| def _refetch_missing_data( | ||
| self, worker: str, tasks: Iterable[TaskState], stimulus_id: str | ||
| ) -> RecsInstrs: |
There was a problem hiding this comment.
I strongly dislike the idea of reusing this code. This "helper" already shows signs of too much complexity, we're dealing with many individual ts.state values to make a decision. If we restrict ourselves to the specialized invocation this should be much less complex.
Reusing code was one of the major reasons why the except/except/finally block caused so many problems.
Particularly with the MissingDataMsg singal in here (#6445) I do not trust this to be the correct answer for both success-but-missing and network-failure responses, see also #6112 (comment)
distributed/worker.py
Outdated
| refetch = set(self._gather_dep_done_common(ev)) | ||
| refetch |= {self.tasks[key] for key in self.has_what[ev.worker]} |
There was a problem hiding this comment.
I don't think these two sets should be mixed, see my earlier comment about the shared "helper" function.
ensure_communicatingtransitions to new WorkerState event mechanism #5896Changes in transition logic
Simplify transitions both in case of missing keys in a successful response and in case of network failure.
This will slightly delay transitions to missing, but it allows for much cleaner and simpler code.