[Serve] Optimize `ServeController.get_app_config()` #45878

JoshKarpel · 2024-06-11T21:07:14Z

Why are these changes needed?

Currently, ServeController.get_app_config() is called once for each application during ServeController.get_serve_instance_details(). Each of those calls requires a round-trip to the GCS to get the Serve checkpoint, but the Serve checkpoint has all of the information for all apps already. This PR replaces ServeController.get_app_config() with a new method that I've very cleverly named ServeController.get_app_configs(), which effectively pre-fetches all the application configs and leaves it up to the caller to decide what to do with them.

Before, 1800 apps with 1 deployment each, no DeploymentHandles, with #45872 :

After: so little time it doesn't even appear in the flamegraph!

Related issue number

Discovered alongside #45872

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>

JoshKarpel · 2024-06-12T17:28:35Z

python/ray/serve/_private/client.py

-    @_ensure_connected
-    def get_app_config(self, name: str = SERVE_DEFAULT_APP_NAME) -> Dict:
-        """Returns the most recently requested Serve config."""
-        return ray.get(self._controller.get_app_config.remote(name))


This seems to be unused, so I went ahead and removed it instead of refactoring it. But I'm happy to provide an equivalent method in this PR if desired.

JoshKarpel · 2024-06-12T17:30:23Z

I'm seeing a test failure on test_get_applications_while_gcs_down https://buildkite.com/ray-project/microcheck/builds/1501#01900d0b-bdb6-4ed7-b49e-f6716901d47f that seems to be coming from

ray/dashboard/modules/serve/serve_rest_api_impl.py

Lines 122 to 134 in 24aa146

    
           try: 
        
               details = await controller.get_serve_instance_details.remote() 
        
           except ray.exceptions.RayTaskError as e: 
        
               # Task failure sometimes are due to GCS 
        
               # failure. When GCS failed, we expect a longer time 
        
               # to recover. 
        
               return Response( 
        
                   status=503, 
        
                   text=( 
        
                       "Failed to get a response from the controller. " 
        
                       f"The GCS may be down, please retry later: {e}" 
        
                   ), 
        
               )

returning an HTTP 503 error, but I don't see how my changes could change the semantics such that that test could fail now but pass previously 🤔 ... the test seems to be expecting to get HTTP 200 responses even when GCS is down?

venkatkalluru

LGTM

python/ray/serve/_private/controller.py

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

shrekris-anyscale

Thanks for the contribution! I left a couple comments.

python/ray/serve/_private/controller.py

zcin · 2024-06-19T00:11:31Z

python/ray/serve/_private/controller.py

@@ -877,6 +877,8 @@ def get_serve_instance_details(self) -> Dict:
        grpc_config = self.get_grpc_config()
        applications = {}

+        app_configs = self.get_app_configs() or {}


I'm seeing a test failure on test_get_applications_while_gcs_down

I believe it's because you're trying to access the KV store while the GCS is down. Could you change this to not fetch the app configs when there are no applications?

Thanks! Just pushed this up 🤞🏻

Looks like this is passing now! Re-requested your review

Co-authored-by: shrekris-anyscale <92341594+shrekris-anyscale@users.noreply.github.com> Signed-off-by: Josh Karpel <josh.karpel@gmail.com>

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

zcin

LGTM!

shrekris-anyscale · 2024-06-24T22:06:39Z

Premerge is failing @JoshKarpel. Could you take a look?

JoshKarpel · 2024-06-25T20:51:53Z

It looks like a backwards compat test in the Ray Jobs API with a (hopefully) unrelated error https://buildkite.com/ray-project/premerge/builds/27215#01904b67-78b6-49f9-b489-0c09dd2a9dd5:


[2024-06-24T18:22:19Z] 2024-06-24 18:21:31,250	ERR cli.py:68 -- ---------------------------------------------
--
  | [2024-06-24T18:22:19Z] 2024-06-24 18:21:31,250	ERR cli.py:69 -- Job 'c20edd7507564af4960751cdb1f096f9' failed
  | [2024-06-24T18:22:19Z] 2024-06-24 18:21:31,250	ERR cli.py:70 -- ---------------------------------------------
  | [2024-06-24T18:22:19Z] 2024-06-24 18:21:31,250	INFO cli.py:83 -- Status message: Unexpected error occurred: The actor died because of an error raised in its creation task, ray::_ray_internal_job_actor_c20edd7507564af4960751cdb1f096f9:JobSupervisor.__init__ (pid=2706, ip=172.16.0.5, repr=<ray.dashboard.modules.job.job_manager.FunctionActorManager._create_fake_actor_class.<locals>.TemporaryActor object at 0x7fe8210a0070>)
  | [2024-06-24T18:22:19Z]   File "/opt/miniconda/envs/jobs-backwards-compatibility-f3829cebbbab4d94b5d878308ae2264f/lib/python3.9/concurrent/futures/_base.py", line 439, in result
  | [2024-06-24T18:22:19Z]     return self.__get_result()
  | [2024-06-24T18:22:19Z]   File "/opt/miniconda/envs/jobs-backwards-compatibility-f3829cebbbab4d94b5d878308ae2264f/lib/python3.9/concurrent/futures/_base.py", line 391, in __get_result
  | [2024-06-24T18:22:19Z]     raise self._exception
  | [2024-06-24T18:22:19Z] RuntimeError: The actor with name JobSupervisor failed to import on the worker. This may be because needed library dependencies are not installed in the worker environment:
  | [2024-06-24T18:22:19Z]
  | [2024-06-24T18:22:19Z] ray::_ray_internal_job_actor_c20edd7507564af4960751cdb1f096f9:JobSupervisor.__init__ (pid=2706, ip=172.16.0.5, repr=<ray.dashboard.modules.job.job_manager.FunctionActorManager._create_fake_actor_class.<locals>.TemporaryActor object at 0x7fe8210a0070>)
  | [2024-06-24T18:22:19Z]   File "/opt/miniconda/envs/jobs-backwards-compatibility-f3829cebbbab4d94b5d878308ae2264f/lib/python3.9/site-packages/ray/dashboard/modules/job/job_manager.py", line 28, in <module>
  | [2024-06-24T18:22:19Z]     from ray.job_submission import JobStatus
  | [2024-06-24T18:22:19Z]   File "/opt/miniconda/envs/jobs-backwards-compatibility-f3829cebbbab4d94b5d878308ae2264f/lib/python3.9/site-packages/ray/job_submission/__init__.py", line 2, in <module>
  | [2024-06-24T18:22:19Z]     from ray.dashboard.modules.job.sdk import JobSubmissionClient
  | [2024-06-24T18:22:19Z]   File "/opt/miniconda/envs/jobs-backwards-compatibility-f3829cebbbab4d94b5d878308ae2264f/lib/python3.9/site-packages/ray/dashboard/modules/job/sdk.py", line 24, in <module>
  | [2024-06-24T18:22:19Z]     from ray.dashboard.modules.dashboard_sdk import SubmissionClient
  | [2024-06-24T18:22:19Z]   File "/opt/miniconda/envs/jobs-backwards-compatibility-f3829cebbbab4d94b5d878308ae2264f/lib/python3.9/site-packages/ray/dashboard/modules/dashboard_sdk.py", line 9, in <module>
  | [2024-06-24T18:22:19Z]     from pkg_resources import packaging
  | [2024-06-24T18:22:19Z] ImportError: cannot import name 'packaging' from 'pkg_resources' (/tmp/ray/session_2024-06-24_18-21-21_620793_2512/runtime_resources/pip/8e07f3c7538ebeb5b023a55baf02d6c8fee8bc80/virtualenv/lib/python3.9/site-packages/pkg_resources/__init__.py)

I merged latest master, will see if it still fails on the new CI run 🤞🏻

JoshKarpel · 2024-06-25T21:59:59Z

Looks like the failure was ephemeral @shrekris-anyscale 🤞🏻

JoshKarpel · 2024-06-26T01:52:41Z

Thanks!

JoshKarpel added 3 commits June 11, 2024 16:04

optimize ServeController.get_app_config()

80281b1

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

handle case where config is None

e95128b

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

Merge branch 'master' into optimize-get-app-config

6750598

JoshKarpel commented Jun 12, 2024

View reviewed changes

JoshKarpel marked this pull request as ready for review June 12, 2024 17:30

venkatkalluru reviewed Jun 12, 2024

View reviewed changes

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

use a guard clause

17e77c8

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

JoshKarpel mentioned this pull request Jun 12, 2024

[Serve] Avoid looping over all snapshot ids for each long poll request #45881

Merged

8 tasks

shrekris-anyscale reviewed Jun 18, 2024

View reviewed changes

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

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

zcin reviewed Jun 19, 2024

View reviewed changes

JoshKarpel and others added 3 commits June 20, 2024 13:00

Apply suggestions from code review

a4ad1ed

Co-authored-by: shrekris-anyscale <92341594+shrekris-anyscale@users.noreply.github.com> Signed-off-by: Josh Karpel <josh.karpel@gmail.com>

try avoiding getting app configs if the GCS is down

10f6d4c

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

Merge branch 'master' into optimize-get-app-config

1d2aa6d

JoshKarpel requested review from zcin and shrekris-anyscale June 24, 2024 17:56

zcin approved these changes Jun 24, 2024

View reviewed changes

zcin added the go add ONLY when ready to merge, run all tests label Jun 24, 2024

shrekris-anyscale approved these changes Jun 24, 2024

View reviewed changes

Merge branch 'master' into optimize-get-app-config

5f722b2

shrekris-anyscale merged commit a709b8f into ray-project:master Jun 25, 2024
6 checks passed

JoshKarpel deleted the optimize-get-app-config branch June 26, 2024 01:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Serve] Optimize `ServeController.get_app_config()` #45878

[Serve] Optimize `ServeController.get_app_config()` #45878

JoshKarpel commented Jun 11, 2024 •

edited

Loading

JoshKarpel Jun 12, 2024

JoshKarpel commented Jun 12, 2024 •

edited

Loading

venkatkalluru left a comment

shrekris-anyscale left a comment

zcin Jun 19, 2024

JoshKarpel Jun 20, 2024

JoshKarpel Jun 24, 2024

zcin left a comment

shrekris-anyscale commented Jun 24, 2024

JoshKarpel commented Jun 25, 2024

JoshKarpel commented Jun 25, 2024

JoshKarpel commented Jun 26, 2024

[Serve] Optimize ServeController.get_app_config() #45878

[Serve] Optimize ServeController.get_app_config() #45878

Conversation

JoshKarpel commented Jun 11, 2024 • edited Loading

Why are these changes needed?

Related issue number

Checks

JoshKarpel Jun 12, 2024

Choose a reason for hiding this comment

JoshKarpel commented Jun 12, 2024 • edited Loading

venkatkalluru left a comment

Choose a reason for hiding this comment

shrekris-anyscale left a comment

Choose a reason for hiding this comment

zcin Jun 19, 2024

Choose a reason for hiding this comment

JoshKarpel Jun 20, 2024

Choose a reason for hiding this comment

JoshKarpel Jun 24, 2024

Choose a reason for hiding this comment

zcin left a comment

Choose a reason for hiding this comment

shrekris-anyscale commented Jun 24, 2024

JoshKarpel commented Jun 25, 2024

JoshKarpel commented Jun 25, 2024

JoshKarpel commented Jun 26, 2024

[Serve] Optimize `ServeController.get_app_config()` #45878

[Serve] Optimize `ServeController.get_app_config()` #45878

JoshKarpel commented Jun 11, 2024 •

edited

Loading

JoshKarpel commented Jun 12, 2024 •

edited

Loading