Fixes TypeError and infinite looping in MPITaskScheduler #3783
Conversation
yadudoc
force-pushed
the
mpi_sched_fix
branch
from
6715018 to
f399687
Compare
* test_larger_jobs_prioritized checks to confirm the ordering of jobs in the backlog queue * test_hashable_backlog_queue tests to confirm that the PrioritizedTask dataclass avoid the priority queue failing to hash tasks with the same priority. * an extended test for new MPITaskScheduler logic
…g logic * `schedule_backlog_tasks` is now updated to fetch all tasks in the backlog_queue and then attempt to schedule them avoiding the infinite loop. * A new `PrioritizedTask` dataclass is added that disable comparison on the task: dict element. * The priority is set num_nodes * -1 to ensure that larger jobs get prioritized.
yadudoc
force-pushed
the
mpi_sched_fix
branch
from
6a4b1a1 to
22a4ce0
Compare
yadudoc
changed the title
| # Separate fetching tasks from the _backlog_queue and scheduling them | ||
| # since tasks that failed to schedule will be pushed to the _backlog_queue | ||
| backlogged_tasks = [] | ||
| while not self._backlog_queue.empty(): | ||
| prioritized_task = self._backlog_queue.get(block=False) | ||
| backlogged_tasks.append(prioritized_task.task) | ||
|
|
||
| for backlogged_task in backlogged_tasks: | ||
| self.put_task(backlogged_task) | ||
|
|
There was a problem hiding this comment.
From a static analysis, this looks better to me. No more infinite loop potential, but I do observe that this could mean a lot of unpacking and then repacking. "It works," so I'm not going to fuss about it, but a different data structure might help with that.
More actionably, however, this looks like it would lose tasks? What happens when .get(block=False) raises queue.Empty?
There was a problem hiding this comment.
Good catch, Kevin! These few lines do make me worry about race conditions.
Additionally, will the very aggressive scheduling here (always attempt to schedule everything) will still result in large tasks being continually delayed? If there are small tasks, they'll get scheduled before the big one still.
That might be ok with some users, but what about a simple "run in the order of execution" strategy as our baseline?
There was a problem hiding this comment.
(we shouldn't overtrivialise this or assume there's a universal solution or try to make a comprehensive set of here are the options that will satisfy everyone)
There was a problem hiding this comment.
@khk-globus Thanks for the review, this is a good catch!
- Unpacking-repacking: Yep, we shouldn't have to do this if we store the resource_spec
queue.Empty: I was working with the idea that since only this function can pop an item from the queue, checking forempty()is sufficient to guarantee thatgetwill not raise aqueue.Empty. I can rework this to avoid this.
@WardLT I share your concern. There's no notion of fairness here, and as @benclifford pointed out coming up with scheduling logic that'll work for everyone is hard. Right now, I expect larger tasks to end up getting delayed. Like @benclifford mentioned we could move this logic to the interchange (#3323), but we still need to implement these alternative scheduling algorithms but I'm hesitant to do so without user feedback.
|
there's enough interesting stuff here that the "if your PR description is an itemised list, there should be one PR per item" rule probably applies. |
|
@benclifford Your comment on splitting the PR is fair, I can get that sorted. |
…l with `TypeError` (#3794) # Description The `MPITaskScheduler` uses Python's PriorityQueue to prioritize tasks based on the number of nodes requested. When items with identical priorities are are submitted to the PriorityQueue, they attempt to sort based on the task dict which fails with TypeError unhashable type: dict. This PR adds a new `PrioritizedTask` dataclass that sets the task element to `field(compare=False)`. I'm splitting changes in #3783 to keep the PR concise. This is split 1 of 3. # Changed Behaviour Fixes the bug described above. ## Type of change Choose which options apply, and delete the ones which do not apply. - Bug fix
Description
This PR attempts to fix the following bugs in the MPITaskScheduler:
schedule_backlog_tasksmethod takes tasks from the backlog and attempts to schedule them until the queue is empty. However since calling put_task pops the task back onto the backlog queue, this ends up in an infinite loop if there's at least 1 task that cannot be scheduled.TypeErrorunhashable type: dict.Changed Behaviour
Fixes
schedule_backlog_tasksis now updated to fetch all tasks in the backlog_queue and then attempt to schedule them avoiding the infinite loop.PrioritizedTaskdataclass is added that disable comparison on thetask: dictelement.num_nodes * -1to ensure that larger jobs get prioritized.Type of change
Choose which options apply, and delete the ones which do not apply.