-
Notifications
You must be signed in to change notification settings - Fork 203
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: add HTTP api to interact with Manager #267
Conversation
@@ -0,0 +1,45 @@ | |||
from fastapi import APIRouter, File, UploadFile, HTTPException |
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 didn't spend too much time for these three endpoints, will define the appropriate response objects later
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 looks good to me! Very straightforward
A few things are hardcoded (the api server host/port, deployment filepaths), but I'm guessing thats because we still lack the actual CLI for now 👍🏻
@logan-markewich correct, we need a proper configuration system for the apiserver, likely just its own |
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 left one question re: about self._queue
and self._simple_message_queue_task
, but other than that, looks good!
llama_deploy/apiserver/deployment.py
Outdated
@@ -79,7 +79,9 @@ async def start(self) -> None: | |||
# Spawn SimpleMessageQueue if needed | |||
if self._simple_message_queue_task: | |||
# If SimpleMessageQueue was selected in the config file we take care of running the task | |||
tasks.append(asyncio.create_task(self._queue.launch_server())) | |||
tasks.append( | |||
asyncio.create_task(self._simple_message_queue_task.launch_server()) |
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.
How come this is no longer self._queue
? Maybe dumb question, but why do we have these two? (simple_message_queue_task
is a bit misleading maybe, cause it is a SimpleMessageQueue
rather than a task?)
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.
The problem is with the simple queue we have 2 entities: SimpleMessageQueue
and SimpleRemoteClientMessageQueue
and I call "task" the former since it has a launch_server
method. self._queue
holds the client, for consistency with the other queue types. I can probably rename self._queue
to self._queue_client
for clarity.
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.
Now that I read the code, probably what we call "message queues" should be "message queue clients", yellow flag is no class reimplements launch_server
and launch_local
from the base class except the simple message queue. Also SimpleRemoteClientMessageQueue
raises on launch_server
and launch_local
, we might consider a refactoring of that part?
This PR adds an HTTP API implemented with FastAPI in front of the deployment Manager.
To run the server, from the llama_deploy folder:
To create a deployment: