Make scale in of htex_auto_scale more effective #2196
Merged
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
benclifford
reviewed
Jan 20, 2022
parsl/dataflow/strategy.py
Outdated
| @@ -262,7 +262,10 @@ def _general_strategy(self, status_list, tasks, *, strategy_type): | |||
| logger.debug("More slots than tasks") | |||
| if isinstance(executor, HighThroughputExecutor): | |||
| if active_blocks > min_blocks: | |||
| exec_status.scale_in(1, force=False, max_idletime=self.max_idletime) | |||
| exec_status.scale_in( | |||
| active_blocks - active_tasks // tasks_per_node // nodes_per_block, | |||
There was a problem hiding this comment.
I think this calculation should look similar to the calculation around line 246: both of them arg calculating some kind of "target number of blocks" and it makes me uncomfortable that they don't look exactly the same (eg ceil, min and parallelism)
other than that, this looks like the right thing to be doing.
|
Adjusted the computation. Indeed I didn't take parallelism and min_blocks into the calculation. |
yadudoc
reviewed
Jan 20, 2022
parsl/dataflow/strategy.py
Outdated
| @@ -262,7 +262,10 @@ def _general_strategy(self, status_list, tasks, *, strategy_type): | |||
| logger.debug("More slots than tasks") | |||
| if isinstance(executor, HighThroughputExecutor): | |||
| if active_blocks > min_blocks: | |||
| exec_status.scale_in(1, force=False, max_idletime=self.max_idletime) | |||
| excess = math.ceil(active_slots - (active_tasks * parallelism)) | |||
There was a problem hiding this comment.
It might be clearer to rename the var excess to excess_slots.
There was a problem hiding this comment.
Do you also want excess renamed above, in line 245? It was using this term before.
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.
Description
For htex_auto_scale instead of removing only 1 block at most in case there are more slots than tasks, remove try to remove many blocks so that the number of slots matches the number of tasks. This is of course still limited by how many blocks have reached the configured max_idletime.
Fixes #2195
Type of change
Choose which options apply, and delete the ones which do not apply.