-
Notifications
You must be signed in to change notification settings - Fork 7k
[serve] Fail on the change of 'proxy_location' or 'http_options' parameters for the 'serve' API #56507
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
base: master
Are you sure you want to change the base?
[serve] Fail on the change of 'proxy_location' or 'http_options' parameters for the 'serve' API #56507
Changes from all commits
fa2cf17
30a9c20
d55b10e
beb9b62
bd39834
dbd0825
50dfc45
c49706c
52b5d44
e59e19a
53e1561
2f1f4c7
392c14a
0821975
312f354
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -76,11 +76,10 @@ def test_serve_namespace(ray_start_stop): | |
| def test_put_with_http_options(ray_start_stop, option, override): | ||
| """Submits a config with HTTP options specified. | ||
|
|
||
| Trying to submit a config to the serve agent with the HTTP options modified should | ||
| NOT fail: | ||
| Trying to submit a config to the serve agent with the HTTP options modified should fail: | ||
| - If Serve is NOT running, HTTP options will be honored when starting Serve | ||
| - If Serve is running, HTTP options will be ignored, and warning will be logged | ||
| urging users to restart Serve if they want their options to take effect | ||
| - If Serve is running and HTTP options are attempted to be changed, deployment will fail, | ||
| forcing users to restart Serve if they want their options to take effect. | ||
| """ | ||
|
|
||
| pizza_import_path = "ray.serve.tests.test_config_files.pizza.serve_dag" | ||
|
|
@@ -126,7 +125,7 @@ def test_put_with_http_options(ray_start_stop, option, override): | |
| put_response = requests.put( | ||
| SERVE_HEAD_URL, json=updated_serve_config_json, timeout=5 | ||
| ) | ||
| assert put_response.status_code == 200 | ||
| assert put_response.status_code == 500 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. shouldn't we send 409 Conflict in such scenario? @abrar thoughts? |
||
|
|
||
| # Fetch Serve status and confirm that HTTP options are unchanged | ||
| get_response = requests.get(SERVE_HEAD_URL, timeout=5) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,37 +14,54 @@ | |
| SERVE_NAMESPACE, | ||
| ) | ||
| from ray.serve._private.default_impl import get_controller_impl | ||
| from ray.serve.config import HTTPOptions, gRPCOptions | ||
| from ray.serve.config import DeploymentMode, HTTPOptions, ProxyLocation, gRPCOptions | ||
| from ray.serve.context import _get_global_client, _set_global_client | ||
| from ray.serve.deployment import Application | ||
| from ray.serve.exceptions import RayServeException | ||
| from ray.serve.exceptions import RayServeConfigException, RayServeException | ||
| from ray.serve.schema import LoggingConfig | ||
|
|
||
| logger = logging.getLogger(SERVE_LOGGER_NAME) | ||
|
|
||
|
|
||
| def _check_http_options( | ||
| client: ServeControllerClient, http_options: Union[dict, HTTPOptions] | ||
| curr_http_options: HTTPOptions, new_http_options: Union[dict, HTTPOptions] | ||
| ) -> None: | ||
| if http_options: | ||
| client_http_options = client.http_config | ||
| new_http_options = ( | ||
| http_options | ||
| if isinstance(http_options, HTTPOptions) | ||
| else HTTPOptions.parse_obj(http_options) | ||
| ) | ||
| different_fields = [] | ||
| all_http_option_fields = new_http_options.__dict__ | ||
| for field in all_http_option_fields: | ||
| if getattr(new_http_options, field) != getattr(client_http_options, field): | ||
| different_fields.append(field) | ||
|
|
||
| if len(different_fields): | ||
| logger.warning( | ||
| "The new client HTTP config differs from the existing one " | ||
| f"in the following fields: {different_fields}. " | ||
| "The new HTTP config is ignored." | ||
| def maybe_restore_proxy_location(prev_value, new_value) -> (str, str): | ||
| if isinstance(prev_value, DeploymentMode) and isinstance( | ||
| new_value, DeploymentMode | ||
| ): | ||
| # restore ProxyLocation as this is the property user configured | ||
| prev_value = ProxyLocation._from_deployment_mode(prev_value).value | ||
| new_value = ProxyLocation._from_deployment_mode(new_value).value | ||
| return prev_value, new_value | ||
axreldable marked this conversation as resolved.
Show resolved
Hide resolved
axreldable marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| if not new_http_options: | ||
| return | ||
|
|
||
| if isinstance(new_http_options, HTTPOptions): | ||
| new_http_options = new_http_options.dict(exclude_unset=True) | ||
|
|
||
| diff_http_options = {} | ||
| for option, new_value in new_http_options.items(): | ||
| if not hasattr(curr_http_options, option): | ||
| valid_fields = ( | ||
| getattr(HTTPOptions, "__fields__", None) | ||
| or getattr(HTTPOptions, "model_fields", {}).keys() | ||
| ) | ||
| raise RayServeConfigException( | ||
| f"Invalid http_options key '{option}'. Valid keys: {sorted(valid_fields)}" | ||
| ) | ||
| prev_value = getattr(curr_http_options, option) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: BugThe
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed. |
||
| if prev_value != new_value: | ||
| prev_value, new_value = maybe_restore_proxy_location(prev_value, new_value) | ||
| diff_http_options[option] = {"previous": prev_value, "new": new_value} | ||
axreldable marked this conversation as resolved.
Show resolved
Hide resolved
axreldable marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| if diff_http_options: | ||
| raise RayServeConfigException( | ||
| "Attempt to update `http_options` or `proxy_location` has been detected! " | ||
| f"Attempted updates: `{diff_http_options}`. " | ||
| "HTTP config is global to your Ray cluster, and you can't update it during runtime. " | ||
| "Please restart Ray Serve to apply the change." | ||
axreldable marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| ) | ||
|
|
||
|
|
||
| def _start_controller( | ||
|
|
@@ -136,7 +153,7 @@ async def serve_start_async( | |
| " New http options will not be applied." | ||
| ) | ||
| if http_options: | ||
| _check_http_options(client, http_options) | ||
| _check_http_options(client.http_config, http_options) | ||
axreldable marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| return client | ||
| except RayServeException: | ||
| pass | ||
|
|
@@ -210,7 +227,7 @@ def serve_start( | |
| " New http options will not be applied." | ||
| ) | ||
| if http_options: | ||
| _check_http_options(client, http_options) | ||
| _check_http_options(client.http_config, http_options) | ||
| return client | ||
| except RayServeException: | ||
| pass | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -573,8 +573,8 @@ class HTTPOptions(BaseModel): | |
|
|
||
| - "HeadOnly": start one HTTP server on the head node. Serve | ||
| assumes the head node is the node you executed serve.start | ||
| on. This is the default. | ||
| - "EveryNode": start one HTTP server per node. | ||
| on. | ||
| - "EveryNode": start one HTTP server per node. This is the default. | ||
| - "NoServer": disable HTTP server. | ||
|
|
||
| - num_cpus: [DEPRECATED] The number of CPU cores to reserve for each | ||
|
|
@@ -584,7 +584,7 @@ class HTTPOptions(BaseModel): | |
| host: Optional[str] = DEFAULT_HTTP_HOST | ||
| port: int = DEFAULT_HTTP_PORT | ||
| middlewares: List[Any] = [] | ||
| location: Optional[DeploymentMode] = DeploymentMode.HeadOnly | ||
| location: Optional[DeploymentMode] = DeploymentMode.EveryNode | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @abrarsheikh , please also notice that I change this default value. Aiming to align it with Proxy-location default. Without this if I remember correctly it's challenging to align all of the tests and can be confusing for users either.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd also align default |
||
| num_cpus: int = 0 | ||
| root_url: str = "" | ||
| root_path: str = "" | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -58,7 +58,7 @@ def check_http_response(expected_text: str, json: Optional[Dict] = None): | |
|
|
||
|
|
||
| def test_start_shutdown(ray_start_stop): | ||
| subprocess.check_output(["serve", "start"]) | ||
| subprocess.check_output(["serve", "start", "--http-host", "0.0.0.0"]) | ||
| # deploy a simple app | ||
| import_path = "ray.serve.tests.test_config_files.arg_builders.build_echo_app" | ||
|
|
||
|
|
@@ -253,7 +253,7 @@ def test_idempotence_after_controller_death(ray_start_stop, use_command: bool): | |
| deploy_response = subprocess.check_output(["serve", "deploy", config_file_name]) | ||
| assert success_message_fragment in deploy_response | ||
|
|
||
| serve.start() | ||
| serve.start(http_options={"host": "0.0.0.0"}) | ||
| wait_for_condition( | ||
| lambda: len(list_actors(filters=[("state", "=", "ALIVE")])) == 4, | ||
| timeout=15, | ||
|
|
@@ -274,7 +274,7 @@ def test_idempotence_after_controller_death(ray_start_stop, use_command: bool): | |
| assert success_message_fragment in deploy_response | ||
|
|
||
| # Restore testing controller | ||
| serve.start() | ||
| serve.start(http_options={"host": "0.0.0.0"}) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why change this?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is the same discrepancy in default values. |
||
| wait_for_condition( | ||
| lambda: len(list_actors(filters=[("state", "=", "ALIVE")])) == 4, | ||
| timeout=15, | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.