Repository navigation
Add helper for FastAPI integration - #451
Conversation
Signed-off-by: Anuraag Agrawal <anuraaga@gmail.com>
Signed-off-by: Anuraag Agrawal <anuraaga@gmail.com>
Signed-off-by: Anuraag Agrawal <anuraaga@gmail.com>
stefanvanburen
left a comment
stefanvanburen
left a comment
There was a problem hiding this comment.
Took a pass at this (with some help from Claude, and checked each of these against 68269af). The first four are the ones I think matter before a release; the rest are non-blocking nits.
| "Topic :: System :: Networking", | ||
| ] | ||
| dependencies = [ | ||
| "connectrpc>=0.13.0", |
There was a problem hiding this comment.
method_local_name and ConnectASGIApplication(desc=...) both came in with #445, which isn't in v0.13.0 (git grep method_local_name v0.13.0 comes up empty), so I think import connectrpc_fastapi blows up against the latest release. Probably ought to bump this floor to the next connectrpc release and cut that first? (scripts/check_wheels.py doesn't catch it since it installs connectrpc from dist/.)
There was a problem hiding this comment.
Yeah we have others needing this too. I'm planning to just have the bump task bump these dependencies to the latest, even when not releasing the secondary packages, for simplicity. And when they do get bumped, a user gets the latest connectrpc at the same time if they otherwise wouldn't, which seems like reasonable behavior for connectrpc helpers
There was a problem hiding this comment.
sounds good to me; same thing applies to grpchealth on main right now, so having bump handle all of them makes sense. (Assuming fastapi doesn't go out before the next connectrpc release.)
| tags=list(tags) if tags is not None else [desc.type_name], | ||
| generate_unique_id_function=_operation_id, | ||
| ) | ||
| create_application = application or _default_application(desc) |
There was a problem hiding this comment.
Tried this with the google_compat Haberdasher and no application. The handler gets a protobuf._message.Size instead of a haberdasher_pb2.Size, and then returning a google Hat fails in the codec with AttributeError: to_json, which escapes the app instead of becoming a Connect error. Passing HaberdasherASGIApplication works fine. Feels pretty easy to miss in the docstring; any chance we can detect google mode here and either raise when building the router or pick up the generated application? (Not sure what the best signal is.)
There was a problem hiding this comment.
Fixed this by finding the ASGI application - relies on gencode structure, but seems like a very stable property of it
There was a problem hiding this comment.
confirmed — with no application, the google_compat Haberdasher now gets a haberdasher_pb2.Size and MakeHat returns a 200 (68269af still escapes with AttributeError: to_json). Works for a service that doesn't subclass the protocol too. Might be worth a google_compat case in test_router.py? test_default_application_is_generated_class only covers Eliza.
| service: Any = Depends(get_service), # noqa: B008 | ||
| ) -> Response: | ||
| """Serve the request with the Connect application.""" | ||
| connect_response = _ConnectResponse(create_application(service), path, request) |
There was a problem hiding this comment.
non-blocking: this builds a new ConnectASGIApplication per request, so every method's endpoint (and interceptors) gets rebuilt each time. Measured ~10µs/request for Eliza's 3 methods, so not huge, but it grows with the method count. Maybe build just the one endpoint for this route, or cache per service instance when get_service returns a singleton?
| @@ -0,0 +1,43 @@ | |||
| name: Release connectrpc-fastapi | |||
There was a problem hiding this comment.
nit: this is identical to the grpchealth/grpcreflect/otel workflows other than the name; fine for now, I owe a follow-up folding these into one workflow keyed on the tag prefix.
stefanvanburen
left a comment
stefanvanburen
left a comment
There was a problem hiding this comment.
LGTM, thanks! One non-blocking note inline.
| return receive_body | ||
|
|
||
|
|
||
| def connect_router( |
There was a problem hiding this comment.
non-blocking: dropping _is_async means passing the sync protocol (connect_router(ElizaServiceSync, ...)) now builds fine and then every request escapes with TypeError: 'SayResponse' object can't be awaited, where 68269af raised TypeError: ElizaServiceSync.say is not an async method up front. Type checkers don't catch it either (ServiceType only needs desc()). Could we keep some up-front check, here or in a follow-up? Even just rejecting sync protocols (no …ASGIApplication generated next to them) would cover the common mistake; the annotation-sniffing was what felt fragile to me.
There was a problem hiding this comment.
Sure - checking the asgiapplication vs wsgiapplication sounds good
Signed-off-by: Anuraag Agrawal <anuraaga@gmail.com>
Inspired by #442 (and maybe enough to close it)
We can provide a helper to make it easy to register a Connect service with FastAPI in a way that resolves dependencies. I also went so far as wiring up the schema for docs. Some parts of FastAPI's design meant we need a second entrypoint to enrich the docs unfortunately, but if the second call is missed the service itself will still work without docs. In some sense, this docs integration may make FastAPI a definitive way of serving connect apps.