[serve] Add list_deployments API #15152
Conversation
…-route-to-deployment-options
…-route-to-deployment-options
…-route-to-deployment-options
…-route-to-deployment-options
…e-list-deployments
| class ServeDeployment(ABC): | ||
| @classmethod | ||
| def deploy(cls, *init_args) -> None: | ||
| class ServeDeployment: |
There was a problem hiding this comment.
I needed to make the decorator return a class instance instead of a class in order to define __str__ and __cmp__ methods. The other way would be to define a metaclass but cloudpickle really didn't like me doing that so I reverted to this.
There was a problem hiding this comment.
Sounds good. Initially, why was it better to make it return a class instead of a class instance? In other words, what's the downside of this change?
There was a problem hiding this comment.
Eh there isn't a huge difference (everything in python is an object anyways :)), but it is more intuitive to return a class rather than an object from the decorator given that an object was never explicitly constructed.
There was a problem hiding this comment.
Makes sense. And did we need __str__ and __cmp__ specifically for this PR, or are they just generally useful to have? Seems worthwhile to add them in this PR either way
There was a problem hiding this comment.
I guess we needed it to test equality of the deployment list dict.
There was a problem hiding this comment.
Yep exactly! The __str__ is just an additional niceity
| @@ -1308,14 +1313,41 @@ def get_deployment(name: str) -> ServeDeployment: | |||
| ServeDeployment | |||
| """ | |||
| try: | |||
| backend_info, route = _get_global_client().get_deployment_info(name) | |||
| backend_info, route_prefix = _get_global_client().get_deployment_info( | |||
| name, _internal=True) | |||
There was a problem hiding this comment.
Here it looks like the signature of get_deployment_info is changed to accept an additional bool, but I can't find the changed implementation of get_deployment_info in this PR... Tests are passing though so maybe I'm just confused.
| class ServeDeployment(ABC): | ||
| @classmethod | ||
| def deploy(cls, *init_args) -> None: | ||
| class ServeDeployment: |
There was a problem hiding this comment.
Makes sense. And did we need __str__ and __cmp__ specifically for this PR, or are they just generally useful to have? Seems worthwhile to add them in this PR either way
Why are these changes needed?
Related issue number
Closes #15149
Checks
scripts/format.shto lint the changes in this PR.