-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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] Make HTTP proxy (almost) a regular Serve deployment #15820
Conversation
…-proxy-as-deployment
e75ce44
to
4a91de7
Compare
87360c9
to
30396f5
Compare
…-proxy-as-deployment
…-proxy-as-deployment
Blocking this on #15909 so we can wait for config changes to propagate without needing a hacky solution |
…-proxy-as-deployment
…-proxy-as-deployment
@@ -31,7 +31,7 @@ class BackendConfig(BaseModel): | |||
for shutdown. Defaults to 20s. | |||
""" | |||
|
|||
num_replicas: PositiveInt = 1 | |||
num_replicas: Any = 1 |
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.
Union[str, PositiveInt]
raise TimeoutError( | ||
"HTTP proxies not available after {HTTP_PROXY_TIMEOUT}s.") | ||
client = Client(controller, controller_name, detached=detached) | ||
_set_global_client(client) |
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 calling these lines twice ? Line 730
@@ -81,6 +88,8 @@ def actor_handle(self) -> ActorHandle: | |||
|
|||
def start_or_update(self, backend_info: BackendInfo): | |||
self._actor_resources = backend_info.replica_config.resource_dict | |||
if self._node_resource is not None: | |||
self._actor_resources[self._node_resource] = 0.001 |
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.
what does 0.001 mean here ?
@@ -837,8 +864,14 @@ def _scale_backend_replicas( | |||
""" | |||
assert (backend_tag in self._backend_metadata | |||
), "Backend {} is not registered.".format(backend_tag) | |||
assert target_replicas >= 0, ("Number of replicas must be" | |||
" greater than or equal to 0.") | |||
if isinstance(target_replicas, str): |
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 changes target_replicas' type from int (which also implies it from its name) defined in line 853, maybe we can assign a special value for it like -1, or introduce a new flag to control its behavior ?
assert target_replicas >= 0, ("Number of replicas must be" | ||
" greater than or equal to 0.") | ||
if isinstance(target_replicas, str): | ||
assert target_replicas == "EveryNode" |
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.
sorry i don't have full context on this concept yet .. does "EveryNode" means the target replica num == all nodes we have in the ray cluster ? If so it might be simpler to handle it outside of this function call and simply pass in a number.
…-proxy-as-deployment
…-proxy-as-deployment
…-proxy-as-deployment
…-proxy-as-deployment
…-proxy-as-deployment
…-proxy-as-deployment
…-proxy-as-deployment
…-proxy-as-deployment
is this WIP or blocked on review ? |
@jiaodong feel free to review, the only thing it's really blocked on is better handling of constructor failure so we can raise an error if the HTTP proxy fails to start up (e.g., bad HTTP host or port) |
i see .. let me ensure to actually put sometime on the constructor failure issue to unblock this. |
…-proxy-as-deployment
Is this still WIP ? |
@jiaodong this is unblocked now that you made the constructor failure PR, but there are a ton of merge conflicts from the refactors. I will try to clean it up in the background otherwise we can prioritize sometime in the coming weeks. |
|
@edoakes should we close this as a prototype and I can open a new PR to tackle this? |
@simon-mo sounds good to me |
Why are these changes needed?
Entirely removes
http_state.py
and instead expresses the HTTP proxy as a regular deployment. This reduces the number of core concepts we need to maintain. It also offers a clear avenue to make the HTTP settings declarative and updateable in the future .We do need some special casing here to handle the "EveryNode" deployment strategy. We should be able to remove this in the future if we can lean on placement group policies.
Related issue number
Closes #15561
Checks
scripts/format.sh
to lint the changes in this PR.