-
Notifications
You must be signed in to change notification settings - Fork 6.7k
[data] Extract backpressure-related code from ResourceManager as a policy #54376
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
[data] Extract backpressure-related code from ResourceManager as a policy #54376
Conversation
Signed-off-by: Hao Chen <[email protected]>
Signed-off-by: Hao Chen <[email protected]>
Signed-off-by: Hao Chen <[email protected]>
Signed-off-by: Hao Chen <[email protected]>
Signed-off-by: Hao Chen <[email protected]>
Signed-off-by: Hao Chen <[email protected]>
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.
Pull Request Overview
This PR extracts backpressure logic from ResourceManager
into a standalone ResourceBudgetBackpressurePolicy
, integrates configurable backpressure policies into the streaming executor for both task submission and task output, and adds a new task_output_backpressure_time
metric.
- Introduce a
BackpressurePolicy
interface with methods for input (can_add_input
) and output (max_task_output_bytes_to_read
) backpressure and extract a resource-based policy. - Refactor
process_completed_tasks
andget_eligible_operators
to use a list of backpressure policies instead of tightly coupling toResourceManager
. - Add
task_output_backpressure_time
metric inOpRuntimeMetrics
and notify operators of output backpressure.
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
python/ray/data/tests/test_streaming_executor.py | Updated tests to call process_completed_tasks and get_eligible_operators with policy lists and introduced a TestBackpressurePolicy . |
python/ray/data/tests/test_resource_manager.py | Replaced direct allocator.can_submit_new_task calls with a helper that uses get_budget . |
python/ray/data/_internal/execution/streaming_executor_state.py | Refactored process_completed_tasks and get_eligible_operators to use backpressure policy lists. |
python/ray/data/_internal/execution/streaming_executor.py | Updated executor initialization to pass data_context , topology , and resource_manager into policy factory and use a list of policies. |
python/ray/data/_internal/execution/resource_manager.py | Exposed max_task_output_bytes_to_read and get_budget wrappers and removed can_submit_new_task from the allocator interface. |
python/ray/data/_internal/execution/interfaces/physical_operator.py | Added notify_in_task_output_backpressure for output backpressure metrics. |
python/ray/data/_internal/execution/interfaces/op_runtime_metrics.py | Added task_output_backpressure_time metric and timing logic. |
python/ray/data/_internal/execution/backpressure_policy/backpressure_policy.py | Defined BackpressurePolicy base class with new constructor and default output method. |
python/ray/data/_internal/execution/backpressure_policy/resource_budget_backpressure_policy.py | New ResourceBudgetBackpressurePolicy implementation. |
python/ray/data/_internal/execution/backpressure_policy/concurrency_cap_backpressure_policy.py | Adapted constructor to accept common parameters via super() . |
python/ray/data/_internal/execution/backpressure_policy/init.py | Updated get_backpressure_policies to pass data_context , topology , and resource_manager . |
Comments suppressed due to low confidence (1)
python/ray/data/_internal/execution/interfaces/op_runtime_metrics.py:361
- Tests for the new task_output_backpressure_time metric are missing. Add unit tests to verify that on_toggle_task_output_backpressure correctly starts, stops, and accumulates the backpressure timer.
task_output_backpressure_time: float = metric_field(
python/ray/data/_internal/execution/backpressure_policy/resource_budget_backpressure_policy.py
Outdated
Show resolved
Hide resolved
Signed-off-by: Hao Chen <[email protected]>
Signed-off-by: Hao Chen <[email protected]>
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.
Small nits but lgtm!
if max_bytes_to_read is not None: | ||
max_bytes_to_read_per_op[state] = max_bytes_to_read | ||
for op, state in topology.items(): | ||
# Check all backpressure policies for max_task_output_bytes_to_read |
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.
Nit: For future debugging purposes, do we want to log the backpressure policy that is being used? Wondering about the case where one might be overly restrictive but it becomes hard to tell which one?
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.
yes, I thought of that. but not sure what's the best way to surface this info (too long to progress bars). I'll leave a TODO here.
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.
Yeah I wonder if just logging at the debug level would be sufficient? If that would cause it to only appear in the ray_data.log file then maybe that would work? Fine to punt on that though.
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.
even logging might be too verbose.
given that we don't have that many policies right now. Logging that doesn't add much value.
So I'll leave it for now
in_backpressure = not under_resource_limits or not all( | ||
p.can_add_input(op) for p in backpressure_policies | ||
) | ||
# Operator is considered being in task-submission back-pressure any |
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.
Nit: I think missing an if: "Operator is considered being in task-submission back-pressure any"
-> "Operator is considered being in task-submission back-pressure if any"
Signed-off-by: Hao Chen <[email protected]>
"""Initialize the backpressure policy. | ||
Args: |
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.
"""Initialize the backpressure policy. | |
Args: | |
"""Initialize the backpressure policy. | |
Args: |
Signed-off-by: Hao Chen <[email protected]>
Signed-off-by: Hao Chen <[email protected]>
Signed-off-by: Hao Chen <[email protected]>
Signed-off-by: Hao Chen <[email protected]>
Signed-off-by: Hao Chen <[email protected]>
Signed-off-by: Hao Chen <[email protected]>
Signed-off-by: Hao Chen <[email protected]>
task_output_backpressure_time: float = metric_field( | ||
default=0, | ||
description="Time spent in task output backpressure.", | ||
metrics_group=MetricsGroup.TASKS, | ||
) |
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.
Should we track this as per-task metric? I don't think cumulative one is very useful
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.
It keeps track of the total time when the op is backpressured.
Op-level metric would be more useful as we can show them in the Grafana dashboard.
task-level metric will result in too many lines and make the chart difficult to read
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.
How can be the cumulative time be useful?
task-level metric will result in too many lines
Well, you don't show every line you just show the distribution.
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.
we can handle that later. currently we already report cumulative metric for task submission backpressure time.
def get_budget(self, op: PhysicalOperator) -> ExecutionResources: | ||
return self._op_budgets[op] | ||
def get_budget(self, op: PhysicalOperator) -> Optional[ExecutionResources]: | ||
return self._op_budgets.get(op, None) |
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.
return self._op_budgets.get(op, None) | |
return self._op_budgets.get(op) |
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.
I intentionally changed this to return an optional. Because ineligible ops don't have budgets.
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.
Yeah, i understand that but what's the point of specifying default? It will return none anyways
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.
got it. somehow I thought it raises key error by default.
max_bytes_to_read = None | ||
for policy in backpressure_policies: | ||
policy_limit = policy.max_task_output_bytes_to_read(op) | ||
if policy_limit is not None: | ||
if max_bytes_to_read is None: | ||
max_bytes_to_read = policy_limit | ||
else: | ||
max_bytes_to_read = min(max_bytes_to_read, policy_limit) |
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.
max_bytes_to_read = None | |
for policy in backpressure_policies: | |
policy_limit = policy.max_task_output_bytes_to_read(op) | |
if policy_limit is not None: | |
if max_bytes_to_read is None: | |
max_bytes_to_read = policy_limit | |
else: | |
max_bytes_to_read = min(max_bytes_to_read, policy_limit) | |
max_bytes = min([p.max_task_output_bytes_to_read(op) or float("inf") for p in policies]) |
# Operator is considered being in task-submission back-pressure if any | ||
# back-pressure policy is violated | ||
in_backpressure = any(not p.can_add_input(op) for p in backpressure_policies) |
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.
As @omatthew98 pointed out above -- let's make back-pressuring traceable (by logging when the op becomes throttled first time)
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.
The op will be flipping between backpressure and non-backpressure status pretty frequently. I don't think logging the first time would be useful.
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.
It should be every-time it goes from no-throttling to being throttled
) as _mock: | ||
_mock.side_effect = lambda op: False if op is o2 else True | ||
assert _get_eligible_ops_to_run(ensure_liveness=False) == [o3] | ||
class TestBackpressurePolicy(BackpressurePolicy): |
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.
Let's actually test actual back-pressure policies that we have
def can_add_input(self, op: "PhysicalOperator") -> bool: | ||
budget = self._resource_manager.get_budget(op) | ||
if budget is None: | ||
return True | ||
return op.incremental_resource_usage().satisfies_limit(budget) | ||
|
||
def max_task_output_bytes_to_read(self, op: "PhysicalOperator") -> Optional[int]: | ||
"""Determine maximum bytes to read based on the resource budgets. | ||
|
||
Args: | ||
op: The operator to get the limit for. | ||
|
||
Returns: | ||
The maximum bytes that can be read, or None if no limit. | ||
""" | ||
return self._resource_manager.max_task_output_bytes_to_read(op) |
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.
Let's make sure we keep existing coverage of this
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.
yeah, it's already tested in test_resource_manager.
…licy (ray-project#54376) * Extract backpressure related methods (`can_submit_new_tasks` and `max_task_output_bytes_to_read`) from ResourceManager, as a standalone policy `ResourceBudgetBackpressurePolicy`) * Report task_output_backpressure_time metric. --------- Signed-off-by: Hao Chen <[email protected]> Signed-off-by: doyoung <[email protected]>
…licy (ray-project#54376) * Extract backpressure related methods (`can_submit_new_tasks` and `max_task_output_bytes_to_read`) from ResourceManager, as a standalone policy `ResourceBudgetBackpressurePolicy`) * Report task_output_backpressure_time metric. --------- Signed-off-by: Hao Chen <[email protected]> Signed-off-by: doyoung <[email protected]>
…licy (ray-project#54376) * Extract backpressure related methods (`can_submit_new_tasks` and `max_task_output_bytes_to_read`) from ResourceManager, as a standalone policy `ResourceBudgetBackpressurePolicy`) * Report task_output_backpressure_time metric. --------- Signed-off-by: Hao Chen <[email protected]> Signed-off-by: ChanChan Mao <[email protected]>
can_submit_new_tasks
andmax_task_output_bytes_to_read
) from ResourceManager, as a standalone policyResourceBudgetBackpressurePolicy
)