Long running occupancy #5395
Long running occupancy #5395
Conversation
fjetter
force-pushed
the
long_running_occupancy
branch
from
e1c1ee9 to
c15051c
Compare
fjetter
force-pushed
the
long_running_occupancy
branch
from
486719f to
ab517ea
Compare
gjoseph92
left a comment
gjoseph92
left a comment
There was a problem hiding this comment.
LGTM, though I'm a little curious why the task needs to stay in processing on the WorkerState
|
|
||
| @ccall | ||
| @exceptval(check=False) | ||
| def _reevaluate_occupancy_worker(self, ws: WorkerState): |
There was a problem hiding this comment.
What was the reason for moving this to be a method instead of a standalone function? I agree methods are more readable, just not sure about the Cython consequences cc @jakirkham
There was a problem hiding this comment.
Both will work. However there was extra overhead from the method call vs. the function call, which is why it was moved to a function. Given this gets called a lot, that overhead mattered in our benchmarks.
fjetter
force-pushed
the
long_running_occupancy
branch
from
ab517ea to
a151a4b
Compare
| self.scheduler._reevaluate_occupancy_worker(thief) | ||
| self.scheduler._reevaluate_occupancy_worker(victim) |
There was a problem hiding this comment.
Since it looks like _reevaluate_occupancy_worker was removed (even as a method), should we be doing something else here?
There was a problem hiding this comment.
Ahh, I missed this one. I think this is why I put it in as a mathod to avoid cyclic imports
There was a problem hiding this comment.
FWIW did like the idea you had of getting rid of this function if that is an option. Though admittedly there might need to be more thought on this test
There was a problem hiding this comment.
Intuitively, it feels like this should not even be here since this interacts pretty deeply with the scheduler and the stealing thing should be an extension and not interact on this deep level. However, the only way to not do this here would be to solely rely on the eventual update via a callback. No idea how disruptive this would be
fjetter
force-pushed
the
long_running_occupancy
branch
from
0eda3c9 to
a151a4b
Compare
3 participants
We're removing long running tasks from a workers occupancy. However, occasionally we're recalculating the worker occupancy to account for new information since the occupancy gets inaccurate over time. However, this reevaluation does not take into account
long-runningtasks since the scheduler does not further break the stateprocessingdown. Similar for the substateexecuting, we're tracking this on a per-worker base and we should do the same for thelong-runningsubstateThis builds on top of #5392 since I pulled together the occupancy calculation over there.
Fixes of this PR in last commit 9c20b32
pre-commit run --all-files