[Serve] Add initial support for FastAPI #14754

simon-mo · 2021-03-18T00:11:57Z

Add the most basic FastAPI support, passing the following test.

def test_fastapi_function(serve_instance):
    client = serve_instance
    app = FastAPI()

    @serve.deployment(app)
    @app.get("/{a}")
    def func(a: int):
        return {"result": a}

    client.deploy("f", func)

    resp = requests.get(f"http://localhost:8000/f/100")
    assert resp.json() == {"result": 100}

    resp = requests.get(f"http://localhost:8000/f/not-number")
    assert resp.status_code == 422  # Unprocessable Entity
    assert resp.json()["detail"][0]["type"] == "type_error.integer"

Related issue number

Checks

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 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 :(

simon-mo · 2021-03-18T00:22:44Z

python/ray/serve/config.py

@@ -71,8 +72,8 @@ class BackendConfig(BaseModel):
    max_concurrent_queries: Optional[int] = None
    user_config: Any = None

-    experimental_graceful_shutdown_wait_loop_s: PositiveFloat = 2.0
-    experimental_graceful_shutdown_timeout_s: confloat(ge=0) = 20.0


confloat is a private api

simon-mo · 2021-03-18T00:24:24Z

@edoakes @architkulkarni This PR should lay the ground work for subsequent PRs. I hope to get this most basic case merged and unblock other FastAPI support (middlewares, classes, dependency injection test) in parallel.

architkulkarni

Looks great, this is an exciting new feature

architkulkarni · 2021-03-18T02:10:03Z

python/ray/serve/backend_worker.py

+                            headers=dict(self.header))
+
+                sender = MockSender()
+                path_params = req.scope.pop("path_params")


Should we copy the scope, or does it not matter in this case? Just parroting https://asgi.readthedocs.io/en/latest/specs/main.html#middleware which says the scope should be copied before passing to a child.

If we don't copy the scope, we should leave a comment here explaining what the assumptions are that allow us to do that. This could lead to hard-to-debug errors in the future.

architkulkarni · 2021-03-18T05:34:49Z

python/ray/serve/utils.py

@@ -365,3 +365,42 @@ def get_current_node_resource_key() -> str:
                    return key
    else:
        raise ValueError("Cannot found the node dictionary for current node.")
+
+
+def register_custom_serializers():


It's great that you were able to find ways to serialize these!

edoakes

Nice, looks pretty straightforward once the pydantic serialization was sorted out. My main concern is around the custom path handling logic -- I don't think we should be doing any kind of manual stripping or we're sure to run into trouble. I think we should be able to use the following:
https://fastapi.tiangolo.com/advanced/behind-a-proxy/#proxy-with-a-stripped-path-prefix

edoakes · 2021-03-18T15:24:31Z

python/ray/serve/BUILD

+py_test(
+    name = "test_pydantic_serialization",
+    size = "small",
+    srcs = serve_tests_srcs,
+    tags = ["exclusive"],
+    deps = [":serve_lib"],
+)


In the future it'd be better to make a separate PR for this. Makes it easier to review and merged with fewer moving pieces.

edoakes · 2021-03-18T15:25:37Z

python/ray/serve/api.py

+    >>> @serve.deployment(app)
+        @app.get("/")


API still smells a little funny to me but we can discuss that offline. I will focus on the implementation for the review for now.

edoakes · 2021-03-18T15:26:34Z

python/ray/serve/backend_worker.py

@@ -256,7 +257,39 @@ async def invoke_single(self, request_item: Query) -> Any:

        start = time.time()
        try:
-            result = await method_to_call(arg)
+            if self.config.internal_metadata.is_asgi_app:


Please pull this logic out and add unit tests for it

it's a bit hard to pull out, I think we should do it after remove the batching part so there's only one "invoke", and then we can make it invoke & invoke_asgi

sure, sounds good. can you leave a note so we remember to clean it up when that happens?

edoakes · 2021-03-18T15:27:47Z

python/ray/serve/backend_worker.py

+                        if (message["type"] == "http.response.body"):
+                            self.buffer.append(message["body"])
+                        if (message["type"] == "http.response.start"):
+                            self.status_code = message["status"]
+                            for key, value in message["headers"]:
+                                self.header[key.decode()] = value.decode()


Are there other unhandled message types here? Please add an else clause that either does assert False if not, or raises a useful exception if there are unhandled types

edoakes · 2021-03-18T15:28:53Z

python/ray/serve/backend_worker.py

+                            headers=dict(self.header))
+
+                sender = MockSender()
+                path_params = req.scope.pop("path_params")


If we don't copy the scope, we should leave a comment here explaining what the assumptions are that allow us to do that. This could lead to hard-to-debug errors in the future.

edoakes · 2021-03-18T15:29:47Z

python/ray/serve/backend_worker.py

+                matched_path = path_params["wildcard"]
+                req.scope["path"] = f"/{matched_path}"


What exactly is this doing? This seems like something we shouldn't need to do and should be handled by FastAPI

edoakes · 2021-03-18T15:31:15Z

python/ray/serve/controller.py

+                methods = [
+                    "GET", "HEAD", "POST", "PUT", "DELETE", "CONNECT",
+                    "OPTIONS", "TRACE", "PATCH"
+                ]


In the future we can just transparently proxy all methods at the HTTP proxy and let the downstream application handle them. Can you leave a comment here mentioning that?

edoakes · 2021-03-18T15:32:07Z

python/ray/serve/controller.py

+            route = f"/{name}"
+            methods = ["GET", "POST"]
+            if replica_config.is_asgi_app:
+                route += "/{wildcard:path}"


Hmm I'm still not understanding this logic. Should we just need to configure the base path in FastAPI to /{name}?

edoakes · 2021-03-18T15:33:09Z

python/ray/serve/tests/test_fastapi.py

+from ray import serve
+
+
+def test_fastapi_function(serve_instance):


So cool!!!!

edoakes · 2021-03-19T16:04:18Z

python/ray/serve/api.py

@@ -719,6 +726,8 @@ def connect() -> Client:
    if not ray.is_initialized():
        ray.init()

+    register_custom_serializers()


we should probably upstream these into ray, right?

edoakes · 2021-03-19T16:04:45Z

python/ray/serve/api.py

+
+
+def ingress(
+        app: Union[FastAPI, APIRouter, None] = None,


Why is this optional? What is the behavior when it isn't specified?

edoakes

Looks mostly good, a few more comments. Biggest question: didn't we conclude that we only need to handle the class case and not the function case? If the user is passing in a full FastAPI app, we wrap it in a "dummy" class. This PR seems to only be handling the function case.

edoakes · 2021-03-19T16:05:58Z

python/ray/serve/api.py

+        @wraps(f)
+        def inner(*args, **kwargs):
+            return f(*args, **kwargs)


Don't we only need to handle classes here? This looks like it only handles functions?

edoakes · 2021-03-19T16:06:25Z

python/ray/serve/backend_worker.py

@@ -256,7 +257,39 @@ async def invoke_single(self, request_item: Query) -> Any:

        start = time.time()
        try:
-            result = await method_to_call(arg)
+            if self.config.internal_metadata.is_asgi_app:


sure, sounds good. can you leave a note so we remember to clean it up when that happens?

edoakes · 2021-03-19T16:08:53Z

python/ray/serve/controller.py

+        if replica_config.path_prefix is None:
+            replica_config.path_prefix = f"/{name}"
+            # Updated here because it is used by backend worker
+            backend_config.internal_metadata.path_prefix = f"/{name}"
+        route = replica_config.path_prefix
+        methods = ["GET", "POST"]
+        if replica_config.is_asgi_app:
+            # When the backend is asgi application, we want to proxy it
+            # with a prefixed path as well as proxy all HTTP methods.
+            # {wildcard:path} is used so HTTPProxy's Starlette router can match
+            # arbitrary path.
+            route = "/{route}/{wildcard:path}"
+            # https://developer.mozilla.org/en-US/docs/Web/HTTP/Methods
+            methods = [
+                "GET", "HEAD", "POST", "PUT", "DELETE", "CONNECT", "OPTIONS",
+                "TRACE", "PATCH"
+            ]


I'm a bit confused at this logic overall. Shouldn't there only be a single branch here?

if replica_config.is_ingress: route = replica_config.path_prefix or f"{name}" methods = [...] # all

we still need to set the default path prefix using the backend name, updated the code to make it cleaner.

edoakes · 2021-03-19T16:10:16Z

python/ray/serve/backend_worker.py

+                # The incoming scope["path"] contains prefixed path and it
+                # won't be stripped by FastAPI.
+                request.scope["path"] = scope["path"].replace(root_path, "", 1)
+                # root_path is used such that the reverse look up and
+                # redirection works.
+                request.scope["root_path"] = root_path


Hmm why won't this be handled by fastapi? It still seems like we shouldn't need to be mucking around with the fields directly

root_path only works for the redirects, it actually is not used by routing. FastAPI expects the incoming route is cleaned and root_path is stripped by proxy already. We are essentially performing the path stripping here.

Ok, cool, makes sense

edoakes · 2021-03-19T16:10:36Z

python/ray/serve/backend_worker.py

+                app = self.callable._serve_asgi_app
+                sender = ASGIHTTPSender()
+                await app(
+                    request.scope,
+                    request._receive,
+                    sender,
+                )
+                result = sender.build_starlette_response()


Suggested change

app = self.callable._serve_asgi_app

sender = ASGIHTTPSender()

await app(

request.scope,

request._receive,

sender,

)

result = sender.build_starlette_response()

sender = ASGIHTTPSender()

await self.callable._serve_asgi_app(

request.scope,

request._receive,

sender,

)

result = sender.build_starlette_response()

edoakes · 2021-03-20T18:08:20Z

python/ray/serve/utils.py

+    # Pydantic's Cython validators are not serializable.
+    # https://github.com/cloudpipe/cloudpickle/issues/408
+    ray.worker.global_worker.run_function_on_all_workers(
+        lambda _: ray.util.register_serializer(
+            pydantic.fields.ModelField,
+            serializer=lambda o: {
+                "name": o.name,
+                "type_": o.type_,
+                "class_validators": o.class_validators,
+                "model_config": o.model_config,
+                "default": o.default,
+                "default_factory": o.default_factory,
+                "required": o.required,
+                "alias": o.alias,
+                "field_info": o.field_info,
+            },
+            deserializer=lambda kwargs: pydantic.fields.ModelField(**kwargs),


@simon-mo can we upstream this into Ray so we don't need to dynamically register this callback?

simon-mo added 2 commits March 17, 2021 17:00

[WIP] FastAPI support in Ray Serve

d811add

lint

fb47beb

simon-mo requested review from edoakes and architkulkarni March 18, 2021 00:12

simon-mo commented Mar 18, 2021

View reviewed changes

comments

abbcd35

architkulkarni approved these changes Mar 18, 2021

View reviewed changes

edoakes requested changes Mar 18, 2021

View reviewed changes

comments

123b9c9

simon-mo requested a review from edoakes March 18, 2021 21:13

simon-mo assigned edoakes Mar 18, 2021

add fastapi to dep

51a46a9

edoakes reviewed Mar 19, 2021

View reviewed changes

edoakes added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Mar 19, 2021

edoakes added this to the [serve] v2 API milestone Mar 19, 2021

simon-mo added 3 commits March 19, 2021 17:33

comments

1fdb5cc

Merge branch 'master' of github.com:ray-project/ray into serve-fastapi

f3f4ca8

Fix

8930903

simon-mo removed the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Mar 20, 2021

simon-mo requested a review from edoakes March 20, 2021 00:46

fix test

834ded3

simon-mo added the tests-ok The tagger certifies test failures are unrelated and assumes personal liability. label Mar 20, 2021

edoakes reviewed Mar 20, 2021

View reviewed changes

edoakes approved these changes Mar 20, 2021

View reviewed changes

edoakes merged commit 91b9928 into ray-project:master Mar 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Serve] Add initial support for FastAPI #14754

[Serve] Add initial support for FastAPI #14754

simon-mo commented Mar 18, 2021

simon-mo Mar 18, 2021

simon-mo commented Mar 18, 2021

architkulkarni left a comment

architkulkarni Mar 18, 2021

edoakes Mar 18, 2021

architkulkarni Mar 18, 2021

edoakes left a comment

edoakes Mar 18, 2021

edoakes Mar 18, 2021

edoakes Mar 18, 2021

simon-mo Mar 18, 2021

edoakes Mar 19, 2021

edoakes Mar 18, 2021

edoakes Mar 18, 2021

edoakes Mar 18, 2021

edoakes Mar 18, 2021

edoakes Mar 18, 2021

edoakes Mar 18, 2021

edoakes Mar 19, 2021

edoakes Mar 19, 2021

edoakes left a comment

edoakes Mar 19, 2021

edoakes Mar 19, 2021

edoakes Mar 19, 2021

simon-mo Mar 20, 2021

edoakes Mar 19, 2021

simon-mo Mar 20, 2021

edoakes Mar 20, 2021

edoakes Mar 19, 2021

edoakes Mar 20, 2021

		matched_path = path_params["wildcard"]
		req.scope["path"] = f"/{matched_path}"

		from ray import serve


		def test_fastapi_function(serve_instance):

[Serve] Add initial support for FastAPI #14754

[Serve] Add initial support for FastAPI #14754

Conversation

simon-mo commented Mar 18, 2021

Related issue number

Checks

Choose a reason for hiding this comment

simon-mo commented Mar 18, 2021

architkulkarni left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edoakes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edoakes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment