-
Notifications
You must be signed in to change notification settings - Fork 6.8k
[Serve] Check multiple FastAPI ingress deployments in a single application #53647
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR enhances Ray Serve’s application state logic to detect multiple FastAPI ingress deployments via @serve.ingress
rather than relying on docs_path
.
- Import
ASGIAppReplicaWrapper
and use it to count ingress deployments. - Update
_check_routes
to raise if more than one ingress deployment is found. - Revise exception messaging and docstring to reflect the new check.
Comments suppressed due to low confidence (1)
python/ray/serve/_private/application_state.py:710
- The docstring’s
Returns
section still referencesdocs_path
. Consider updating it to clarify that the function returns(route_prefix, docs_path)
and thatdocs_path
is retained for backward compatibility.
"""Check route prefixes and @serve.ingress of deployments in app.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These adjacent string literals concatenate without a space, producing @serve.ingressin
. Add a trailing space in the first string or a leading space in the second to fix the message formatting.
"Please only include one deployment with @serve.ingress" | |
"Please only include one deployment with @serve.ingress " |
Copilot uses AI. Check for mistakes.
Hey, @abrarsheikh . Do you know why this test case fails? It seems not to be related to my code changes. Serve test is blocked by test_delete_multi_app: (myenv) simple@wsl2-ubuntu:~/c/p/r/python[multi_app]> pytest -vs ray/dashboard/modules/serve/tests/test_serve_dashboard.py::test_delete_multi_app
"NUMEXPR_MAX_THREADS" not set, so enforcing safe limit of 8.
2025-06-08 21:46:22,213 - INFO - NumExpr defaulting to 8 threads.
2025-06-08 21:46:23,578 - INFO - Note: NumExpr detected 12 cores but "NUMEXPR_MAX_THREADS" not set, so enforcing safe limit of 8.
2025-06-08 21:46:23,578 - INFO - NumExpr defaulting to 8 threads.
*** Starting Iteration 1/2 ***
Sending PUT request for config.
PUT request sent successfully.
――――――――――――――――――――――――――――――――――――――――― test_delete_multi_app ―――――――――――――――――――――――――――――――――――――――――
ray_start_stop = None
@pytest.mark.skipif(
sys.platform == "darwin" and not TEST_ON_DARWIN, reason="Flaky on OSX."
)
def test_delete_multi_app(ray_start_stop):
py_module = (
"https://github.com/ray-project/test_module/archive/"
"aa6f366f7daa78c98408c27d917a983caa9f888b.zip"
)
config = {
"applications": [
{
"name": "app1",
"route_prefix": "/app1",
"import_path": "dir.subdir.a.add_and_sub.serve_dag",
"runtime_env": {
"working_dir": (
"https://github.com/ray-project/test_dag/archive/"
"78b4a5da38796123d9f9ffff59bab2792a043e95.zip"
)
},
"deployments": [
{
"name": "Subtract",
"ray_actor_options": {
"runtime_env": {"py_modules": [py_module]}
},
}
],
},
{
"name": "app2",
"route_prefix": "/app2",
"import_path": "ray.serve.tests.test_config_files.world.DagNode",
},
],
}
# Ensure the REST API is idempotent
num_iterations = 2
for iteration in range(1, num_iterations + 1):
print(f"*** Starting Iteration {iteration}/{num_iterations} ***\n")
print("Sending PUT request for config.")
deploy_config_multi_app(config, SERVE_HEAD_URL)
> wait_for_condition(
lambda: requests.post("http://localhost:8000/app1", json=["ADD", 1]).text
== "2",
timeout=15,
)
ray/dashboard/modules/serve/tests/test_serve_dashboard.py:240:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
condition_predictor = <function test_delete_multi_app.<locals>.<lambda> at 0x7fe56c8e1700>, timeout = 15
retry_interval_ms = 100, raise_exceptions = False, kwargs = {}, start = 1749390391.7724278
last_ex = None, message = "The condition wasn't met before the timeout expired."
def wait_for_condition(
condition_predictor,
timeout=10,
retry_interval_ms=100,
raise_exceptions=False,
**kwargs: Any,
):
"""Wait until a condition is met or time out with an exception.
Args:
condition_predictor: A function that predicts the condition.
timeout: Maximum timeout in seconds.
retry_interval_ms: Retry interval in milliseconds.
raise_exceptions: If true, exceptions that occur while executing
condition_predictor won't be caught and instead will be raised.
**kwargs: Arguments to pass to the condition_predictor.
Raises:
RuntimeError: If the condition is not met before the timeout expires.
"""
start = time.time()
last_ex = None
while time.time() - start <= timeout:
try:
if condition_predictor(**kwargs):
return
except Exception:
if raise_exceptions:
raise
last_ex = ray._private.utils.format_error_message(traceback.format_exc())
time.sleep(retry_interval_ms / 1000.0)
message = "The condition wasn't met before the timeout expired."
if last_ex is not None:
message += f" Last exception: {last_ex}"
> raise RuntimeError(message)
E RuntimeError: The condition wasn't met before the timeout expired.
ray/_private/test_utils.py:615: RuntimeError |
Most likely, the master is broken. Give it a few days foe someone to fix it, then you can pull master into your branch to fix the broken test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know that the old implementation abused this function, but let's extract out the logic for checking duplicate ASGI apps outside of this function.
4222805
to
f4d7566
Compare
@abrarsheikh, It is wired that running a single test ace
|
cc @abrarsheikh @zcin . Do you have any idea about it? |
master seems good
```
❯ pytest -vs python/ray/dashboard/modules/serve/tests/test_serve_dashboard.py
Test session starts (platform: linux, Python 3.9.21, pytest 7.4.4, pytest-sugar 0.9.5)
cachedir: .pytest_cache
rootdir: /home/ubuntu/ray
configfile: pytest.ini
plugins: docker-tools-3.1.3, timeout-2.1.0, asyncio-0.17.2, forked-1.4.0, virtualenv-1.7.0, sugar-0.9.5, shutil-1.7.0, aiohttp-1.1.0, httpserver-1.0.6, nbval-0.11.0, anyio-3.7.1, sphinx-0.5.1.dev0, rerunfailures-11.1.2, lazy-fixture-0.6.3
timeout: 180.0s
timeout method: signal
timeout func_only: False
asyncio: mode=auto
collecting ... 2025-06-23 16:11:48,908 - INFO - NumExpr defaulting to 8 threads.
2025-06-23 16:11:50,464 - INFO - NumExpr defaulting to 8 threads.
*** Starting Iteration 1/2 ***
Sending PUT request for config1. Sending PUT request for config2. Sending PUT request for config3. *** Starting Iteration 2/2 *** Sending PUT request for config1. Sending PUT request for config2. Sending PUT request for config3. 2025-06-23 16:12:11,299 - INFO - NumExpr defaulting to 8 threads. 2025-06-23 16:12:19,255 - INFO - NumExpr defaulting to 8 threads. 2025-06-23 16:12:27,374 - INFO - NumExpr defaulting to 8 threads. 2025-06-23 16:12:35,480 - INFO - NumExpr defaulting to 8 threads. Sending PUT request for config. Sending DELETE request for config. *** Starting Iteration 2/2 *** Sending PUT request for config. Sending DELETE request for config. 2025-06-23 16:13:00,779 - INFO - NumExpr defaulting to 8 threads. 2025-06-23 16:13:08,938 - INFO - NumExpr defaulting to 8 threads. 2025-06-23 16:13:21,414 - INFO - NumExpr defaulting to 8 threads. 2025-06-23 16:13:34,133 - INFO - NumExpr defaulting to 8 threads. All applications are in a RUNNING state. 2025-06-23 16:13:48,140 - INFO - NumExpr defaulting to 8 threads. Results (123.63s):
|
nice work handling the imperative and declarative code paths. I think its worth adding tests for both flows. |
…ation Signed-off-by: Ziy1-Tan <[email protected]>
Signed-off-by: Ziy1-Tan <[email protected]>
…ation (ray-project#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 ray-project#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 <[email protected]> Signed-off-by: ChanChan Mao <[email protected]>
…ation (ray-project#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 ray-project#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 <[email protected]> Signed-off-by: jugalshah291 <[email protected]>
Why are these changes needed?
Related issue number
Closes #53024
Checks
git commit -s
) in this PR.scripts/format.sh
to lint the changes in this PR.method in Tune, I've added it in
doc/source/tune/api/
under thecorresponding
.rst
file.