[Serve] Track user-configured options in Serve deployments #28313

shrekris-anyscale · 2022-09-06T23:15:10Z

Signed-off-by: Shreyas Krishnaswamy [email protected]

Why are these changes needed?

Background
Serve sets deployment config defaults directly via keyword arguments in the @serve.deployment decorator. This makes it it makes it impossible to track whether the config options were set by default or by the user. Distinguishing between these would let serve build print only the user-configured options, instead of all options (including defaults).

Changes
This change adds a layer of indirection in the @serve.deployment decorator. It sets all the decorator config options to an enum element, DEFAULT.VALUE, by default. This element indicates that the config setting was set by default– not by the user. The Serve DeploymentSchema is also updated to use DEFAULT.VALUE as default.

This change also introduces a new Set called user_configured_option_names to the DeploymentConfig, which tracks the group of options configured by the user. When Serve is schematized or serialized, it can use the names in this set to indicate which options are default and which ones are user-configured.

serve build now prints only user-configured values (as well as deployment names) to the Serve config file.

None is no longer the default value for deployment options. The only options that can be set to None (or null in the Serve config) are:

route_prefix: None means the deployment should not be exposed over HTTP.
num_replicas: None is allowed if an autoscaling_config is provided.
autoscaling_config: None is allowed if a num_replicas is provided.
user_config: None indicates that there's no user_config to pass into reconfigure.

Follow-up Changes

serve build should offer a flag that lets users print all options, including the defaults. This is especially useful for users that want to explicitly track all options settings. It may also be useful to always print the route_prefixes for Serve deployments if they're not None to easily see which deployment is the driver.
Plumb the Serve codebase to actually make certain options non-optional (e.g. ray_actor_options). This change only modifies the thin wrappers over the Deployment object (i.e. the @serve.deployment decorator and the Serve schemas) but not the Deployment object itself.

Related issue number

N/A

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 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: Shreyas Krishnaswamy <[email protected]>

sihanwang41 · 2022-09-14T16:19:17Z

python/ray/serve/api.py

+    init_kwargs: Default[Dict[Any, Any]] = DEFAULT.VALUE,
+    route_prefix: Default[Union[str, None]] = DEFAULT.VALUE,
+    ray_actor_options: Default[Dict] = DEFAULT.VALUE,
+    user_config: Default[Optional[Any]] = DEFAULT.VALUE,


Please correct me if i am wrong, we are having DEFAULT.VALUE is 1, what does it mean for setting every attribute to 1 here?

It’s not quite setting every attribute to 1 here. DEFAULT.VALUE is a Python enum element with an arbitrary value (in this case 1). It’s not an alias for 1. It could be set to any other arbitrary value (e.g. 2, “hello”, etc.) and the code here would still be the same.

I’m using DEFAULT.VALUE as the default because the user will never pass that in as a value for a deployment option (since it’s meaningless to the user and it would require them to import the enum from the Serve codebase). So in the decorator body, if any attributes are set to DEFAULT.VALUE, we know that the user didn’t set them. That lets us populate user_configured_options correctly.

In [3]: from enum import Enum, auto In [4]: class DEFAULT(Enum): ...: VALUE = auto() ...: In [5]: DEFAULT.VALUE Out[5]: <DEFAULT.VALUE: 1> In [6]: DEFAULT.VALUE == 1 Out[6]: False In [7]: DEFAULT.VALUE == DEFAULT.VALUE Out[7]: True

simon-mo · 2022-09-19T19:19:00Z

python/ray/serve/BUILD

@@ -49,6 +49,14 @@ py_test(
    deps = [":serve_lib"],
 )

+py_test(
+    name = "test_deployment",


more descriptive naming please

I renamed it to test_deployment_class. Let me know if you have other suggestions. I'm trying to differentiate these tests, which are about the Deployment class and how it manipulates data, and the Deployment objects that actually run on the Ray cluster.

simon-mo · 2022-09-19T19:19:43Z

python/ray/serve/_private/json_serde.py

@@ -88,7 +88,7 @@ def default(self, obj):
        if isinstance(obj, DeploymentSchema):
            return {
                DAGNODE_TYPE_KEY: "DeploymentSchema",
-                "schema": obj.dict(),
+                "schema": obj.dict(exclude_defaults=True),


add a comment on why do we need this flag

Sounds good, I added a comment.

simon-mo · 2022-09-19T19:20:23Z

python/ray/serve/config.py

-            ignore_none: When True, any valid keywords with value None
-                are ignored, and their values stay default. Invalid keywords
-                still raise a TypeError.
+            ignore_default: When True, any valid keywords with value


this flag appear to be unused

Good catch– I removed the argument from the docstring.

simon-mo · 2022-09-19T19:21:11Z

src/ray/protobuf/serve.proto

@@ -88,6 +88,8 @@ message DeploymentConfig {
  AutoscalingConfig autoscaling_config = 10;

  string version = 11;
+
+  repeated string user_configured_options = 12;


why is this repeated string type?

In the protobuf, user_configured_options is a list of strings. Each string is a deployment option that was manually tuned by the user.

I used repeated string because it seemed to best match a list of strings. Is there another datatype I should use?

Each string is a deployment options that was manually tuned by the user.

Ahhh these are options keys. not the value. can you change the variable name so it implies that?

Good idea, I renamed user_configured_options to user_configured_option_names.

Signed-off-by: Shreyas Krishnaswamy <[email protected]>

simon-mo · 2022-09-21T21:51:08Z

python/ray/serve/_private/json_serde.py

@@ -88,7 +88,10 @@ def default(self, obj):
        if isinstance(obj, DeploymentSchema):
            return {
                DAGNODE_TYPE_KEY: "DeploymentSchema",
-                "schema": obj.dict(),
+                # The schema's default values are Python enums that aren't
+                # JSON-serializable. exclude_defaults omits these, so the


Suggested change

# JSON-serializable. exclude_defaults omits these, so the

# JSON-serializable by design. exclude_defaults omits these, so the

simon-mo · 2022-09-21T21:52:31Z

python/ray/serve/deployment.py

+            _internal: If True, this function:
+                    1. Won't log deprecation warnings
+                    2. Won't update this deployment's config's
+                        user_configured_options.
+                Should only be True when used internally by Serve.
+                Should be False when called by users.


make these complete sentences in one section please.

simon-mo · 2022-09-21T21:58:42Z

python/ray/serve/_private/utils.py

+# Type alias: objects that can be DEFAULT.VALUE have type Default[T]
+T = TypeVar("T")
+Default = Union[DEFAULT, T]


This is so cool, learned something new :D.

from typing import TypeVar, Union from enum import Enum, auto class DEFAULT(Enum): VALUE = auto() T = TypeVar("T") Default = Union[DEFAULT, T] def func(name: Default[str] = DEFAULT.VALUE): print(name) func() func("a") func(1)

$ mypy app.py app.py:15: error: Argument 1 to "func" has incompatible type "int"; expected "Union[DEFAULT, str]" Found 1 error in 1 file (checked 1 source file)

simon-mo · 2022-09-21T22:00:49Z

python/ray/serve/scripts.py

+    # Remove extraneous newline
+    config_str = config_str[:-1]


is this really necessary? if so, please use config.rstrip("\n") so it's more readable

Without that line, the file ends with two newlines. Ideally, it should end with a single newline.

I've replaced that snippet with:

# Ensure file ends with only one newline config_str = config_str.rstrip("\n") + "\n"

simon-mo · 2022-09-21T22:02:17Z

src/ray/protobuf/serve.proto

@@ -88,6 +88,8 @@ message DeploymentConfig {
  AutoscalingConfig autoscaling_config = 10;

  string version = 11;
+
+  repeated string user_configured_options = 12;


Each string is a deployment options that was manually tuned by the user.

Ahhh these are options keys. not the value. can you change the variable name so it implies that?

Signed-off-by: Shreyas Krishnaswamy <[email protected]>

sihanwang41 · 2022-09-29T16:48:59Z

python/ray/serve/api.py

+    # Create list of all user-configured options from keyword args
+    user_configured_option_names = [
+        option
+        for option, value in locals().items()


maybe add a comment, we should not have any local variable because of using locals()? Or Deployment can return you a user-configured attributes.

Ah good idea. We can still have local variables, but they should be defined after this list. Creating that list should be the first thing that happens in the function.

Yeah, the order is difficult to guarantee and audit, better to have a way to predefine user-configured attributes.

I think unit tests should be able to catch any issues with the ordering since any local variables that are defined before this list will show up as a user-configured attribute and cause the unit tests to fail. What do you mean by predefine user-configured attributes?

Deployment can provide the user_configured_option_names attributes, you can filter the attributes from locals().

Signed-off-by: Shreyas Krishnaswamy <[email protected]>

shrekris-anyscale · 2022-09-29T22:32:20Z

The remaining test failures are unrelated:

…ct#28313) Signed-off-by: Weichen Xu <[email protected]>

shrekris-anyscale added 23 commits September 6, 2022 16:09

Make default values DEFAULT.VALUE

a38756a

Signed-off-by: Shreyas Krishnaswamy <[email protected]>

Update types

23b7c9b

Signed-off-by: Shreyas Krishnaswamy <[email protected]>

Check for DEFAULT.VALUE in deployment decorator body

0523be8

Signed-off-by: Shreyas Krishnaswamy <[email protected]>

Update from_default to process DEFAULT.VALUE instead of None

0e245ee

Signed-off-by: Shreyas Krishnaswamy <[email protected]>

Update from_default() tests

4e62084

Signed-off-by: Shreyas Krishnaswamy <[email protected]>

Use DEFAULT.VALUE in schema defaults

fc57c3e

Signed-off-by: Shreyas Krishnaswamy <[email protected]>

Use DEFAULT.VALUE in schema validators

d09e71b

Signed-off-by: Shreyas Krishnaswamy <[email protected]>

Merge branch 'master' of github.com:ray-project/ray into default_options

48e291a

Add user_configured_options to DeploymentConfig

80a8a85

Signed-off-by: Shreyas Krishnaswamy <[email protected]>

Handle user_configured_options in decorator

411bd26

Signed-off-by: Shreyas Krishnaswamy <[email protected]>

Handle defaults in options()

fa5f4a6

Signed-off-by: Shreyas Krishnaswamy <[email protected]>

Remove defaults in RayActorOptionsSchema

2658d7e

Signed-off-by: Shreyas Krishnaswamy <[email protected]>

Handle defaults in schema helpers

dffaa2c

Signed-off-by: Shreyas Krishnaswamy <[email protected]>

Iterate through locals items() instead of locals directly

62a1258

Signed-off-by: Shreyas Krishnaswamy <[email protected]>

Flip order in list comprehensions

e4a0d8b

Signed-off-by: Shreyas Krishnaswamy <[email protected]>

Add missing not

5ffa7a7

Signed-off-by: Shreyas Krishnaswamy <[email protected]>

Fix dictionary iterations

a3f0e76

Signed-off-by: Shreyas Krishnaswamy <[email protected]>

Merge branch 'master' of github.com:ray-project/ray into default_options

4ec73c7

Keep name field when creating schema

8f84682

Signed-off-by: Shreyas Krishnaswamy <[email protected]>

Specify optional arguments

c1f83f9

Signed-off-by: Shreyas Krishnaswamy <[email protected]>

Remove defaults from serve build

190491e

Signed-off-by: Shreyas Krishnaswamy <[email protected]>

Disable flaky serve agent tests on mac

a64a651

Signed-off-by: Shreyas Krishnaswamy <[email protected]>

Exclude defaults during serialization

1fc6cbf

Signed-off-by: Shreyas Krishnaswamy <[email protected]>

shrekris-anyscale changed the title ~~[WIP] [Serve] Use DEFAULT.VALUE as default config values for deployments~~ [WIP] [Serve] Track user-configured options in Serve deployments Sep 13, 2022

shrekris-anyscale added 6 commits September 13, 2022 15:28

Introduce test_deployment.py

7ba9676

Signed-off-by: Shreyas Krishnaswamy <[email protected]>

Merge branch 'master' of github.com:ray-project/ray into default_options

c5748e3

Make ray_actor_options not nullable

aa075b6

Signed-off-by: Shreyas Krishnaswamy <[email protected]>

Add tests for user_configured_options

b7c34c0

Signed-off-by: Shreyas Krishnaswamy <[email protected]>

Add test for empty user_config

ba0892e

Signed-off-by: Shreyas Krishnaswamy <[email protected]>

Create test_nullable_options

9b807f6

Signed-off-by: Shreyas Krishnaswamy <[email protected]>

shrekris-anyscale added 2 commits September 13, 2022 18:17

Add test for nullable options to schema

90a15dd

Signed-off-by: Shreyas Krishnaswamy <[email protected]>

Remove route_prefixes from serve build output

af68e2e

Signed-off-by: Shreyas Krishnaswamy <[email protected]>

shrekris-anyscale changed the title ~~[WIP] [Serve] Track user-configured options in Serve deployments~~ [Serve] Track user-configured options in Serve deployments Sep 14, 2022

shrekris-anyscale requested review from simon-mo and sihanwang41 September 14, 2022 01:29

shrekris-anyscale assigned simon-mo and sihanwang41 Sep 14, 2022

Make size 'small'

696ae58

Signed-off-by: Shreyas Krishnaswamy <[email protected]>

sihanwang41 reviewed Sep 14, 2022

View reviewed changes

simon-mo reviewed Sep 19, 2022

View reviewed changes

shrekris-anyscale added 4 commits September 20, 2022 14:17

Merge branch 'master' of github.com:ray-project/ray into default_options

9a3cb6b

Remove unused arg from docstring in from_default

0463c2e

Signed-off-by: Shreyas Krishnaswamy <[email protected]>

Add comment about flag

2fba994

Signed-off-by: Shreyas Krishnaswamy <[email protected]>

Rename test_deployment to test_deployment_class

67fca53

Signed-off-by: Shreyas Krishnaswamy <[email protected]>

simon-mo approved these changes Sep 21, 2022

View reviewed changes

shrekris-anyscale added 5 commits September 27, 2022 14:04

Merge branch 'master' of github.com:ray-project/ray into default_options

f1b9bb7

Address comments

6f5502c

Signed-off-by: Shreyas Krishnaswamy <[email protected]>

Rename user_configured_options to user_configured_option_names

842ceee

Signed-off-by: Shreyas Krishnaswamy <[email protected]>

Merge branch 'master' of github.com:ray-project/ray into default_options

23e7ce5

Add test for options

f871ef0

Signed-off-by: Shreyas Krishnaswamy <[email protected]>

sihanwang41 approved these changes Sep 29, 2022

View reviewed changes

shrekris-anyscale added 2 commits September 29, 2022 13:08

Add note about where user_configured_options should be defined

1bfb333

Signed-off-by: Shreyas Krishnaswamy <[email protected]>

Make test_working_dir_scale_up_in_new_driver use deployment.deploy()

b41a42c

Signed-off-by: Shreyas Krishnaswamy <[email protected]>

simon-mo merged commit 18b38c5 into ray-project:master Sep 29, 2022

shrekris-anyscale mentioned this pull request Oct 6, 2022

[Serve] [Docs] Update serve build documentation #29147

Merged

4 tasks

WeichenXu123 pushed a commit to WeichenXu123/ray that referenced this pull request Dec 19, 2022

[Serve] Track user-configured options in Serve deployments (ray-proje…

073b31c

…ct#28313) Signed-off-by: Weichen Xu <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Serve] Track user-configured options in Serve deployments #28313

[Serve] Track user-configured options in Serve deployments #28313

shrekris-anyscale commented Sep 6, 2022 •

edited

Loading

sihanwang41 Sep 14, 2022

shrekris-anyscale Sep 15, 2022

simon-mo Sep 19, 2022

simon-mo Sep 19, 2022

shrekris-anyscale Sep 20, 2022

simon-mo Sep 19, 2022

shrekris-anyscale Sep 20, 2022

simon-mo Sep 19, 2022

shrekris-anyscale Sep 20, 2022

simon-mo Sep 19, 2022

shrekris-anyscale Sep 20, 2022 •

edited

Loading

simon-mo Sep 21, 2022

shrekris-anyscale Sep 27, 2022

simon-mo Sep 21, 2022

simon-mo Sep 21, 2022

simon-mo Sep 21, 2022

simon-mo Sep 21, 2022

shrekris-anyscale Sep 27, 2022

simon-mo Sep 21, 2022

sihanwang41 Sep 29, 2022

shrekris-anyscale Sep 29, 2022

sihanwang41 Sep 29, 2022

shrekris-anyscale Sep 29, 2022

sihanwang41 Sep 29, 2022

shrekris-anyscale commented Sep 29, 2022

	# JSON-serializable. exclude_defaults omits these, so the
	# JSON-serializable by design. exclude_defaults omits these, so the

[Serve] Track user-configured options in Serve deployments #28313

[Serve] Track user-configured options in Serve deployments #28313

Conversation

shrekris-anyscale commented Sep 6, 2022 • 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

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

shrekris-anyscale Sep 20, 2022 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shrekris-anyscale commented Sep 29, 2022

shrekris-anyscale commented Sep 6, 2022 •

edited

Loading

shrekris-anyscale Sep 20, 2022 •

edited

Loading