AssertionError: waiting on released dep when worker-saturation < .inf #7204

Activity

1. crusaderky

   added

   scheduler

   on Oct 27, 2022
2. crusaderky

   assigned

   gjoseph92

   on Oct 27, 2022
3. crusaderky

   added

   bugSomething is brokenSomething is broken

   on Oct 27, 2022
4. gjoseph92 commented on Oct 27, 2022

   

   gjoseph92

   on Oct 27, 2022

   Collaborator

   More actions

   I think this failure is a red herring. I don't think there's actually any issue here.

   The test in #7065 is not appropriate to run with queuing on. The test asserts many things that are specific to no-queuing co-assignment.

   What happens is simply that this assertion fails with queuing on (since there's obviously no last worker):

   distributed/distributed/tests/test_scheduler.py

   Line 299 in 06fff7c

   assert s.tasks["y-2"].group.last_worker == s.workers[a.address]

   Then the test hangs for a while because it's trying to clean up the cluster, but the workers' threadpools are all busy, so eventually it just times out. I don't think the validation errors you're seeing here are real problems, I think they're artifacts of an ugly cluster shutdown.

   I haven't been able to come up with a way to formulate this test into something that would be meaningful with queuing. This test is relying on a couple of bad behaviors:

   1. The three x tasks aren't considered root-ish, even though they don't have dependencies, because their task group is too small. So they won't be queued, even though it would be appropriate to queue them.

   2. The y tasks are considered root-ish because of their naming, but structurally, they're really not root-ish tasks Fix decide_worker_rootish_queuing_disabled assert #7065 (comment). They should be scheduled with regard to their dependencies and not go through the root task logic at all. Root-ish tasks should, logically, ignore the location of their dependencies when scheduling (if their dependencies mattered, they wouldn't be root-ish). So when queuing is on, they'll be scheduled on any worker.

      That nullifies the core thing this test is trying to test: what if a task should be scheduled onto a particular worker because its dependencies are there, but that worker is closing_gracefully? Well, with queuing on, there's no concept of what worker it "should" be scheduled on, because dependencies are ignored. Additionally, the main thing this test was trying to capture is when last_used_worker is now closing_gracefully. That's irrelevant with queuing, since there's no statefulness and no last_used_worker.

   We already have some queueing tests around paused workers. Maybe we should add one for retiring workers too, just to be safe, though it should be the same code path. But I don't think there's a bug here; instead, we should just make the test in #7065 only run when queuing is turned off.
5. gjoseph92

   closed this as not plannedon Oct 27, 2022
6. crusaderky commented on Oct 28, 2022

   

   crusaderky

   on Oct 28, 2022

   CollaboratorAuthor

   More actions

   The three x tasks aren't considered root-ish, even though they don't have dependencies, because their task group is too small. So they won't be queued, even though it would be appropriate to queue them.

   By the current definition of rootish, the only way a rootish group could depend on another rootish group would be to have exactly 1 worker with 1 thread:

   • must have at least total_nthreads * 2 + 1 tasks
   • must have at most 4 dependencies

   the above cannot be satisfied for both dependent and dependency already with total_nthreads=2.
7. gjoseph92 commented on Oct 28, 2022

   

   gjoseph92

   on Oct 28, 2022

   Collaborator

   More actions

   Correct. I'm saying the current definition of rootish is a bit silly because something that is definitively a root task (has no dependencies) is not considered rootish. And something that shouldn't be considered rootish (the y tasks in your graph) is.