Improving the scheduling logic of the local engine #185
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dthulke
changed the title
|
Good point! It bothers me a little that this now changes the order of the queue if it finds a job which needs less resources. It would be nice to keep the FIFO property. You could just pull everything out of the Queue and store it in a normal list. Since the LocalEngine has only one thread checking for new tasks it shouldn't be a problem that a normal list isn't thread safe like the Queue. You can than simply iterate over the list and remove submitted jobs from it. |
dthulke
force-pushed
the
localengine-scheduling
branch
from
ef5e080 to
eb77402
Compare
dthulke
force-pushed
the
localengine-scheduling
branch
from
eb77402 to
43470da
Compare
|
Good point, changed it to a normal list (wrapped in sync_object to be safe). |
michelwi
approved these changes
Apr 22, 2024
critias
reviewed
Apr 22, 2024
sisyphus/localengine.py
Outdated
|
|
||
| # run next task if the capacities are available | ||
| if next_task is not None: | ||
| while runnable_task_idx < total_runnable_tasks: |
There was a problem hiding this comment.
Why not use len(runnable_tasks) directly?
There was a problem hiding this comment.
Good question :D now I also don't see a reason to introduce the extra variable.
critias
approved these changes
Apr 24, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In the current implementation the LocalEngine always tries to schedule the first runnable task in its input queue (fifo). In cases where this jobs requires resources that are currently not available (e.g. a GPU), the scheduler waits till the resources are available even if there are smaller jobs that could be scheduled now.
As a very simple fix, with this change, if a task cannot be scheduled it is moved to the end of the queue and the next task is checked.