[serve][deploy refactor][2/X] Move lightweight update logic to DeploymentVersion #34430

zcin · 2023-04-14T23:43:31Z

Why are these changes needed?

DeploymentVersion already has existing support for code version + other config options that affect the version (before, only user config was taken into account). We can leverage this to do lightweight config updates, so that:

The config's import_path and runtime_env tells us the code version for the deployments
The logic for lightweight config updates is hidden in DeploymentVersion

So instead of having DeploymentVersion only rely on user config, we want it to rely on other info from DeploymentInfo, so we can decide which config options should trigger a redeployment and which shouldn't. I've added annotations DeploymentOptionUpdateType to fields in DeploymentConfig to indicate which options fall into which category as described below.

Heavyweight options that will force replicas to restart: version and ray_actor_options (which is part of replica config)
Lightweight options that need to call reconfigure on the replica actor: user_config and graceful_shutdown_wait_loop_s
Lightweight options (that need to update replicas in replica state container, but won't need to call into the actual actor): max_concurrent_queries, graceful_shutdown_timeout_s, health_check_period_s, health_check_timeout_s
Purely lightweight options: num_replicas and autoscaling_config

This fixes the issue of deployments not listed in config being redeployed. See the newly added test test_deployments_not_listed_in_config.

Related issue number

Checks

I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
I've run scripts/format.sh to lint the changes in this PR.
I've included any doc changes needed for https://docs.ray.io/en/master/.
- I've added any new APIs to the API Reference. For example, if I added a
  method in Tune, I've added it in doc/source/tune/api/ under the
  corresponding .rst file.
I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
Testing Strategy
- Unit tests
- Release tests
- This PR is not tested :(

Signed-off-by: Cindy Zhang <[email protected]>

zcin · 2023-04-18T16:02:23Z

python/ray/serve/_private/version.py

+                    "max_concurrent_queries": deployment_config.max_concurrent_queries,
+                    "graceful_shutdown_timeout_s": (
+                        deployment_config.graceful_shutdown_timeout_s
+                    ),
+                    "graceful_shutdown_wait_loop_s": (
+                        deployment_config.graceful_shutdown_wait_loop_s
+                    ),
+                    "health_check_period_s": deployment_config.health_check_period_s,
+                    "health_check_timeout_s": deployment_config.health_check_timeout_s,


Are these the deployment config options that, if changed, we want to redeploy the replicas?

It would be great if we could encode the options that require lightweight/heavyweight updates directly in the code somehow. e.g., an annotation on the deployment_config struct definition. can you think of a way to do that?

but to answer your question directly, if we are broadcasting these updates to the replicas using the existing logic for user_config (as I saw above), then none of these should require restarting the replicas.

It would be great if we could encode the options that require lightweight/heavyweight updates directly in the code somehow. e.g., an annotation on the deployment_config struct definition. can you think of a way to do that?

@edoakes I've added it as an extra arg in each pydantic field

edoakes · 2023-04-18T16:02:34Z

@zcin this is fantastic, it also solves a long-standing issue we've had where some of the config options are not actually dynamically updated because we weren't broadcasting the config!

python/ray/serve/_private/version.py

sihanwang41 · 2023-04-21T02:13:13Z

python/ray/serve/tests/test_config_files/pid.py

@@ -21,7 +21,7 @@ def __init__(self, dag: RayServeDAGHandle):
        self.dag = dag

    async def __call__(self):
-        return await self.dag.remote()
+        return await (await self.dag.remote())


any reason we have this change because of this pr?

I'm pretty sure this code didn't work before 🤦‍♀️. Not sure how all the tests were passing in test_deploy_config_update, but some of the false positives were because the response weren't casted to int, so I've updated the test to do that.

sihanwang41 · 2023-04-21T18:25:17Z

python/ray/serve/_private/deploy_utils.py

+            "import_path": app_config.import_path,
+            "runtime_env": app_config.runtime_env,
+        },
+        sort_keys=True,


nice with sort_keys.

sihanwang41 · 2023-04-21T18:32:51Z

python/ray/serve/_private/deployment_state.py

@@ -1180,10 +1219,14 @@ def deploy(self, deployment_info: DeploymentInfo) -> bool:
        existing_info = self._target_state.info
        if existing_info is not None:
            # Redeploying should not reset the deployment's start time.
-            deployment_info.start_time_ms = existing_info.start_time_ms
+            if not self._target_state.deleting:


if not self._target_state.deleting, what the reason for adding this check? is this an existing bug?

I'm not sure, I don't know how often this case will occur (probably not often?).
This is specifically for when a deployment has been deleted (e.g. the deployment graph changed) and then redeployed (e.g. the deployment graph changed back).

The biggest change I made here is on line 1211, so that if all configs are the same but the current target state is deleting, the deployment isn't blocked from redeploying. I ran into this problem when debugging some unit tests, not sure why this pops up in my PR specifically.

sihanwang41 · 2023-04-21T18:40:42Z

python/ray/serve/_private/replica.py

-            self.version = DeploymentVersion(
-                self.version.code_version, user_config=user_config
+            user_config_changed = False
+            if (


nit: should we directly if self.deployment_config.user_config != deployment_config.user_config

When the replica is first initialized, deployment_config is None.

sihanwang41 · 2023-04-21T18:50:39Z

python/ray/serve/_private/version.py

-        self._hash = crc32(serialized_user_config + self.code_version.encode("utf-8"))
+        self.deployment_config: DeploymentConfig = deployment_config
+        self.ray_actor_options: Dict = ray_actor_options
+        self.compute_hashes()


I assume we don't allow anyone to update the attribute, since it will change the hash value. I suggest to change the class DeploymentVersion into pydantic and freeze it after initialization. Or we raise error when anyone update the attribute.

Would it become too heavyweight if we change it to pydantic? I'm in favor of making deployment version immutable, though.

both sounds good to me, "freezing the class" makes the code less error-proned.

Yeah, agreed. I'll probably look into this in a potential follow up PR.

python/ray/serve/config.py

Signed-off-by: Cindy Zhang <[email protected]>

edoakes

@zcin can you add an integration test that graceful_shutdown_period_s is correctly updated without restarting the replica? Ideally we'd have this for each of the options that can be set.

You can do this by:

Start with an extremely long graceful_shutdown_period_s.
Update it to be very short.
Check that the replica ID/actor ID doesn't change.
Send a request that hangs forever.
Shutdown or do an update, check that it completes quickly.

Signed-off-by: Cindy Zhang <[email protected]>

…utdown timeout Signed-off-by: Cindy Zhang <[email protected]>

Signed-off-by: Cindy Zhang <[email protected]>

edoakes · 2023-04-25T20:51:57Z

@zcin serve tests failing.

zcin · 2023-04-25T21:24:53Z

@edoakes The failure is caused by Sihan's PR, he is working on a fix right now. The errors look unrelated to this PR.

sihanwang41 · 2023-04-25T22:45:45Z

python/ray/serve/_private/deployment_state.py

@@ -1243,48 +1271,47 @@ def _stop_wrong_version_replicas(self, max_to_stop=math.inf) -> int:
            max_replicas=max_to_stop,
            ranking_function=rank_replicas_for_stopping,
        )
-        replicas_stopped = False
+        replicas_changed = False


nit: rename _stop_wrong_version_replicas function.

Renamed to _stop_or_update_outdated_version_replicas

sihanwang41 · 2023-04-25T23:01:10Z

python/ray/serve/_private/deployment_state.py

        for replica in replicas_to_update:
-            # If the code version is a mismatch, we stop the replica. A new one
-            # with the correct version will be started later as part of the
+            replicas_changed = True


In reconfigure use case, will this trigger _stop_wrong_version_replicas returning True and potentially trigger a long poll broadcast which is not necessary?

That's a good point, it's only necessary for replicas that have max concurrent queries updated. I've changed the code to only return True in that case.

Signed-off-by: Cindy Zhang <[email protected]>

sihanwang41 · 2023-04-26T15:45:58Z

could you rebase master? dashboard tests are fixed.

Signed-off-by: Cindy Zhang <[email protected]>

zcin · 2023-04-26T18:06:48Z

@edoakes Tests look good, dataset tests unrelated and failing on master.

…mentVersion (ray-project#34430) `DeploymentVersion` already has existing support for code version + other config options that affect the version (before, only user config was taken into account). We can leverage this to do lightweight config updates, so that: - The config's `import_path` and `runtime_env` tells us the code version for the deployments - The logic for lightweight config updates is hidden in `DeploymentVersion` So instead of having `DeploymentVersion` only rely on user config, we want it to rely on other info from `DeploymentInfo`, so we can decide which config options should trigger a redeployment and which shouldn't. I've added annotations `DeploymentOptionUpdateType` to fields in `DeploymentConfig` to indicate which options fall into which category as described below. * Heavyweight options that will force replicas to restart: `version` and `ray_actor_options` (which is part of replica config) * Lightweight options that need to call reconfigure on the replica actor: `user_config` and `graceful_shutdown_wait_loop_s` * Lightweight options (that need to update replicas in replica state container, but won't need to call into the actual actor): `max_concurrent_queries`, `graceful_shutdown_timeout_s`, `health_check_period_s`, `health_check_timeout_s` * Purely lightweight options: `num_replicas` and `autoscaling_config` <img width="799" alt="Screen Shot 2023-04-20 at 6 50 13 PM" src="https://user-images.githubusercontent.com/15851518/233521791-537bf512-3f7a-4add-8a9a-701fde8ebeb1.png"> <img width="800" alt="Screen Shot 2023-04-20 at 6 50 31 PM" src="https://user-images.githubusercontent.com/15851518/233521825-b3a74e76-d863-4715-b66f-fa128319a461.png"> This fixes the issue of deployments not listed in config being redeployed. See the newly added test `test_deployments_not_listed_in_config`. Signed-off-by: Jack He <[email protected]>

…mentVersion (ray-project#34430) `DeploymentVersion` already has existing support for code version + other config options that affect the version (before, only user config was taken into account). We can leverage this to do lightweight config updates, so that: - The config's `import_path` and `runtime_env` tells us the code version for the deployments - The logic for lightweight config updates is hidden in `DeploymentVersion` So instead of having `DeploymentVersion` only rely on user config, we want it to rely on other info from `DeploymentInfo`, so we can decide which config options should trigger a redeployment and which shouldn't. I've added annotations `DeploymentOptionUpdateType` to fields in `DeploymentConfig` to indicate which options fall into which category as described below. * Heavyweight options that will force replicas to restart: `version` and `ray_actor_options` (which is part of replica config) * Lightweight options that need to call reconfigure on the replica actor: `user_config` and `graceful_shutdown_wait_loop_s` * Lightweight options (that need to update replicas in replica state container, but won't need to call into the actual actor): `max_concurrent_queries`, `graceful_shutdown_timeout_s`, `health_check_period_s`, `health_check_timeout_s` * Purely lightweight options: `num_replicas` and `autoscaling_config` <img width="799" alt="Screen Shot 2023-04-20 at 6 50 13 PM" src="https://user-images.githubusercontent.com/15851518/233521791-537bf512-3f7a-4add-8a9a-701fde8ebeb1.png"> <img width="800" alt="Screen Shot 2023-04-20 at 6 50 31 PM" src="https://user-images.githubusercontent.com/15851518/233521825-b3a74e76-d863-4715-b66f-fa128319a461.png"> This fixes the issue of deployments not listed in config being redeployed. See the newly added test `test_deployments_not_listed_in_config`.

zcin changed the title ~~[serve] Move lightweight update logic to DeploymentVersion~~ [serve][deploy refactor][2/X] Move lightweight update logic to DeploymentVersion Apr 14, 2023

zcin force-pushed the version-improvements branch from 6f30290 to 1818e6b Compare April 15, 2023 01:17

move lightweight update logic to DeploymentVersion

c8fe847

Signed-off-by: Cindy Zhang <[email protected]>

zcin force-pushed the version-improvements branch 2 times, most recently from 2b4dde6 to 4b1047f Compare April 15, 2023 15:16

wip

ded09de

Signed-off-by: Cindy Zhang <[email protected]>

zcin force-pushed the version-improvements branch 13 times, most recently from 8d8f9b5 to 2b5f193 Compare April 18, 2023 06:08

java attempt

c06156f

Signed-off-by: Cindy Zhang <[email protected]>

zcin force-pushed the version-improvements branch from 2b5f193 to c06156f Compare April 18, 2023 07:10

zcin added 2 commits April 18, 2023 07:59

Merge branch 'master' into version-improvements

6b2e1b3

Signed-off-by: Cindy Zhang <[email protected]>

add test for missing deployments in config

9361ec0

Signed-off-by: Cindy Zhang <[email protected]>

zcin marked this pull request as ready for review April 18, 2023 15:56

zcin requested review from sihanwang41, edoakes and shrekris-anyscale April 18, 2023 15:56

zcin commented Apr 18, 2023

View reviewed changes

python/ray/serve/_private/version.py Outdated Show resolved Hide resolved

sihanwang41 reviewed Apr 21, 2023

View reviewed changes

zcin added 4 commits April 21, 2023 17:52

move version into actor replica wrapper

6c7cab5

Signed-off-by: Cindy Zhang <[email protected]>

add more tests

daa1dca

Signed-off-by: Cindy Zhang <[email protected]>

fix + improve

5c97224

Signed-off-by: Cindy Zhang <[email protected]>

Merge branch 'master' into version-improvements

9371a1f

Signed-off-by: Cindy Zhang <[email protected]>

edoakes reviewed Apr 24, 2023

View reviewed changes

zcin added 4 commits April 24, 2023 11:56

fix java

4988852

Signed-off-by: Cindy Zhang <[email protected]>

Merge branch 'master' into version-improvements

35ac43d

Signed-off-by: Cindy Zhang <[email protected]>

add e2e test for user config, max concurrent queries, and graceful sh…

4d200c5

…utdown timeout Signed-off-by: Cindy Zhang <[email protected]>

fix typo

40e8bfb

Signed-off-by: Cindy Zhang <[email protected]>

zcin force-pushed the version-improvements branch from 846f349 to 40e8bfb Compare April 25, 2023 17:45

zcin added 2 commits April 25, 2023 10:52

Merge branch 'master' into version-improvements

0257ada

Signed-off-by: Cindy Zhang <[email protected]>

fix deployment names

aef4ad1

Signed-off-by: Cindy Zhang <[email protected]>

edoakes approved these changes Apr 25, 2023

View reviewed changes

sihanwang41 reviewed Apr 25, 2023

View reviewed changes

zcin added 3 commits April 25, 2023 18:33

address comments

e13b317

Signed-off-by: Cindy Zhang <[email protected]>

add e2e tests for health check timout and health check period

6b09917

Signed-off-by: Cindy Zhang <[email protected]>

fix

127a4ed

Signed-off-by: Cindy Zhang <[email protected]>

sihanwang41 approved these changes Apr 26, 2023

View reviewed changes

Merge branch 'master' into version-improvements

8a1edd6

Signed-off-by: Cindy Zhang <[email protected]>

zcin mentioned this pull request Apr 26, 2023

[serve] Better surfacing of errors in serve status #34773

Merged

8 tasks

edoakes merged commit bc1c378 into ray-project:master Apr 26, 2023

zcin mentioned this pull request May 2, 2023

[serve] Controller seems to always re-deploy deployments on controller restart #34097

Closed

JalinWang mentioned this pull request Aug 31, 2023

[Serve] fail to detect changes in code and re-deploy #38714

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[serve][deploy refactor][2/X] Move lightweight update logic to DeploymentVersion #34430

[serve][deploy refactor][2/X] Move lightweight update logic to DeploymentVersion #34430

zcin commented Apr 14, 2023 •

edited

Loading

zcin Apr 18, 2023

edoakes Apr 18, 2023

edoakes Apr 18, 2023

zcin Apr 21, 2023

edoakes commented Apr 18, 2023

sihanwang41 Apr 21, 2023

zcin Apr 21, 2023 •

edited

Loading

edoakes Apr 24, 2023

sihanwang41 Apr 21, 2023

sihanwang41 Apr 21, 2023

zcin Apr 25, 2023

sihanwang41 Apr 21, 2023

zcin Apr 21, 2023

sihanwang41 Apr 21, 2023

zcin Apr 21, 2023

sihanwang41 Apr 24, 2023

zcin Apr 25, 2023

edoakes left a comment

edoakes commented Apr 25, 2023

zcin commented Apr 25, 2023 •

edited

Loading

sihanwang41 Apr 25, 2023

zcin Apr 26, 2023

sihanwang41 Apr 25, 2023 •

edited

Loading

zcin Apr 26, 2023

sihanwang41 commented Apr 26, 2023

zcin commented Apr 26, 2023

[serve][deploy refactor][2/X] Move lightweight update logic to DeploymentVersion #34430

[serve][deploy refactor][2/X] Move lightweight update logic to DeploymentVersion #34430

Conversation

zcin commented Apr 14, 2023 • edited Loading

Why are these changes needed?

Related issue number

Checks

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edoakes commented Apr 18, 2023

Choose a reason for hiding this comment

zcin Apr 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edoakes left a comment

Choose a reason for hiding this comment

edoakes commented Apr 25, 2023

zcin commented Apr 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sihanwang41 Apr 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sihanwang41 commented Apr 26, 2023

zcin commented Apr 26, 2023

zcin commented Apr 14, 2023 •

edited

Loading

zcin Apr 21, 2023 •

edited

Loading

zcin commented Apr 25, 2023 •

edited

Loading

sihanwang41 Apr 25, 2023 •

edited

Loading