Conversation
edoakes
left a comment
There was a problem hiding this comment.
Looks good, just a few small comments.
Assume you'll add the status endpoint in a follow up?
|
|
||
| return DeploymentSchema( | ||
| name=d.name, | ||
| import_path=d.func_or_class, |
There was a problem hiding this comment.
hmm what will happen if this is a serialized class? we should make the change to only support import_paths in the controller so this makes sense
There was a problem hiding this comment.
Good question, that would cause an issue only when using both the REST API and the Python API at the same time. I can add handling to avoid setting the import path to a serialized function/class.
Making the controller only use import paths is a good idea, and I believe we’re making that change as a part of the controller unification. Since it’s somewhat extensive, I’d rather include it in a follow-up change.
There was a problem hiding this comment.
yes sounds good, file an issue for it?
| @routes.delete("/api/serve/deployments/") | ||
| @optional_utils.init_ray_and_catch_exceptions(connect_to_serve=True) | ||
| async def delete_all_deployments(self, req: Request) -> Response: | ||
| serve.shutdown() |
There was a problem hiding this comment.
is this actually what we want to do or should we instead just delete the deployments themselves? this behavior seems ok to me given that we aren't accepting namespaces or anything
There was a problem hiding this comment.
I think serve.shutdown() makes sense because then a DELETE request deletes the entire serve instance, not just the deployments. I've renamed the function to delete_serve_instance to reflect that.
| all_deployments = serve.list_deployments() | ||
| all_names = set(all_deployments.keys()) | ||
| names_to_delete = all_names.difference(new_names) | ||
| for name in names_to_delete: | ||
| all_deployments[name].delete() | ||
|
|
||
| return Response() |
There was a problem hiding this comment.
I think this logic should live in the deploy_group API?
There was a problem hiding this comment.
Since deletions don’t need to be atomic, I think it’s ok to keep them out of deploy_group. It makes deploy_group more widely usable since it remains a way to atomically deploy a set of deployments.
There was a problem hiding this comment.
ok, this is fine for now we can revisit it. not too important w/o upgrade in tandem
|
Thanks for the review! Yes, I’m planning to add statuses in a follow up. |
edoakes
left a comment
There was a problem hiding this comment.
LGTM for first cut, we can continue to iterate on the points I brought up below.
This change adds the GET, PUT, and DELETE commands for Serve’s REST API. The dashboard receives these commands and issues corresponding requests to the Serve controller.
Why are these changes needed?
This change adds the GET, PUT, and DELETE commands for Serve’s REST API. The dashboard receives these commands and issues corresponding requests to the Serve controller.
Related issue number
Closes #21880.
Checks
scripts/format.shto lint the changes in this PR.test_serve_head.pyto assess the REST API’s behavior. It also adds new tests totest_schema.pyto assess updated behavior in the JSON schemas.