-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
[Serve] Enable serve status
when proxies disabled
#42286
[Serve] Enable serve status
when proxies disabled
#42286
Conversation
Signed-off-by: Shreyas Krishnaswamy <shrekris@anyscale.com>
Signed-off-by: Shreyas Krishnaswamy <shrekris@anyscale.com>
Signed-off-by: Shreyas Krishnaswamy <shrekris@anyscale.com>
Signed-off-by: Shreyas Krishnaswamy <shrekris@anyscale.com>
def _to_deployment_mode( | ||
cls, proxy_location: Union["ProxyLocation", str] | ||
) -> DeploymentMode: | ||
if isinstance(proxy_location, str): | ||
proxy_location = ProxyLocation(proxy_location) | ||
elif not isinstance(proxy_location, ProxyLocation): | ||
raise TypeError( | ||
f"Must be a `ProxyLocation` or str, got: {type(proxy_location)}." | ||
) | ||
|
||
if proxy_location == ProxyLocation.Disabled: | ||
return DeploymentMode.NoServer | ||
elif v == ProxyLocation.HeadOnly: | ||
return DeploymentMode.HeadOnly | ||
elif v == ProxyLocation.EveryNode: | ||
return DeploymentMode.EveryNode | ||
else: | ||
raise ValueError(f"Unrecognized `ProxyLocation`: {v}.") | ||
return DeploymentMode(proxy_location.value) |
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 should not change behavior. I refactored this for simplicity's sake.
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.
Small non-blocking suggestions, but overall LGTM! Thanks for fixing the issue Shreyas!
Signed-off-by: Shreyas Krishnaswamy <shrekris@anyscale.com>
Serve proxies can be disabled by setting serve.start(..., proxy_location=ProxyLocation.Disabled). However, serve status currently fails when proxies are disabled. This change fixes the issue and lets serve status succeed even when proxy_location==ProxyLocation.Disabled. --------- Signed-off-by: Shreyas Krishnaswamy <shrekris@anyscale.com>
Why are these changes needed?
Serve proxies can be disabled by setting
serve.start(..., proxy_location=ProxyLocation.Disabled)
. However,serve status
currently fails when proxies are disabled.This change fixes the issue and lets
serve status
succeed even whenproxy_location==ProxyLocation.Disabled
.Related issue number
Closes #41723.
Checks
git commit -s
) in this PR.scripts/format.sh
to lint the changes in this PR.method in Tune, I've added it in
doc/source/tune/api/
under thecorresponding
.rst
file.test_config.py
.