[Serve] Fix `make_fastapi_class_based_view` for inherited methods by kouroshHakha · Pull Request #59410 · ray-project/ray

kouroshHakha · 2025-12-12T18:55:38Z

Summary

Fixed a bug in Ray Serve where inheriting from a class with FastAPI-decorated methods would cause FastAPI to treat self as a required query parameter, resulting in 400 Bad Request errors:

{"error":{"message":"Invalid request format: Field required at ('query', 'self')"}}

This affected any Serve user who inherits decorated methods, including Ray LLM's OpenAiIngress.

Root Cause

When a class inherits from another class with FastAPI-decorated methods, inherited methods retain their parent class's __qualname__:

class Parent:
    @app.get("/endpoint")
    def my_method(self): ...

class Child(Parent):
    pass  # my_method.__qualname__ is still "Parent.my_method"

In make_fastapi_class_based_view, the check to identify class methods was:

cls.__qualname__ in route.endpoint.__qualname__

This fails for inherited methods because "Child" is not in "Parent.my_method". As a result, these routes are not transformed, and FastAPI treats self as a required query parameter.

Fix

Updated make_fastapi_class_based_view in ray/serve/_private/http_util.py to check the entire MRO (Method Resolution Order) instead of just the immediate class:

# Before (broken for inheritance):
cls.__qualname__ in route.endpoint.__qualname__

# After (handles inheritance correctly):
any(
    base.__qualname__ in route.endpoint.__qualname__
    for base in cls.__mro__
    if base is not object
)

This ensures inherited methods from any ancestor class are properly recognized and transformed.

Testing

Added test_ingress_inheritance to ray/serve/tests/test_fastapi.py that verifies:

Multi-level inheritance: Grandparent → Parent → Child → ServedIngress
Direct inheritance: BaseIngress → DirectServedIngress

Both cases test that inherited endpoints work correctly via actual HTTP requests (not mock/unit tests).

Fixed a bug where subclassing OpenAiIngress and using make_fastapi_ingress would cause FastAPI to treat 'self' as a required query parameter. The issue was that inherited methods retained their parent class's __qualname__, causing make_fastapi_class_based_view to skip them during route transformation. The fix updates each decorated method's __qualname__ to match the new class so that make_fastapi_class_based_view properly recognizes all methods. Signed-off-by: Kourosh Hakhamaneshi <kourosh@anyscale.com>

gemini-code-assist

Code Review

This pull request effectively addresses a subtle bug in make_fastapi_ingress concerning inherited methods and their __qualname__. The fix is clean and well-commented. The accompanying tests are thorough, covering various inheritance scenarios, which significantly improves confidence in the solution. I have a few suggestions to make the new tests even more precise by using exact-match assertions for __qualname__ instead of substring checks. Overall, this is a great contribution.

gemini-code-assist · 2025-12-12T18:56:58Z

python/ray/llm/tests/serve/cpu/deployments/routers/test_make_fastapi_ingress.py

+            assert "MyCustomIngress" in method.__qualname__, (
+                f"Method {method_name} should have MyCustomIngress in __qualname__, "
+                f"but got {method.__qualname__}"
+            )


The assertion "MyCustomIngress" in method.__qualname__ is good, but we can make it more precise. Asserting for the exact __qualname__ ensures that the naming is constructed exactly as expected, making the test more robust against future changes. This would also provide a clearer failure message if the __qualname__ format changes unexpectedly.

Suggested change

assert "MyCustomIngress" in method.__qualname__, (

f"Method {method_name} should have MyCustomIngress in __qualname__, "

f"but got {method.__qualname__}"

)

expected_qualname = f"{MyCustomIngress.__qualname__}.{method_name}"

assert method.__qualname__ == expected_qualname, (

f"Method {method_name} should have __qualname__ '{expected_qualname}', "

f"but got '{method.__qualname__}'"

)

gemini-code-assist · 2025-12-12T18:56:58Z

python/ray/llm/tests/serve/cpu/deployments/routers/test_make_fastapi_ingress.py

+
+        # Check custom method has correct qualname
+        custom_method = ingress_cls.custom_endpoint
+        assert "MyCustomIngress" in custom_method.__qualname__


For a more precise test, it's better to assert the exact __qualname__ instead of checking for a substring. This ensures the name is constructed exactly as expected and provides a more informative failure message.

Suggested change

assert "MyCustomIngress" in custom_method.__qualname__

assert custom_method.__qualname__ == f"{MyCustomIngress.__qualname__}.custom_endpoint"

gemini-code-assist · 2025-12-12T18:56:58Z

python/ray/llm/tests/serve/cpu/deployments/routers/test_make_fastapi_ingress.py

+
+        # Check inherited methods also have correct qualname
+        completions_method = ingress_cls.completions
+        assert "MyCustomIngress" in completions_method.__qualname__


Similar to the previous comment, asserting the exact __qualname__ for this inherited method will make the test more robust.

Suggested change

assert "MyCustomIngress" in completions_method.__qualname__

assert completions_method.__qualname__ == f"{MyCustomIngress.__qualname__}.completions"

gemini-code-assist · 2025-12-12T18:56:58Z

python/ray/llm/tests/serve/cpu/deployments/routers/test_make_fastapi_ingress.py

+            assert "FinalIngress" in method.__qualname__, (
+                f"Method {method_name} should have FinalIngress in __qualname__, "
+                f"but got {method.__qualname__}"
+            )


To improve test precision, it's better to assert the exact __qualname__ rather than using a substring check. This will catch any unintended changes to the __qualname__ construction for deeply nested classes.

Suggested change

assert "FinalIngress" in method.__qualname__, (

f"Method {method_name} should have FinalIngress in __qualname__, "

f"but got {method.__qualname__}"

)

expected_qualname = f"{FinalIngress.__qualname__}.{method_name}"

assert method.__qualname__ == expected_qualname, (

f"Method {method_name} should have __qualname__ '{expected_qualname}', "

f"but got '{method.__qualname__}'"

)

cursor · 2025-12-12T19:05:20Z

python/ray/llm/tests/serve/cpu/deployments/routers/test_make_fastapi_ingress.py

+
+        # Check that the class qualname matches the input class qualname
+        # (classes defined inside methods have full path in qualname)
+        assert ingress_cls.__qualname__ == MyCustomIngress.__qualname__


Bug: Test assertion will fail due to unset __qualname__

The assertion ingress_cls.__qualname__ == MyCustomIngress.__qualname__ will fail. The make_fastapi_ingress function returns the result of serve.ingress(app)(new_cls), which is an ASGIIngressWrapper class. Looking at serve.ingress in python/ray/serve/api.py, only ASGIIngressWrapper.__name__ is explicitly set (to cls.__name__), but __qualname__ is not set. The ASGIIngressWrapper class gets its default __qualname__ from where it's defined inside the decorator function, which will be something like "ingress.<locals>.decorator.<locals>.ASGIIngressWrapper" — not equal to MyCustomIngress.__qualname__.

eicherseiji · 2025-12-12T19:27:32Z

This behavior is surprising. Is it something normal Serve users could hit when inheriting decorated methods? I.e. any reason we aren't targeting a fix in Serve?

kouroshHakha · 2025-12-12T19:33:19Z

Good point @eicherseiji. Let me see what the fix would look like from serve's perspective. I may have fixed it in the wrong layer.

Signed-off-by: Kourosh Hakhamaneshi <kourosh@anyscale.com>

kouroshHakha · 2025-12-12T21:59:00Z

/gemini review

gemini-code-assist

Code Review

This pull request addresses a bug in Ray Serve where inherited FastAPI-decorated methods were not being correctly identified, causing self to be treated as a query parameter. The fix cleverly resolves this by inspecting the class's full Method Resolution Order (MRO) instead of just the class itself. The change is well-supported by new unit tests for make_fastapi_ingress and new integration tests that validate both multi-level and direct inheritance scenarios. The code quality is high, and the fix is robust. I have one minor suggestion to make a new test assertion more specific.

gemini-code-assist · 2025-12-12T22:00:31Z

python/ray/llm/tests/serve/cpu/deployments/routers/test_make_fastapi_ingress.py

+        ingress_cls = make_fastapi_ingress(
+            MyCustomIngress, endpoint_map=custom_endpoints
+        )
+
+        # Verify the class was created
+        assert ingress_cls is not None


The assertion assert ingress_cls is not None is a bit weak. To make this test more robust and directly verify the functionality, consider also asserting that the custom route (/custom) has been correctly registered on the FastAPI application. This will confirm that make_fastapi_ingress is handling the custom method as expected.

Suggested change

ingress_cls = make_fastapi_ingress(

MyCustomIngress, endpoint_map=custom_endpoints

)

# Verify the class was created

assert ingress_cls is not None

app = FastAPI()

ingress_cls = make_fastapi_ingress(

MyCustomIngress, endpoint_map=custom_endpoints, app=app

)

# Verify the class was created and the route is registered

assert ingress_cls is not None

route_paths = [route.path for route in app.routes if isinstance(route, APIRoute)]

assert "/custom" in route_paths

Signed-off-by: Kourosh Hakhamaneshi <kourosh@anyscale.com>

cursor · 2025-12-13T00:03:30Z

python/ray/serve/_private/http_util.py

+            route.endpoint.__qualname__.startswith(base.__qualname__ + ".")
+            for base in cls.__mro__
+            if base is not object
+        )


Bug: Nested class routes incorrectly matched via MRO prefix

The new prefix matching logic can incorrectly match routes from nested classes. If a parent class Parent contains a nested class Parent.Nested with decorated methods, and a Child(Parent) class is served, the check route.endpoint.__qualname__.startswith("Parent.") would match "Parent.Nested.method" even though Parent.Nested is not in Child's MRO. The old substring check cls.__qualname__ in route.endpoint.__qualname__ would correctly reject this case. This could cause routes from unrelated nested classes to have incorrect self injection when a class inheriting from their container is served.

eicherseiji · 2025-12-13T00:13:33Z

python/ray/llm/_internal/serve/core/ingress/ingress.py

-    # (it checks if cls.__qualname__ is in route.endpoint.__qualname__).
+    # We keep the same __name__ and __qualname__ as the original class
+    # so that the new class properly represents the input class.
    new_cls = type(cls.__name__, (cls,), class_dict)


Are these type assignments still needed?

not strictly anymore, but I kept them here, because they keep the repr(cls) name nicer.

harshit-anyscale · 2025-12-15T08:18:30Z

serve-side changes looks good

one ques though: how does fastAPI resolves if the parent and child class have a same route? who will take preference - parent or child? or error? and do we replicate that same behavior in our code?

abrarsheikh

lg2m

kouroshHakha · 2025-12-15T17:27:38Z

@harshit-anyscale

one ques though: how does fastAPI resolves if the parent and child class have a same route? who will take preference - parent or child? or error? and do we replicate that same behavior in our code?

It certainly doesn't error. Fast API registers both but will match the first registered one (in this case the parent).

In our code we follow python's MRO logic to decide who to register (only one will be registered and that'd be the child) So it is a bit different. Example:

from fastapi import FastAPI
from fastapi.routing import APIRoute

print('=== NATIVE FASTAPI (decorators on both parent & child) ===')
app1 = FastAPI()

class Parent1:
    @app1.get('/test')
    def method(self):
        return 'parent'

class Child1(Parent1):
    @app1.get('/test')  # Same path, override
    def method(self):
        return 'child'

routes1 = [(r.path, r.endpoint.__qualname__) for r in app1.routes if isinstance(r, APIRoute)]
print(f'Routes: {routes1}')
print(f'Count: {len(routes1)}')
print('-> BOTH registered, PARENT wins (first match)')
print()

print('=== OUR CODE (make_fastapi_ingress approach) ===')
app2 = FastAPI()

class Parent2:
    def method(self):
        return 'parent'

class Child2(Parent2):
    def method(self):  # Override
        return 'child'

# What make_fastapi_ingress does:
endpoint_map = {'method': lambda app: app.get('/test')}
for method_name, route_factory in endpoint_map.items():
    original_method = getattr(Child2, method_name)  # MRO resolution
    route_factory(app2)(original_method)

routes2 = [(r.path, r.endpoint.__qualname__) for r in app2.routes if isinstance(r, APIRoute)]
print(f'Routes: {routes2}')
print(f'Count: {len(routes2)}')
print('-> Only ONE registered, CHILD wins (MRO resolution)')

Output:

=== NATIVE FASTAPI (decorators on both parent & child) ===
Routes: [('/test', 'Parent1.method'), ('/test', 'Child1.method')]
Count: 2
-> BOTH registered, PARENT wins (first match)

=== OUR CODE (make_fastapi_ingress approach) ===
Routes: [('/test', 'Child2.method')]
Count: 1
-> Only ONE registered, CHILD wins (MRO resolution)

I think our behavior is more correct because it follows the class inheritance semantics.

…y-project#59410) Signed-off-by: Kourosh Hakhamaneshi <kourosh@anyscale.com> Signed-off-by: kriyanshii <kriyanshishah06@gmail.com>

…y-project#59410) Signed-off-by: Kourosh Hakhamaneshi <kourosh@anyscale.com>

…y-project#59410) Signed-off-by: Kourosh Hakhamaneshi <kourosh@anyscale.com> Signed-off-by: peterxcli <peterxcli@gmail.com>

kouroshHakha added the go add ONLY when ready to merge, run all tests label Dec 12, 2025

kouroshHakha marked this pull request as ready for review December 12, 2025 18:55

kouroshHakha requested a review from a team as a code owner December 12, 2025 18:55

gemini-code-assist bot reviewed Dec 12, 2025

View reviewed changes

ray-gardener bot added serve Ray Serve Related Issue llm labels Dec 12, 2025

cursor bot reviewed Dec 12, 2025

View reviewed changes

wip

08d9959

Signed-off-by: Kourosh Hakhamaneshi <kourosh@anyscale.com>

kouroshHakha changed the title ~~[Serve][LLM] Fix make_fastapi_ingress for inherited classes~~ [Serve] Fix make_fastapi_class_based_view for inherited methods Dec 12, 2025

wip

e14a7f0

Signed-off-by: Kourosh Hakhamaneshi <kourosh@anyscale.com>

kouroshHakha requested a review from a team as a code owner December 12, 2025 21:58

gemini-code-assist bot reviewed Dec 12, 2025

View reviewed changes

wip

76e1419

Signed-off-by: Kourosh Hakhamaneshi <kourosh@anyscale.com>

kouroshHakha mentioned this pull request Dec 12, 2025

[Serve][LLM] Add dispatch API for control plane operations across all replicas #59373

Merged

kouroshHakha added 2 commits December 12, 2025 15:50

Wip

fe9654f

Signed-off-by: Kourosh Hakhamaneshi <kourosh@anyscale.com>

wip

848feac

Signed-off-by: Kourosh Hakhamaneshi <kourosh@anyscale.com>

cursor bot reviewed Dec 13, 2025

View reviewed changes

eicherseiji reviewed Dec 13, 2025

View reviewed changes

abrarsheikh approved these changes Dec 15, 2025

View reviewed changes

kouroshHakha enabled auto-merge (squash) December 15, 2025 17:33

eicherseiji approved these changes Dec 15, 2025

View reviewed changes

kouroshHakha merged commit abdc820 into ray-project:master Dec 15, 2025
7 checks passed

Yicheng-Lu-llll pushed a commit to Yicheng-Lu-llll/ray that referenced this pull request Dec 22, 2025

[Serve] Fix make_fastapi_class_based_view for inherited methods (ra…

daf6ed5

…y-project#59410) Signed-off-by: Kourosh Hakhamaneshi <kourosh@anyscale.com>

-            assert "MyCustomIngress" in method.__qualname__, (
-                f"Method {method_name} should have MyCustomIngress in __qualname__, "
-                f"but got {method.__qualname__}"
-            )
+            expected_qualname = f"{MyCustomIngress.__qualname__}.{method_name}"
+            assert method.__qualname__ == expected_qualname, (
+                f"Method {method_name} should have __qualname__ '{expected_qualname}', "
+                f"but got '{method.__qualname__}'"
+            )

	assert "MyCustomIngress" in custom_method.__qualname__
	assert custom_method.__qualname__ == f"{MyCustomIngress.__qualname__}.custom_endpoint"

	assert "MyCustomIngress" in completions_method.__qualname__
	assert completions_method.__qualname__ == f"{MyCustomIngress.__qualname__}.completions"

Conversation

kouroshHakha commented Dec 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Root Cause

Fix

Testing

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Dec 12, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Dec 12, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Dec 12, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Dec 12, 2025

Choose a reason for hiding this comment

Uh oh!

cursor bot Dec 12, 2025

Choose a reason for hiding this comment

Bug: Test assertion will fail due to unset __qualname__

Uh oh!

eicherseiji commented Dec 12, 2025

Uh oh!

kouroshHakha commented Dec 12, 2025

Uh oh!

kouroshHakha commented Dec 12, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Dec 12, 2025

Choose a reason for hiding this comment

Uh oh!

cursor bot Dec 13, 2025

Choose a reason for hiding this comment

Bug: Nested class routes incorrectly matched via MRO prefix

Uh oh!

eicherseiji Dec 13, 2025

Choose a reason for hiding this comment

Uh oh!

kouroshHakha Dec 13, 2025

Choose a reason for hiding this comment

Uh oh!

harshit-anyscale commented Dec 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

abrarsheikh left a comment

Choose a reason for hiding this comment

Uh oh!

kouroshHakha commented Dec 15, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kouroshHakha commented Dec 12, 2025 •

edited

Loading

Bug: Test assertion will fail due to unset `qualname`

harshit-anyscale commented Dec 15, 2025 •

edited

Loading