[Serve] Group `DeploymentHandle` autoscaling metrics pushes by process by JoshKarpel · Pull Request #45957 · ray-project/ray

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 <josh.karpel@gmail.com>

…shing-v2 Signed-off-by: Josh Karpel <josh.karpel@gmail.com>

Signed-off-by: Josh Karpel <josh.karpel@gmail.com>

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 <josh.karpel@gmail.com>

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

python/ray/serve/_private/router.py

+                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 <josh.karpel@gmail.com>

## 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: ray-project#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 <josh.karpel@gmail.com>

akshay-anyscale · 2025-05-23T13:29:11Z

@JoshKarpel were you planning to pick this back up?

JoshKarpel · 2025-05-23T13:57:41Z

@JoshKarpel were you planning to pick this back up?

Thanks for the bump - yes, but it keeps getting pushed off my plate with higher-priority internal projects and things like #53173. I really am hoping to get to it in the next few weeks, but I've been saying that for months...

github-actions · 2025-06-07T00:31:31Z

This pull request has been automatically marked as stale because it has not had
any activity for 14 days. It will be closed in another 14 days if no further activity occurs.
Thank you for your contributions.

You can always ask for help on our discussion forum or Ray's public slack channel.

If you'd like to keep this open, just leave any comment, and the stale label will be removed.

github-actions · 2025-06-22T00:35:06Z

This pull request has been automatically closed because there has been no more activity in the 14 days
since being marked stale.

Please feel free to reopen or open a new pull request if you'd still like this to be addressed.

Again, you can always ask for help on our discussion forum or Ray's public slack channel.

Thanks again for your contribution!

JoshKarpel · 2025-06-23T18:20:26Z

Woops, I let the PR get closed by the bot :(

I'll make a new PR and link it back here when I get back to this

JoshKarpel added 4 commits June 10, 2024 16:54

introduce shared handle metrics pusher

9b3e368

Signed-off-by: Josh Karpel <josh.karpel@gmail.com>

minor optimizations

45dc6e5

Signed-off-by: Josh Karpel <josh.karpel@gmail.com>

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

7cf7299

…shing-v2 Signed-off-by: Josh Karpel <josh.karpel@gmail.com>

debug logs

84f6c10

Signed-off-by: Josh Karpel <josh.karpel@gmail.com>

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 <josh.karpel@gmail.com>

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 <josh.karpel@gmail.com>

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

hainesmichaelc added the community-contribution Contributed by the community label Apr 4, 2025

github-actions bot added the stale The issue is stale. It will be closed within 7 days unless there are further conversation label Jun 7, 2025

github-actions bot closed this Jun 22, 2025

JoshKarpel mentioned this pull request Jul 29, 2025

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

Open

8 tasks

flo-ri-an mentioned this pull request Sep 17, 2025

[Core]: Fatal error reported in dashboard #56581

Closed

	# 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,
	),

Conversation

JoshKarpel commented Jun 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why are these changes needed?

Related issue number

Checks

Uh oh!

venkatkalluru left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

JoshKarpel Dec 18, 2024

Choose a reason for hiding this comment

Uh oh!

JoshKarpel Dec 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edoakes Dec 18, 2024

Choose a reason for hiding this comment

Uh oh!

JoshKarpel Dec 19, 2024

Choose a reason for hiding this comment

Uh oh!

zcin Dec 20, 2024

Choose a reason for hiding this comment

Uh oh!

JoshKarpel Dec 20, 2024

Choose a reason for hiding this comment

Uh oh!

JoshKarpel Jan 6, 2025

Choose a reason for hiding this comment

Uh oh!

zcin Jan 7, 2025

Choose a reason for hiding this comment

Uh oh!

akshay-anyscale commented May 23, 2025

Uh oh!

JoshKarpel commented May 23, 2025

Uh oh!

github-actions bot commented Jun 7, 2025

Uh oh!

github-actions bot commented Jun 22, 2025

Uh oh!

JoshKarpel commented Jun 23, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

JoshKarpel commented Jun 14, 2024 •

edited

Loading

JoshKarpel Dec 18, 2024 •

edited

Loading