[serve] Avoid deleting dynamically-deployed apps when applying config via REST API #44476

edoakes · 2024-04-04T14:44:00Z

Why are these changes needed?

See #44226 for more details, but TL;DR:

The REST API is declarative, meaning if you deploy a config that does not contain some running applications, they will be deleted.
However, in some cases users mix-and-match this declarative API with the imperative serve.run API. In this case, we should not delete the dynamically-created apps.
This PR addresses it by tracking the APIType and only deleting apps that were applied via the declarative API. There is no public API change. In the future we may want to consider exposing this metadata so external systems like Kuberay can treat the types of apps differently (e.g., for determining high-level status).

Related issue number

Closes #44226

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: Edward Oakes <[email protected]>

edoakes · 2024-04-04T15:18:32Z

cc @JoshKarpel

Signed-off-by: Edward Oakes <[email protected]>

python/ray/serve/_private/controller.py

python/ray/serve/_private/application_state.py

zcin

Overall looks great! One question: what happens if the user tries to imperatively deploy an app that was previously deployed declaratively, or vice versa? Do we allow this switching, or will we error out and force the user to delete the former before switching to the latter? (and for what we decide to be expected behavior, can we add a test?)

python/ray/serve/_private/application_state.py

zcin · 2024-04-04T16:12:52Z

python/ray/serve/tests/test_deploy_app.py

+    def check_application_running(
+        name: str, route_prefix: str, *, msg: str = "wonderful world"
+    ):
+        status = serve.status().applications[name]
+        assert status.status == "RUNNING"
+        assert requests.post(f"http://localhost:8000{route_prefix}/").text == msg
+        return True


If this is generalizable, maybe this can be added to test_utils?

yeah let me see if it makes sense

python/ray/serve/tests/unit/test_application_state.py

python/ray/serve/tests/test_deploy_app.py

Signed-off-by: Edward Oakes <[email protected]>

edoakes · 2024-04-04T19:34:07Z

@shrekris-anyscale @zcin I've added tests for the edge cases you pointed out. It's one long mega-test at the moment, probably going to split it out because it's quite confusing this way. But the behavior is that the latest operation for a given app name is how it is treated.

zcin

LGTM!

zcin · 2024-04-04T20:47:15Z

python/ray/serve/_private/application_state.py

+                    self._deployment_state_manager,
+                    endpoint_state=self._endpoint_state,
+                    save_checkpoint_func=self._save_checkpoint_func,
+                )


Not sure if this makes sense/would be helpful, but maybe we can add a log statement for when an app switches from imperative to declarative or vice versa when redeployed?

shrekris-anyscale

Nice work!

JoshKarpel · 2024-04-05T18:12:24Z

Thanks @edoakes , this is awesome! Does this automatically expose the information in the Serve REST API for consumption (in KubeRay) or does that need a follow-up PR?

edoakes · 2024-04-05T20:14:56Z

Thanks @edoakes , this is awesome! Does this automatically expose the information in the Serve REST API for consumption (in KubeRay) or does that need a follow-up PR?

It doesn't, for now I went with the most minimal solution that doesn't add any public API fields because I wanted to get it out for the next branch cut (today).

Do we need it in Kuberay for a specific purpose btw? What will we do with the information?

JoshKarpel · 2024-04-05T20:22:56Z

Do we need it in Kuberay for a specific purpose btw? What will we do with the information?

This was for the second part of the issue

Once that Serve change is made, KubeRay can have another new configuration option that would make it A) not touch applications created by serve.run when it sends config updates, B) not consider applications created by serve.run when determining readiness. This would solve Problem 2. I can contribute this change in KubeRay.

from my original post in #44226 . The first part of that is implicitly handled by this PR because KubeRay uses the declarative REST API, but for the second part KubeRay would need the additional metadata when it hits the dashboard API to get Serve application statuses.

## Why are these changes needed? This change exposes the new Serve app submission API type tracking introduced in #44476 in the dashboard API. My intent is to eventually introduce an *option* in KubeRay to *only* care about the status of `DECLARATIVE` Serve apps, so that it doesn't care about "dynamically deployed" `IMPERATIVE` apps. Per request, I've indicated that this is a developer API in the field docstring. ## Related issue number Follow-up for #44226 (comment) --------- Signed-off-by: Josh Karpel <[email protected]>

…project#45522) ## Why are these changes needed? This change exposes the new Serve app submission API type tracking introduced in ray-project#44476 in the dashboard API. My intent is to eventually introduce an *option* in KubeRay to *only* care about the status of `DECLARATIVE` Serve apps, so that it doesn't care about "dynamically deployed" `IMPERATIVE` apps. Per request, I've indicated that this is a developer API in the field docstring. ## Related issue number Follow-up for ray-project#44226 (comment) --------- Signed-off-by: Josh Karpel <[email protected]>

edoakes added 4 commits April 3, 2024 10:58

fix

af80fde

Signed-off-by: Edward Oakes <[email protected]>

WIP

410c0c9

Signed-off-by: Edward Oakes <[email protected]>

integration test

910e13f

Signed-off-by: Edward Oakes <[email protected]>

WIP

a89e530

Signed-off-by: Edward Oakes <[email protected]>

edoakes requested review from zcin and shrekris-anyscale April 4, 2024 15:18

edoakes added 2 commits April 4, 2024 10:20

WIP

55971db

Signed-off-by: Edward Oakes <[email protected]>

WIP

49b2d7a

Signed-off-by: Edward Oakes <[email protected]>

edoakes commented Apr 4, 2024

View reviewed changes

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

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

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

zcin reviewed Apr 4, 2024

View reviewed changes

shrekris-anyscale reviewed Apr 4, 2024

View reviewed changes

python/ray/serve/tests/test_deploy_app.py Show resolved Hide resolved

WIP

2a6d1dc

Signed-off-by: Edward Oakes <[email protected]>

zcin approved these changes Apr 4, 2024

View reviewed changes

shrekris-anyscale approved these changes Apr 4, 2024

View reviewed changes

edoakes merged commit f09d01e into ray-project:master Apr 4, 2024
5 checks passed

JoshKarpel mentioned this pull request May 23, 2024

[Serve] Expose Serve app source in get_serve_instance_details #45522

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[serve] Avoid deleting dynamically-deployed apps when applying config via REST API #44476

[serve] Avoid deleting dynamically-deployed apps when applying config via REST API #44476

edoakes commented Apr 4, 2024 •

edited

Loading

edoakes commented Apr 4, 2024

zcin left a comment

zcin Apr 4, 2024

edoakes Apr 4, 2024

edoakes commented Apr 4, 2024

zcin left a comment

zcin Apr 4, 2024

shrekris-anyscale left a comment

JoshKarpel commented Apr 5, 2024

edoakes commented Apr 5, 2024

JoshKarpel commented Apr 5, 2024

[serve] Avoid deleting dynamically-deployed apps when applying config via REST API #44476

[serve] Avoid deleting dynamically-deployed apps when applying config via REST API #44476

Conversation

edoakes commented Apr 4, 2024 • edited Loading

Why are these changes needed?

Related issue number

Checks

edoakes commented Apr 4, 2024

zcin left a comment

Choose a reason for hiding this comment

zcin Apr 4, 2024

Choose a reason for hiding this comment

edoakes Apr 4, 2024

Choose a reason for hiding this comment

edoakes commented Apr 4, 2024

zcin left a comment

Choose a reason for hiding this comment

zcin Apr 4, 2024

Choose a reason for hiding this comment

shrekris-anyscale left a comment

Choose a reason for hiding this comment

JoshKarpel commented Apr 5, 2024

edoakes commented Apr 5, 2024

JoshKarpel commented Apr 5, 2024

edoakes commented Apr 4, 2024 •

edited

Loading