[Serve] Group `DeploymentHandle` autoscaling metrics pushes by process #45957

JoshKarpel · 2024-06-14T14:38:05Z

Why are these changes needed?

We're seeing a lot of pressure on the Serve controller from metrics push tasks when running thousands of Serve apps. A lot of this pressure is purely from the overhead of lots of RPC connections incoming to the controller. We might be able to amortize this overhead (and presumably similar overhead in the handles too) by having the metrics push happen at the per-process level instead of the per-handle level.

We've made this change on our setup and it has reduced CPU time spent on this on the Controller, and also our ingress application replicas that have all the handles.

Related issue number

Closes #45777

Checks

I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
I've run scripts/format.sh to lint the changes in this PR.
I've included any doc changes needed for https://docs.ray.io/en/master/.
- I've added any new APIs to the API Reference. For example, if I added a
  method in Tune, I've added it in doc/source/tune/api/ under the
  corresponding .rst file.
I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
Testing Strategy
- Unit tests
- Release tests
- This PR is not tested :(

Signed-off-by: Josh Karpel <[email protected]>

…shing-v2 Signed-off-by: Josh Karpel <[email protected]>

Signed-off-by: Josh Karpel <[email protected]>

venkatkalluru

need to understand a bit more, some nit comments.

python/ray/serve/_private/autoscaling_state.py

python/ray/serve/_private/controller.py

Signed-off-by: Josh Karpel <[email protected]>

JoshKarpel · 2024-12-18T16:03:44Z

python/ray/serve/_private/router.py

+                )
+                shared.register(self)
+                logger.info(
+                    f"Registered {self._handle_id} with shared metrics pusher {shared}."
                )
            else:


I'm not sure I understand this else block - it seems like even if RAY_SERVE_COLLECT_AUTOSCALING_METRICS_ON_HANDLE = False we still register the pushing task? It just won't have any data because the _add_autoscaling_metrics_point task isn't registered? 🤔

JoshKarpel · 2024-12-18T16:05:58Z

python/ray/serve/_private/router.py

-                self.metrics_pusher.register_or_update_task(
-                    self.PUSH_METRICS_TO_CONTROLLER_TASK_NAME,
-                    self.push_autoscaling_metrics_to_controller,
-                    autoscaling_config.metrics_interval_s,


@zcin @edoakes I'm working on resurrecting this PR and I think this was the main sticking point - the shared metrics pusher wouldn't respect the autoscaling_config.metrics_interval_s in the current form of the PR. Can we deprecate that parameter? Would we need to feature-flag this PR so it can be introduced gradually? Or maybe we want to preserve it and have a different shared pusher for each metric_interval_s?

I am fine with making the metrics_interval_s a cluster-level option configured via env var. WDYT Cindy?

Ah! I like that option!

Yes I think making the metrics interval a cluster level option makes sense.

Sounds good - I will take that approach when I get back from the holidays

Oh, it turns out there already is an env var for this, HANDLE_METRIC_PUSH_INTERVAL_S

ray/python/ray/serve/_private/constants.py

Lines 148 to 151 in 4742373

# Handle metric push interval. (This interval will affect the cold start time period)

HANDLE_METRIC_PUSH_INTERVAL_S = float(

os.environ.get("RAY_SERVE_HANDLE_METRIC_PUSH_INTERVAL_S", "10")

)

, and I'm already using it

ray/python/ray/serve/_private/router.py

Line 350 in 86da5fa

HANDLE_METRIC_PUSH_INTERVAL_S,

The autoscaling_config.metrics_interval_s is also used in a variety of other places, like for controlling how often metrics are recorded

ray/python/ray/serve/_private/router.py

Lines 200 to 203 in 86da5fa

min(

RAY_SERVE_HANDLE_AUTOSCALING_METRIC_RECORD_PERIOD_S,

autoscaling_config.metrics_interval_s,

),

Should I remove all of those and just use the single env var, or keep the other uses and only "ignore" the setting here for the shared pusher?

I'm inclined to remove all of them for consistency but that's a much bigger blast radius...

I think we should remove autoscaling_config.metrics_interval_s and just use RAY_SERVE_HANDLE_AUTOSCALING_METRIC_RECORD_PERIOD_S for recording metrics, because the interval at which we record metrics is usually more frequent than pushing metrics to the controller. And then we can use HANDLE_METRIC_PUSH_INTERVAL_S, but perhaps renaming it to include "autoscaling" would be better going forward, if it is to replace autoscaling_config.metrics_interval_s?

# Conflicts: # python/ray/serve/_private/router.py

## Why are these changes needed? In our use case we use Ray Serve with many hundreds/thousands of apps, plus a "router" app that routes traffic to those apps using `DeploymentHandle`s. Right now, that means we have a `LongPollClient` for each `DeploymentHandle` in each router app replica, which could be tens or hundreds of thousands of `LongPollClient`s. This is expensive on both the Serve Controller and on the router app replicas. It can be particularly problematic in resource usage on the Serve Controller - the main thing blocking us from having as many router replicas as we'd like is the stability of the controller. This PR aims to amortize this cost of having so many `LongPollClient`s by going from one-long-poll-client-per-handle to one-long-poll-client-per-process. Each `DeploymentHandle`'s `Router` now registers itself with a shared `LongPollClient` held by a singleton. The actual implementation that I've gone with is a bit clunky because I'm trying to bridge the gap between the current solution and a design that *only* has shared `LongPollClient`s. This could potentially be cleaned up in the future. Right now, each `Router` still gets a dedicated `LongPollClient` that only runs temporarily, until the shared client tells it to stop. Related: #45957 is the same idea but for handle autoscaling metrics pushing.  ## Related issue number  ## Checks - [x] I've signed off every commit(by using the -s flag, i.e., `git commit -s`) in this PR. - [x] I've run `scripts/format.sh` to lint the changes in this PR. - [ ] I've included any doc changes needed for https://docs.ray.io/en/master/. - [ ] I've added any new APIs to the API Reference. For example, if I added a method in Tune, I've added it in `doc/source/tune/api/` under the corresponding `.rst` file. - [x] I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/ - Testing Strategy - [x] Unit tests - [ ] Release tests - [ ] This PR is not tested :( --------- Signed-off-by: Josh Karpel <[email protected]>

JoshKarpel added 4 commits June 10, 2024 16:54

introduce shared handle metrics pusher

9b3e368

Signed-off-by: Josh Karpel <[email protected]>

minor optimizations

45dc6e5

Signed-off-by: Josh Karpel <[email protected]>

Merge branch 'refs/heads/master' into issue-45777-amortize-metrics-pu…

7cf7299

…shing-v2 Signed-off-by: Josh Karpel <[email protected]>

debug logs

84f6c10

Signed-off-by: Josh Karpel <[email protected]>

venkatkalluru reviewed Jun 17, 2024

View reviewed changes

python/ray/serve/_private/autoscaling_state.py Show resolved Hide resolved

python/ray/serve/_private/controller.py Outdated Show resolved Hide resolved

updated HandleMetricReport docstring, change to debug log

4a48902

Signed-off-by: Josh Karpel <[email protected]>

JoshKarpel changed the title ~~Group DeploymentHandle autoscaling metrics pushes by process~~ [Serve] Group DeploymentHandle autoscaling metrics pushes by process Jun 17, 2024

JoshKarpel mentioned this pull request Jun 26, 2024

[Serve] Provide backpressure on handle metrics push #45776

Open

anyscalesam added triage Needs triage (eg: priority, bug/not-bug, and owning component) serve Ray Serve Related Issue labels Aug 8, 2024

JoshKarpel mentioned this pull request Dec 11, 2024

[Serve] Shared LongPollClient for Routers #48807

Merged

8 tasks

JoshKarpel added 3 commits December 12, 2024 10:01

Merge branch 'master' into issue-45777-amortize-metrics-pushing-v2

af5d76a

tidy up

415a44b

Signed-off-by: Josh Karpel <[email protected]>

Merge branch 'master' into issue-45777-amortize-metrics-pushing-v2

088800f

JoshKarpel commented Dec 18, 2024

View reviewed changes

Merge branch 'master' into issue-45777-amortize-metrics-pushing-v2

86da5fa

# Conflicts: # python/ray/serve/_private/router.py

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Serve] Group `DeploymentHandle` autoscaling metrics pushes by process #45957

[Serve] Group `DeploymentHandle` autoscaling metrics pushes by process #45957

JoshKarpel commented Jun 14, 2024 •

edited

Loading

venkatkalluru left a comment

JoshKarpel Dec 18, 2024

JoshKarpel Dec 18, 2024 •

edited

Loading

edoakes Dec 18, 2024

JoshKarpel Dec 19, 2024

zcin Dec 20, 2024

JoshKarpel Dec 20, 2024

JoshKarpel Jan 6, 2025

zcin Jan 7, 2025

	# Handle metric push interval. (This interval will affect the cold start time period)
	HANDLE_METRIC_PUSH_INTERVAL_S = float(
	os.environ.get("RAY_SERVE_HANDLE_METRIC_PUSH_INTERVAL_S", "10")
	)

	min(
	RAY_SERVE_HANDLE_AUTOSCALING_METRIC_RECORD_PERIOD_S,
	autoscaling_config.metrics_interval_s,
	),

[Serve] Group DeploymentHandle autoscaling metrics pushes by process #45957

Are you sure you want to change the base?

[Serve] Group DeploymentHandle autoscaling metrics pushes by process #45957

Conversation

JoshKarpel commented Jun 14, 2024 • edited Loading

Why are these changes needed?

Related issue number

Checks

venkatkalluru left a comment

Choose a reason for hiding this comment

JoshKarpel Dec 18, 2024

Choose a reason for hiding this comment

JoshKarpel Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

edoakes Dec 18, 2024

Choose a reason for hiding this comment

JoshKarpel Dec 19, 2024

Choose a reason for hiding this comment

zcin Dec 20, 2024

Choose a reason for hiding this comment

JoshKarpel Dec 20, 2024

Choose a reason for hiding this comment

JoshKarpel Jan 6, 2025

Choose a reason for hiding this comment

zcin Jan 7, 2025

Choose a reason for hiding this comment

[Serve] Group `DeploymentHandle` autoscaling metrics pushes by process #45957

[Serve] Group `DeploymentHandle` autoscaling metrics pushes by process #45957

JoshKarpel commented Jun 14, 2024 •

edited

Loading

JoshKarpel Dec 18, 2024 •

edited

Loading