[Serve] fix autoscaling json serialization issue #42339

GeneDer · 2024-01-11T18:38:01Z

Why are these changes needed?

TDD ensure to add test cases that will reveal the issue and fix the issue by serialize bytes object into base64 encoded string.

Related issue number

Closes: #42319

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: Gene Su <[email protected]>

GeneDer · 2024-01-11T18:39:05Z

Shoutout to @zcin for finding this issue!

edoakes

The serialized class definition should not be returned in ServeInstanceDetails at all. This is a user-facing struct that is returned in the REST API and the serialized class definition is purely an implementation detail.

python/ray/serve/schema.py

Signed-off-by: Gene Su <[email protected]>

python/ray/serve/schema.py

Signed-off-by: Gene Su <[email protected]>

python/ray/serve/tests/unit/test_schema.py

edoakes · 2024-01-11T22:38:24Z

python/ray/serve/schema.py

+ # `serialized_policy_def` is only used internally and should not be exposed to
+ # the REST api. This method iteratively removes it from each autoscaling config
+ # if exists.


what'll happen here if the user passed a class definition directly for the autoscaling policy?

policy should carry the callable, serialized_policy_def should continue to be hidden away from users

Right, but a callable is not JSON-serializable, so won't this error?

No error, added test cases for none, string, and callable policies https://github.com/ray-project/ray/pull/42339/files#diff-1eaedadea6f2e3382aee6726429ebaad94801e3c55592b3e9f84a0d1b7cc1a14R80

What will the JSON serialized output be if the user passes in a callable?

Oh the policy is actually not part of the response.

{ 'applications': { 'default': { 'deployed_app_config': None, 'deployments': { 'autoscaling_app': { 'deployment_config': { 'autoscaling_config': { 'downscale_delay_s': 600.0, 'downscale_smoothing_factor': None, 'look_back_period_s': 30.0, 'max_replicas': 10, 'metrics_interval_s': 10.0, 'min_replicas': 1, 'smoothing_factor': 1.0, 'target_num_ongoing_requests_per_replica': 1.0, 'upscale_delay_s': 30.0, 'upscale_smoothing_factor': None}, 'graceful_shutdown_timeout_s': 20.0, 'graceful_shutdown_wait_loop_s': 2.0, 'health_check_period_s': 10.0, 'health_check_timeout_s': 30.0, 'max_concurrent_queries': 100, 'name': 'autoscaling_app', 'ray_actor_options': { 'num_cpus': 1.0, 'runtime_env': { }}, 'user_config': None}, 'message': '', 'name': 'autoscaling_app', 'replicas': [ { 'actor_id': '171b2a617ac029cb037c941c01000000', 'actor_name': 'SERVE_REPLICA::default#autoscaling_app#9fydf52f', 'log_file_path': '/serve/replica_default_autoscaling_app_9fydf52f.log', 'node_id': '39d23fbf6cc4e348a7eae48648ed36c1644951a06daf12d3ba43658e', 'node_ip': '127.0.0.1', 'pid': 39584, 'replica_id': 'default#autoscaling_app#9fydf52f', 'start_time_s': 1705093459.648952, 'state': <ReplicaState.RUNNING: 'RUNNING'>, 'worker_id': '699cf6124f76f5c6902956ef7e859c59f580c98202c121109eb88815'}], 'status': <DeploymentStatus.HEALTHY: 'HEALTHY'>, 'status_trigger': <DeploymentStatusTrigger.CONFIG_UPDATE_COMPLETED: 'CONFIG_UPDATE_COMPLETED'>}}, 'docs_path': None, 'last_deployed_time_s': 1705093459.4853802, 'message': '', 'name': 'default', 'route_prefix': '/', 'status': <ApplicationStatus.RUNNING: 'RUNNING'>}}, 'controller_info': { 'actor_id': '9582543ca6801417fcd6370601000000', 'actor_name': 'SERVE_CONTROLLER_ACTOR', 'log_file_path': '/serve/controller_39559.log', 'node_id': '39d23fbf6cc4e348a7eae48648ed36c1644951a06daf12d3ba43658e', 'node_ip': '127.0.0.1', 'worker_id': 'f4f421a9b26e8f281f3c2f40cffd6c96069e4bedc4dc00937d3cc0a6'}, 'grpc_options': {}, 'http_options': {'host': '0.0.0.0'}, 'proxies': { '39d23fbf6cc4e348a7eae48648ed36c1644951a06daf12d3ba43658e': { 'actor_id': '35092b45d842d61dab6acc9701000000', 'actor_name': 'SERVE_PROXY_ACTOR-39d23fbf6cc4e348a7eae48648ed36c1644951a06daf12d3ba43658e', 'log_file_path': '/serve/proxy_127.0.0.1.log', 'node_id': '39d23fbf6cc4e348a7eae48648ed36c1644951a06daf12d3ba43658e', 'node_ip': '127.0.0.1', 'status': <ProxyStatus.HEALTHY: 'HEALTHY'>, 'worker_id': '3e4653db9129065ab21604b19aed6c4e22dc464120a8b0ab96346317'}},

let me see why that is

ok this is due to we do not include policy in the AutoscalingConfig

ray/src/ray/protobuf/serve.proto

Line 65 in f76081f

bytes serialized_policy_def = 12;

so when it's serialized the data back to deployment info, it doesn't contain that field. Kinda try out a few things, and just decided to add a new user facing field policy_name in the status. Let me know what you think. This is how it looks after

{ 'applications': { 'default': { 'deployed_app_config': None, 'deployments': { 'autoscaling_app': { 'deployment_config': { 'autoscaling_config': { 'downscale_delay_s': 600.0, 'downscale_smoothing_factor': None, 'look_back_period_s': 30.0, 'max_replicas': 10, 'metrics_interval_s': 10.0, 'min_replicas': 1, 'policy_name': 'replica_queue_length_autoscaling_policy', ## <---new field 'smoothing_factor': 1.0, 'target_num_ongoing_requests_per_replica': 1.0, 'upscale_delay_s': 30.0, 'upscale_smoothing_factor': None}, 'graceful_shutdown_timeout_s': 20.0, 'graceful_shutdown_wait_loop_s': 2.0, 'health_check_period_s': 10.0, 'health_check_timeout_s': 30.0, 'max_concurrent_queries': 100, 'name': 'autoscaling_app', 'ray_actor_options': { 'num_cpus': 1.0, 'runtime_env': { }}, 'user_config': None}, 'message': '', 'name': 'autoscaling_app', 'replicas': [ { 'actor_id': '2777ad457fa57bbb2480273301000000', 'actor_name': 'SERVE_REPLICA::default#autoscaling_app#qk092qkb', 'log_file_path': '/serve/replica_default_autoscaling_app_qk092qkb.log', 'node_id': '2f61e8786194040ab4a43972a15dc2d1de41e368a36a1061bbbc9e38', 'node_ip': '127.0.0.1', 'pid': 43655, 'replica_id': 'default#autoscaling_app#qk092qkb', 'start_time_s': 1705099874.064668, 'state': <ReplicaState.RUNNING: 'RUNNING'>, 'worker_id': 'a91e56f6fcd2d97bf8ec0a1cb0d036a3066bd4527ca5ccd9ba9a0821'}], 'status': <DeploymentStatus.HEALTHY: 'HEALTHY'>, 'status_trigger': <DeploymentStatusTrigger.CONFIG_UPDATE_COMPLETED: 'CONFIG_UPDATE_COMPLETED'>}}, 'docs_path': None, 'last_deployed_time_s': 1705099873.9227, 'message': '', 'name': 'default', 'route_prefix': '/', 'status': <ApplicationStatus.RUNNING: 'RUNNING'>}}, 'controller_info': { 'actor_id': 'abe00c23c7d83f4b75ad022a01000000', 'actor_name': 'SERVE_CONTROLLER_ACTOR', 'log_file_path': '/serve/controller_43653.log', 'node_id': '2f61e8786194040ab4a43972a15dc2d1de41e368a36a1061bbbc9e38', 'node_ip': '127.0.0.1', 'worker_id': '0531e60d5965a616fca6baee32ac311bdfdabf420a744b77f8b6ee28'}, 'grpc_options': {}, 'http_options': {'host': '0.0.0.0'}, 'proxies': { '2f61e8786194040ab4a43972a15dc2d1de41e368a36a1061bbbc9e38': { 'actor_id': 'e3a47ef12f2a2c8848e5875e01000000', 'actor_name': 'SERVE_PROXY_ACTOR-2f61e8786194040ab4a43972a15dc2d1de41e368a36a1061bbbc9e38', 'log_file_path': '/serve/proxy_127.0.0.1.log', 'node_id': '2f61e8786194040ab4a43972a15dc2d1de41e368a36a1061bbbc9e38', 'node_ip': '127.0.0.1', 'status': <ProxyStatus.HEALTHY: 'HEALTHY'>, 'worker_id': 'dd0916116d09ae0bb83a8b7b53459465bb60101a04048805a14105ca'}}, 'proxy_location': <ProxyLocation.HeadOnly: 'HeadOnly'>, 'target_capacity': None}

Including a field in the response that isn't part of the input config schema is a little odd and breaks the existing behavior where this output can be directly used as input to apply the config

I'd propose the following:

Include the autoscaling_policy field.

If the user gave an import path, we return it in the field.

If the user gave a Callable, we get its import path to the best of our ability (f"{cls.__module__}.{cls.__name__}") and return it in the field

Signed-off-by: Gene Su <[email protected]>

GeneDer

@edoakes @sihanwang41 Addressed all the comments, PTAL again when you have a chance :)

sihanwang41 · 2024-01-12T18:00:53Z

python/ray/serve/tests/unit/test_schema.py

+ json.dumps(details)
+
+ # ensure internal field, serialized_policy_def, is not exposed
+ application = details["applications"]["app1"]


nit: the test case name is checking json serialization, suggest to add expected result and do strict compare between json output and expected output, so that we can prevent other fields in the future.

I feel the test should only check the behavior we expect, in this case the output should be json serializable and not containing internally field (serialized_policy_def). Checking the exact output would be too rigid and hard to maintain. If not a strong preference I would keep this as it :)

sihanwang41 · 2024-01-12T18:03:40Z

python/ray/serve/tests/test_controller.py

+@pytest.mark.parametrize(
+ "policy", [None, DEFAULT_AUTOSCALING_POLICY, default_autoscaling_policy]
+)
+def test_get_serve_instance_details_json_serializable(serve_instance, policy):


nit: same here. we don't need to check serialized_policy_def specifically.

Signed-off-by: Gene Su <[email protected]>

… autoscaling config Signed-off-by: Gene Su <[email protected]>

Signed-off-by: Gene Su <[email protected]>

GeneDer added 2 commits January 11, 2024 10:30

[Serve] fix autoscaling json serialization issue

b6b5041

Signed-off-by: Gene Su <[email protected]>

fixups

682eba9

Signed-off-by: Gene Su <[email protected]>

GeneDer marked this pull request as ready for review January 11, 2024 18:38

GeneDer requested a review from a team January 11, 2024 18:38

edoakes requested changes Jan 11, 2024

View reviewed changes

sihanwang41 reviewed Jan 11, 2024

View reviewed changes

python/ray/serve/schema.py Outdated Show resolved Hide resolved

GeneDer added 2 commits January 11, 2024 13:42

explicitly remove serialized_policy_def so it's not exposed to the users

f04d104

Signed-off-by: Gene Su <[email protected]>

fixup

6562d05

Signed-off-by: Gene Su <[email protected]>

sihanwang41 reviewed Jan 11, 2024

View reviewed changes

python/ray/serve/schema.py Outdated Show resolved Hide resolved

not to expose implementation details to the public api

8ee4bd6

Signed-off-by: Gene Su <[email protected]>

GeneDer force-pushed the fix-autoscaling-config-json-serializable branch from 5f0906e to 8ee4bd6 Compare January 11, 2024 22:19

edoakes reviewed Jan 11, 2024

View reviewed changes

python/ray/serve/tests/unit/test_schema.py Outdated Show resolved Hide resolved

edoakes reviewed Jan 11, 2024

View reviewed changes

GeneDer added 2 commits January 11, 2024 15:19

address comments

10935b3

Signed-off-by: Gene Su <[email protected]>

fix edge case

a9e213b

Signed-off-by: Gene Su <[email protected]>

GeneDer commented Jan 12, 2024

View reviewed changes

sihanwang41 approved these changes Jan 12, 2024

View reviewed changes

GeneDer added 6 commits January 12, 2024 14:53

add policy_name to expose the name of the function to the user

efd3421

Signed-off-by: Gene Su <[email protected]>

format callable policy to be policy.__module__.policy.__name__ in the…

612b155

… autoscaling config Signed-off-by: Gene Su <[email protected]>

fix edge case

bdd787e

Signed-off-by: Gene Su <[email protected]>

add tests to match the exact content of the output

a631b6c

Signed-off-by: Gene Su <[email protected]>

fix

58ffa02

Signed-off-by: Gene Su <[email protected]>

Merge branch 'master' into fix-autoscaling-config-json-serializable

797a27c

edoakes approved these changes Jan 16, 2024

View reviewed changes

edoakes merged commit 0e02dbf into ray-project:master Jan 16, 2024
9 checks passed

GeneDer deleted the fix-autoscaling-config-json-serializable branch January 17, 2024 00:07

zcin pushed a commit to zcin/ray that referenced this pull request Jan 17, 2024

[Serve] fix autoscaling json serialization issue (ray-project#42339)

4deb25e

Signed-off-by: Gene Su <[email protected]>

zcin pushed a commit to zcin/ray that referenced this pull request Feb 6, 2024

[Serve] fix autoscaling json serialization issue (ray-project#42339)

2ec532b

Signed-off-by: Gene Su <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Serve] fix autoscaling json serialization issue #42339

[Serve] fix autoscaling json serialization issue #42339

GeneDer commented Jan 11, 2024

GeneDer commented Jan 11, 2024

edoakes left a comment

edoakes Jan 11, 2024

GeneDer Jan 11, 2024

edoakes Jan 12, 2024

GeneDer Jan 12, 2024

edoakes Jan 12, 2024

GeneDer Jan 12, 2024 •

edited

Loading

GeneDer Jan 12, 2024

edoakes Jan 12, 2024 •

edited

Loading

GeneDer left a comment

sihanwang41 Jan 12, 2024

GeneDer Jan 12, 2024

sihanwang41 Jan 12, 2024

[Serve] fix autoscaling json serialization issue #42339

[Serve] fix autoscaling json serialization issue #42339

Conversation

GeneDer commented Jan 11, 2024

Why are these changes needed?

Related issue number

Checks

GeneDer commented Jan 11, 2024

edoakes left a 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

GeneDer Jan 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edoakes Jan 12, 2024 • edited Loading

Choose a reason for hiding this comment

GeneDer left a 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

GeneDer Jan 12, 2024 •

edited

Loading

edoakes Jan 12, 2024 •

edited

Loading