-
Notifications
You must be signed in to change notification settings - Fork 3
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
FastAPI routing #9
Conversation
|
||
@app.get("/") | ||
def read_root(): | ||
my_cool_coroutine.call() |
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.
Note: this does not exist until #5.
Raises: | ||
ValueError: If any of the required arguments are missing. | ||
""" | ||
api_key = api_key or os.environ.get("DISPATCH_API_KEY") |
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.
api_key
is here for now, but really just a placeholder for #5
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.
Nice work! How do we decide about the coroutine_uri format?
|
||
dispatch_app = _new_app() | ||
|
||
app.__setattr__("dispatch_coroutine", dispatch_app.dispatch_coroutine) |
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.
Are we innovating with __setattr__
on the FastAPI object, or is this common practice?
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 are. I liked the symmetry with their existing @app.get().
src/dispatch/fastapi.py
Outdated
def configure( | ||
app: fastapi.FastAPI, | ||
api_key: None | str = None, | ||
mount_path: str = "/dispatch", |
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'm curious about this, since we're using gRPC, by default the path will be something like /dispatch.coroutine.v1.ExecutorService/Execute
, how would we know to add this prefix in the request?
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 guess it's up to the channel to implement that. The client needs to tell us where to call, but that's not different than just giving us the host 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.
I think it's a bit different than configuring the hostname. When using gRPC, it's common to configure the host, but typically we don't set the path because it's implied by the protocol. If we change the path, we diverge from the gRPC protocol, which will likely make things more difficult for us down the road. For example, this is what the gRPC specification says about it:
Path is case-sensitive. Some gRPC implementations may allow the Path format shown above to be overridden, but this functionality is strongly discouraged. gRPC does not go out of its way to break users that are using this kind of override, but we do not actively support it, and some functionality (e.g., service config support) will not work when the path is not of the form shown above.
I see the use case for wanting to set a prefix, tho it seems more like an advanced usage than something I would like to encourage all users to do.
How about starting with the default gRPC endpoint? (e.g., for now, use /ring.coroutine.v1.ExecutorService/Execute
, then we'll change it to something ring-free later). We can still add this functionality later on.
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.
Ok. I'll remove that option and remove the / endpoint.
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've changed the mount point of the sub app to be /ring.coroutine.v1.ExecutorService
, and the subapp exposes /Execute
.
It's probably the least bad trade-off if we don't want an arbitrary mount point like previously. Not using a mount point (i.e. adding a new route directly to the FastAPI app) means we are subject to any global configuration the user has set, for example app-wide middleware. We cannot mount a sub-app to /
, because existing routes become unavailable. The drawback of mounting at /ring.coroutine.v1.ExecutorService
is that the sub app is not standalone anymore. Because it exposes just /Execute
, we can't point a gRPC client at it directly. I've updated the tests to make this work, but it may cause issues down the line.
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.
That seems like a good compromise, we can figure out the gRPC-only version of this when we get to it 👍
First implementation of FastAPI routing. It allows a hook into an existing FastAPI app to delegate a route to Dispatch. That route implements the ExecutorService interface over gRPC. Coroutines are expected to be manually written. Since the eventual goal is to have the coroutine code auto-generated, I tried to find a balance between being natural enough for a Python developer and not extremely complex to implement. That's why there is sometimes a delta between the proto definitions and the classes provided by the SDK. We can add sugar on top later if we want to and/or line them up later.
Testing is performed using an "actual" gRPC client. The official Google gRPC library is used over a custom channel implementation, which performs the calls over the FastAPI built-in client. The coupling of testing coroutines using FastAPI's structure is not ideal, but it allowed me to test end-to-end quickly. It is also probably going to be the basis for #5.
We should quickly consider writing a "mini ring" to test sequences of requests/responses. Tests in
test_fastapi.py
do this manually, but writing more than a couple of interactions gets verbose and tedious.Overall I'm reasonably happy with the current status of this code, knowing it's the first of many iterations.
As much as possible, all user-facing API types/functions should be documented and with type hints. Internal stuff uses comments instead of docstrings, and usually have names that start with
_
.If you want to play/review locally:
ExecutorService
interface implementation in FastAPI.Fixes #1