-
Notifications
You must be signed in to change notification settings - Fork 194
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add BlockIdManagerSelector #3560
base: master
Are you sure you want to change the base?
Changes from 1 commit
d2a1988
f4e04d8
1f16dcb
e914493
d745670
705cf99
85555fa
e5456bf
85ce33a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,6 +19,14 @@ def sort_managers(self, ready_managers: Dict[bytes, ManagerRecord], manager_list | |
|
||
class RandomManagerSelector(ManagerSelector): | ||
|
||
"""Returns a shuffled list of interesting_managers | ||
|
||
Maintains the behavior of the original interchange. Works well | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the docs should primarily be about the version now, rather than about changes since some other version the user isn't using, so you could get rid of this first sentence. a related thing to express would be that this is (I think) the default - see how some other docstrings on HighThroughputExecutor talk about defaults There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ... but the commit message that this gets merged under (which hopefully will come from the description text of this PR) is the right place to describe how the new code is different/similar to the old code - so describing how the default behaviour now is the same or different to the previous behaviour before this is merged is relevant there: the audience for that is people who want to know what has changed in parsl, vs audience for the user guide is what is stuff like now There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reworded to identify the random manager selector as the default |
||
in distributing workloads equally across all availble compute | ||
resources. Is not effective in conjunction with elastic scaling | ||
behavior as it leads to wasted resource consumption. | ||
""" | ||
|
||
def sort_managers(self, ready_managers: Dict[bytes, ManagerRecord], manager_list: Set[bytes]) -> List[bytes]: | ||
c_manager_list = list(manager_list) | ||
random.shuffle(c_manager_list) | ||
|
@@ -27,5 +35,19 @@ def sort_managers(self, ready_managers: Dict[bytes, ManagerRecord], manager_list | |
|
||
class BlockIdManagerSelector(ManagerSelector): | ||
|
||
"""Returns a interesting_managers list sorted by block ID | ||
|
||
Observations: | ||
1. BlockID manager selector helps with workloads that see a varying | ||
amount of tasks over time. We see new blocks are prioritized with the | ||
blockID manager selector, when used with 'htex_auto_scaling', results | ||
in compute cost savings. | ||
|
||
2. Doesn't really work with bag-of-tasks workloads. When all the tasks | ||
are put into the queue upfront, all blocks operate at near full | ||
utilization for the majority of the workload, which task goes where | ||
doesn't really matter. | ||
""" | ||
|
||
def sort_managers(self, ready_managers: Dict[bytes, ManagerRecord], manager_list: Set[bytes]) -> List[bytes]: | ||
return sorted(manager_list, key=lambda x: (ready_managers[x]['block_id'] is not None, ready_managers[x]['block_id']), reverse=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you could call this Manager selectors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed