-
Notifications
You must be signed in to change notification settings - Fork 57
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
feat: fal.App
for multiple endpoints
#27
Conversation
6724ce2
to
d8b540d
Compare
Is this how you deploy?
|
no, that's how you start the local test server (compared to just calling it we have now), the deployment flow is the same:
|
How come |
There is a set of generic parameters (the same set of arguments we have at Open for comments on this, we can have a single way to configure everything (but it might become super cluttered due to requirements) OR have two different segments which configure two different things (the current way). |
So, when testing locally, I might need to test the fal.endpoints locally as well. How would we do this? |
Given the application above, when developing, you just edit your code and run |
43ea98a
to
63347db
Compare
1adae46
to
31f9c63
Compare
31f9c63
to
6340ecb
Compare
fal.App
for multiple endpointsfal.App
for multiple endpoints
def marker_fn(callable: EndpointT) -> EndpointT: | ||
if hasattr(callable, "route_signature"): | ||
raise ValueError( | ||
f"Can't set multiple routes for the same function: {callable.__name__}" |
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.
Why is that?
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.
we don't handle it yet, but in the future I want to be able to support multiple endpoints for a single function by stacking @fal.endpoint
. this is done to reserve that use case (instead of treating it as an override which we would need to break)
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.
aha ok 👍
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.
great idea and work!
def _build_app(self) -> FastAPI: | ||
from fastapi import FastAPI | ||
from fastapi.middleware.cors import CORSMiddleware | ||
|
||
_app = FastAPI() | ||
|
||
_app.add_middleware( | ||
CORSMiddleware, | ||
allow_credentials=True, | ||
allow_headers=("*"), | ||
allow_methods=("*"), | ||
allow_origins=("*"), | ||
) | ||
|
||
routes: dict[RouteSignature, Callable[..., Any]] = { | ||
signature: endpoint | ||
for _, endpoint in inspect.getmembers(self, inspect.ismethod) | ||
if (signature := getattr(endpoint, "route_signature", None)) | ||
} | ||
if not routes: | ||
raise ValueError("An application must have at least one route!") | ||
|
||
for signature, endpoint in routes.items(): | ||
_app.add_api_route( | ||
signature.path, | ||
endpoint, | ||
name=endpoint.__name__, | ||
methods=["POST"], | ||
) | ||
|
||
return _app | ||
|
||
def openapi(self) -> dict[str, Any]: | ||
""" | ||
Build the OpenAPI specification for the served function. | ||
Attach needed metadata for a better integration to fal. | ||
""" | ||
app = self._build_app() | ||
spec = app.openapi() | ||
self._mark_order_openapi(spec) | ||
return spec | ||
|
||
def _mark_order_openapi(self, spec: dict[str, Any]): | ||
""" | ||
Add x-fal-order-* keys to the OpenAPI specification to help the rendering of UI. | ||
|
||
NOTE: We rely on the fact that fastapi and Python dicts keep the order of properties. | ||
""" | ||
|
||
def mark_order(obj: dict[str, Any], key: str): | ||
obj[f"x-fal-order-{key}"] = list(obj[key].keys()) | ||
|
||
mark_order(spec, "paths") | ||
|
||
def order_schema_object(schema: dict[str, Any]): | ||
""" | ||
Mark the order of properties in the schema object. | ||
They can have 'allOf', 'properties' or '$ref' key. | ||
""" | ||
if "allOf" in schema: | ||
for sub_schema in schema["allOf"]: | ||
order_schema_object(sub_schema) | ||
if "properties" in schema: | ||
mark_order(schema, "properties") | ||
|
||
for key in spec["components"].get("schemas") or {}: | ||
order_schema_object(spec["components"]["schemas"][key]) | ||
|
||
return spec |
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.
this whole thing already existed for serve functions, can we merge them?
mainly for discussions on the idea, not the concrete implementationLet's see if we can get it into a decent state to start using it.