Skip to content

Commit 6e30704

Browse files
authored
[Serve] Check multiple FastAPI ingress deployments in a single application (#53647)
## Why are these changes needed? - Currently, Serve can not catch multiple FastAPI deployments in a single application if user sets the docs path to None in their FastAPI app. - We can check multiple ASGIAppReplicaWrapper in a single application to avoid this issue. ## Related issue number Closes #53024 ## 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. - [x] 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 - [ ] Unit tests - [ ] Release tests - [ ] This PR is not tested :( --------- Signed-off-by: Ziy1-Tan <ajb459684460@gmail.com>
1 parent 05d78b8 commit 6e30704

File tree

4 files changed

+93
-4
lines changed

4 files changed

+93
-4
lines changed

python/ray/serve/_private/application_state.py

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import inspect
12
import json
23
import logging
34
import os
@@ -40,6 +41,7 @@
4041
override_runtime_envs_except_env_vars,
4142
validate_route_prefix,
4243
)
44+
from ray.serve.api import ASGIAppReplicaWrapper
4345
from ray.serve.config import AutoscalingConfig
4446
from ray.serve.exceptions import RayServeException
4547
from ray.serve.generated.serve_pb2 import (
@@ -468,6 +470,7 @@ def deploy_app(self, deployment_infos: Dict[str, DeploymentInfo]):
468470
or docs path.
469471
"""
470472

473+
self._check_ingress_deployments(deployment_infos)
471474
# Check routes are unique in deployment infos
472475
self._route_prefix, self._docs_path = self._check_routes(deployment_infos)
473476

@@ -704,6 +707,28 @@ def _reconcile_build_app_task(self) -> Tuple[Optional[Dict], BuildAppStatus, str
704707
)
705708
return None, BuildAppStatus.FAILED, error_msg
706709

710+
def _check_ingress_deployments(
711+
self, deployment_infos: Dict[str, DeploymentInfo]
712+
) -> None:
713+
"""Check @serve.ingress of deployments in app.
714+
715+
Raises: RayServeException if more than one @serve.ingress
716+
is found among deployments.
717+
"""
718+
num_ingress_deployments = 0
719+
for info in deployment_infos.values():
720+
if inspect.isclass(info.replica_config.deployment_def) and issubclass(
721+
info.replica_config.deployment_def, ASGIAppReplicaWrapper
722+
):
723+
num_ingress_deployments += 1
724+
725+
if num_ingress_deployments > 1:
726+
raise RayServeException(
727+
f'Found multiple FastAPI deployments in application "{self._name}".'
728+
"Please only include one deployment with @serve.ingress"
729+
"in your application to avoid this issue."
730+
)
731+
707732
def _check_routes(
708733
self, deployment_infos: Dict[str, DeploymentInfo]
709734
) -> Tuple[str, str]:
@@ -721,6 +746,9 @@ def _check_routes(
721746
num_route_prefixes = 0
722747
num_docs_paths = 0
723748
route_prefix = None
749+
# TODO(Ziy1-Tan): `docs_path` will be removed when
750+
# https://github.com/ray-project/ray/issues/53023 is resolved.
751+
# We can get it from DeploymentStateManager directly.
724752
docs_path = None
725753
for info in deployment_infos.values():
726754
# Update route prefix of application, which may be updated
@@ -1177,7 +1205,12 @@ def build_serve_application(
11771205
name=name,
11781206
default_runtime_env=ray.get_runtime_context().runtime_env,
11791207
)
1208+
num_ingress_deployments = 0
11801209
for deployment in built_app.deployments:
1210+
if inspect.isclass(deployment.func_or_class) and issubclass(
1211+
deployment.func_or_class, ASGIAppReplicaWrapper
1212+
):
1213+
num_ingress_deployments += 1
11811214
is_ingress = deployment.name == built_app.ingress_deployment_name
11821215
deploy_args_list.append(
11831216
get_deploy_args(
@@ -1190,6 +1223,12 @@ def build_serve_application(
11901223
docs_path=deployment._docs_path,
11911224
)
11921225
)
1226+
if num_ingress_deployments > 1:
1227+
return None, (
1228+
f'Found multiple FastAPI deployments in application "{built_app.name}". '
1229+
"Please only include one deployment with @serve.ingress "
1230+
"in your application to avoid this issue."
1231+
)
11931232
return deploy_args_list, None
11941233
except KeyboardInterrupt:
11951234
# Error is raised when this task is canceled with ray.cancel(), which
Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
from fastapi import FastAPI
2+
3+
from ray import serve
4+
from ray.serve.handle import DeploymentHandle
5+
6+
app1 = FastAPI()
7+
app2 = FastAPI()
8+
9+
10+
@serve.deployment
11+
@serve.ingress(app2)
12+
class SubModel:
13+
def add(self, a: int):
14+
return a + 1
15+
16+
17+
@serve.deployment
18+
@serve.ingress(app1)
19+
class Model:
20+
def __init__(self, submodel: DeploymentHandle):
21+
self.submodel = submodel
22+
23+
@app1.get("/{a}")
24+
async def func(self, a: int):
25+
return await self.submodel.add.remote(a)
26+
27+
28+
invalid_model = Model.bind(SubModel.bind())

python/ray/serve/tests/test_deploy_app.py

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,13 @@ def check_deployments_dead(deployment_ids: List[DeploymentID]):
5151
return all(f"ServeReplica::{p}" not in actor_names for p in prefixes)
5252

5353

54+
def check_deploy_failed(app_name: str, message: str):
55+
status = serve.status().applications[app_name]
56+
assert status.status == "DEPLOY_FAILED"
57+
assert message in status.message
58+
return True
59+
60+
5461
def get_test_config() -> Dict:
5562
return {"import_path": "ray.serve.tests.test_config_files.pizza.serve_dag"}
5663

@@ -127,9 +134,23 @@ def test_deploy_multi_app_basic(serve_instance):
127134
check_multi_app()
128135

129136

130-
def test_deploy_multi_app_update_config(serve_instance):
137+
def test_two_fastapi_in_one_application(serve_instance):
131138
client = serve_instance
139+
config = {
140+
"applications": [
141+
{
142+
"name": "app1",
143+
"route_prefix": "/app1",
144+
"import_path": "ray.serve.tests.test_config_files.multi_fastapi.invalid_model",
145+
}
146+
],
147+
}
148+
client.deploy_apps(ServeDeploySchema.parse_obj(config))
149+
wait_for_condition(check_deploy_failed, app_name="app1", message="FastAPI")
132150

151+
152+
def test_deploy_multi_app_update_config(serve_instance):
153+
client = serve_instance
133154
config = get_test_deploy_config()
134155
client.deploy_apps(ServeDeploySchema.parse_obj(config))
135156
check_multi_app()

python/ray/serve/tests/test_fastapi.py

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -727,15 +727,16 @@ def decr2(self):
727727

728728

729729
@pytest.mark.parametrize("two_fastapi", [True, False])
730+
@pytest.mark.parametrize("docs_url", ["/docs", None])
730731
def test_two_fastapi_in_one_application(
731-
serve_instance: ServeControllerClient, two_fastapi
732+
serve_instance: ServeControllerClient, two_fastapi, docs_url
732733
):
733734
"""
734735
Check that a deployment graph that would normally work, will not deploy
735736
successfully if there are two FastAPI deployments.
736737
"""
737-
app1 = FastAPI()
738-
app2 = FastAPI()
738+
app1 = FastAPI(docs_url=docs_url)
739+
app2 = FastAPI(docs_url=docs_url)
739740

740741
class SubModel:
741742
def add(self, a: int):

0 commit comments

Comments
 (0)