From a38756a326b3087238171cdee721540962f4e69e Mon Sep 17 00:00:00 2001 From: Shreyas Krishnaswamy Date: Tue, 6 Sep 2022 16:09:56 -0700 Subject: [PATCH 01/37] Make default values DEFAULT.VALUE Signed-off-by: Shreyas Krishnaswamy --- python/ray/serve/api.py | 52 ++++++++++++++++++++--------------------- 1 file changed, 26 insertions(+), 26 deletions(-) diff --git a/python/ray/serve/api.py b/python/ray/serve/api.py index 611b665a408c..aaf569b29eb8 100644 --- a/python/ray/serve/api.py +++ b/python/ray/serve/api.py @@ -253,20 +253,20 @@ def deployment(func_or_class: Callable) -> Deployment: @overload def deployment( - name: Optional[str] = None, - version: Optional[str] = None, - num_replicas: Optional[int] = None, - init_args: Optional[Tuple[Any]] = None, - init_kwargs: Optional[Dict[Any, Any]] = None, + name: Optional[str] = DEFAULT.VALUE, + version: Optional[str] = DEFAULT.VALUE, + num_replicas: Optional[int] = DEFAULT.VALUE, + init_args: Optional[Tuple[Any]] = DEFAULT.VALUE, + init_kwargs: Optional[Dict[Any, Any]] = DEFAULT.VALUE, route_prefix: Union[str, None, DEFAULT] = DEFAULT.VALUE, - ray_actor_options: Optional[Dict] = None, - user_config: Optional[Any] = None, - max_concurrent_queries: Optional[int] = None, - autoscaling_config: Optional[Union[Dict, AutoscalingConfig]] = None, - graceful_shutdown_wait_loop_s: Optional[float] = None, - graceful_shutdown_timeout_s: Optional[float] = None, - health_check_period_s: Optional[float] = None, - health_check_timeout_s: Optional[float] = None, + ray_actor_options: Optional[Dict] = DEFAULT.VALUE, + user_config: Optional[Any] = DEFAULT.VALUE, + max_concurrent_queries: Optional[int] = DEFAULT.VALUE, + autoscaling_config: Optional[Union[Dict, AutoscalingConfig]] = DEFAULT.VALUE, + graceful_shutdown_wait_loop_s: Optional[float] = DEFAULT.VALUE, + graceful_shutdown_timeout_s: Optional[float] = DEFAULT.VALUE, + health_check_period_s: Optional[float] = DEFAULT.VALUE, + health_check_timeout_s: Optional[float] = DEFAULT.VALUE, ) -> Callable[[Callable], Deployment]: pass @@ -274,20 +274,20 @@ def deployment( @PublicAPI(stability="beta") def deployment( _func_or_class: Optional[Callable] = None, - name: Optional[str] = None, - version: Optional[str] = None, - num_replicas: Optional[int] = None, - init_args: Optional[Tuple[Any]] = None, - init_kwargs: Optional[Dict[Any, Any]] = None, + name: Optional[str] = DEFAULT.VALUE, + version: Optional[str] = DEFAULT.VALUE, + num_replicas: Optional[int] = DEFAULT.VALUE, + init_args: Optional[Tuple[Any]] = DEFAULT.VALUE, + init_kwargs: Optional[Dict[Any, Any]] = DEFAULT.VALUE, route_prefix: Union[str, None, DEFAULT] = DEFAULT.VALUE, - ray_actor_options: Optional[Dict] = None, - user_config: Optional[Any] = None, - max_concurrent_queries: Optional[int] = None, - autoscaling_config: Optional[Union[Dict, AutoscalingConfig]] = None, - graceful_shutdown_wait_loop_s: Optional[float] = None, - graceful_shutdown_timeout_s: Optional[float] = None, - health_check_period_s: Optional[float] = None, - health_check_timeout_s: Optional[float] = None, + ray_actor_options: Optional[Dict] = DEFAULT.VALUE, + user_config: Optional[Any] = DEFAULT.VALUE, + max_concurrent_queries: Optional[int] = DEFAULT.VALUE, + autoscaling_config: Optional[Union[Dict, AutoscalingConfig]] = DEFAULT.VALUE, + graceful_shutdown_wait_loop_s: Optional[float] = DEFAULT.VALUE, + graceful_shutdown_timeout_s: Optional[float] = DEFAULT.VALUE, + health_check_period_s: Optional[float] = DEFAULT.VALUE, + health_check_timeout_s: Optional[float] = DEFAULT.VALUE, ) -> Callable[[Callable], Deployment]: """Define a Serve deployment. From 23b7c9b6a555c15ed76b734719d0a45e43a24cf2 Mon Sep 17 00:00:00 2001 From: Shreyas Krishnaswamy Date: Tue, 6 Sep 2022 17:23:18 -0700 Subject: [PATCH 02/37] Update types Signed-off-by: Shreyas Krishnaswamy --- python/ray/serve/_private/utils.py | 7 +- python/ray/serve/api.py | 100 +++++++++++++++-------------- 2 files changed, 57 insertions(+), 50 deletions(-) diff --git a/python/ray/serve/_private/utils.py b/python/ray/serve/_private/utils.py index 0b51c9ad6ae6..49c6778730d0 100644 --- a/python/ray/serve/_private/utils.py +++ b/python/ray/serve/_private/utils.py @@ -9,7 +9,7 @@ import traceback from enum import Enum from functools import wraps -from typing import Dict, Iterable, List, Tuple +from typing import Dict, Iterable, List, Tuple, TypeVar, Union import fastapi.encoders import numpy as np @@ -41,6 +41,11 @@ class DEFAULT(Enum): VALUE = 1 +# Type alias: objects that can be DEFAULT.VALUE have type Default[T] +T = TypeVar("T") +Default = Union[DEFAULT, T] + + def parse_request_item(request_item): if len(request_item.args) == 1: arg = request_item.args[0] diff --git a/python/ray/serve/api.py b/python/ray/serve/api.py index aaf569b29eb8..5669fab95a76 100644 --- a/python/ray/serve/api.py +++ b/python/ray/serve/api.py @@ -39,6 +39,7 @@ from ray.serve._private.logging_utils import LoggingContext from ray.serve._private.utils import ( DEFAULT, + Default, ensure_serialization_context, in_interactive_shell, install_serve_encoders_to_fastapi, @@ -253,20 +254,20 @@ def deployment(func_or_class: Callable) -> Deployment: @overload def deployment( - name: Optional[str] = DEFAULT.VALUE, - version: Optional[str] = DEFAULT.VALUE, - num_replicas: Optional[int] = DEFAULT.VALUE, - init_args: Optional[Tuple[Any]] = DEFAULT.VALUE, - init_kwargs: Optional[Dict[Any, Any]] = DEFAULT.VALUE, - route_prefix: Union[str, None, DEFAULT] = DEFAULT.VALUE, - ray_actor_options: Optional[Dict] = DEFAULT.VALUE, - user_config: Optional[Any] = DEFAULT.VALUE, - max_concurrent_queries: Optional[int] = DEFAULT.VALUE, - autoscaling_config: Optional[Union[Dict, AutoscalingConfig]] = DEFAULT.VALUE, - graceful_shutdown_wait_loop_s: Optional[float] = DEFAULT.VALUE, - graceful_shutdown_timeout_s: Optional[float] = DEFAULT.VALUE, - health_check_period_s: Optional[float] = DEFAULT.VALUE, - health_check_timeout_s: Optional[float] = DEFAULT.VALUE, + name: Default[str] = DEFAULT.VALUE, + version: Default[str] = DEFAULT.VALUE, + num_replicas: Default[int] = DEFAULT.VALUE, + init_args: Default[Tuple[Any]] = DEFAULT.VALUE, + 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[Any] = DEFAULT.VALUE, + max_concurrent_queries: Default[int] = DEFAULT.VALUE, + autoscaling_config: Default[Union[Dict, AutoscalingConfig]] = DEFAULT.VALUE, + graceful_shutdown_wait_loop_s: Default[float] = DEFAULT.VALUE, + graceful_shutdown_timeout_s: Default[float] = DEFAULT.VALUE, + health_check_period_s: Default[float] = DEFAULT.VALUE, + health_check_timeout_s: Default[float] = DEFAULT.VALUE, ) -> Callable[[Callable], Deployment]: pass @@ -274,57 +275,58 @@ def deployment( @PublicAPI(stability="beta") def deployment( _func_or_class: Optional[Callable] = None, - name: Optional[str] = DEFAULT.VALUE, - version: Optional[str] = DEFAULT.VALUE, - num_replicas: Optional[int] = DEFAULT.VALUE, - init_args: Optional[Tuple[Any]] = DEFAULT.VALUE, - init_kwargs: Optional[Dict[Any, Any]] = DEFAULT.VALUE, - route_prefix: Union[str, None, DEFAULT] = DEFAULT.VALUE, - ray_actor_options: Optional[Dict] = DEFAULT.VALUE, - user_config: Optional[Any] = DEFAULT.VALUE, - max_concurrent_queries: Optional[int] = DEFAULT.VALUE, - autoscaling_config: Optional[Union[Dict, AutoscalingConfig]] = DEFAULT.VALUE, - graceful_shutdown_wait_loop_s: Optional[float] = DEFAULT.VALUE, - graceful_shutdown_timeout_s: Optional[float] = DEFAULT.VALUE, - health_check_period_s: Optional[float] = DEFAULT.VALUE, - health_check_timeout_s: Optional[float] = DEFAULT.VALUE, + name: Default[str] = DEFAULT.VALUE, + version: Default[str] = DEFAULT.VALUE, + num_replicas: Default[int] = DEFAULT.VALUE, + init_args: Default[Tuple[Any]] = DEFAULT.VALUE, + 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[Any] = DEFAULT.VALUE, + max_concurrent_queries: Default[int] = DEFAULT.VALUE, + autoscaling_config: Default[Union[Dict, AutoscalingConfig]] = DEFAULT.VALUE, + graceful_shutdown_wait_loop_s: Default[float] = DEFAULT.VALUE, + graceful_shutdown_timeout_s: Default[float] = DEFAULT.VALUE, + health_check_period_s: Default[float] = DEFAULT.VALUE, + health_check_timeout_s: Default[float] = DEFAULT.VALUE, ) -> Callable[[Callable], Deployment]: """Define a Serve deployment. Args: - name (Optional[str]): Globally-unique name identifying this deployment. - If not provided, the name of the class or function will be used. - version [DEPRECATED] (Optional[str]): Version of the deployment. This is used to - indicate a code change for the deployment; when it is re-deployed - with a version change, a rolling update of the replicas will be - performed. If not provided, every deployment will be treated as a - new version. - num_replicas (Optional[int]): The number of processes to start up that + name (Default[str]): Globally-unique name identifying this + deployment. If not provided, the name of the class or function will + be used. + version [DEPRECATED] (Default[str]): Version of the deployment. + This is used to indicate a code change for the deployment; when it + is re-deployed with a version change, a rolling update of the + replicas will be performed. If not provided, every deployment will + be treated as a new version. + num_replicas (Default[int]): The number of processes to start up that will handle requests to this deployment. Defaults to 1. - init_args (Optional[Tuple]): Positional args to be passed to the class - constructor when starting up deployment replicas. These can also be - passed when you call `.deploy()` on the returned Deployment. - init_kwargs (Optional[Dict]): Keyword args to be passed to the class - constructor when starting up deployment replicas. These can also be - passed when you call `.deploy()` on the returned Deployment. - route_prefix (Optional[str]): Requests to paths under this HTTP path - prefix will be routed to this deployment. Defaults to '/{name}'. - When set to 'None', no HTTP endpoint will be created. + init_args (Default[Tuple[Any]]): Positional args to be passed to the + class constructor when starting up deployment replicas. These can + also be passed when you call `.deploy()` on the returned Deployment. + init_kwargs (Default[Dict[Any, Any]]): Keyword args to be passed to the + class constructor when starting up deployment replicas. These can + also be passed when you call `.deploy()` on the returned Deployment. + route_prefix (Default[Union[str, None]]): Requests to paths under this + HTTP path prefix will be routed to this deployment. Defaults to + '/{name}'. When set to 'None', no HTTP endpoint will be created. Routing is done based on longest-prefix match, so if you have deployment A with a prefix of '/a' and deployment B with a prefix of '/a/b', requests to '/a', '/a/', and '/a/c' go to A and requests to '/a/b', '/a/b/', and '/a/b/c' go to B. Routes must not end with a '/' unless they're the root (just '/'), which acts as a catch-all. - ray_actor_options: Options to be passed to the Ray actor - constructor such as resource requirements. - user_config (Optional[Any]): Config to pass to the + ray_actor_options (Default[Dict]): Options to be passed to each + replica Ray actor's constructor, such as resource requirements. + user_config (Default[Any]): Config to pass to the reconfigure method of the deployment. This can be updated dynamically without changing the version of the deployment and restarting its replicas. The user_config must be json-serializable to keep track of updates, so it must only contain json-serializable types, or json-serializable types nested in lists and dictionaries. - max_concurrent_queries (Optional[int]): The maximum number of queries + max_concurrent_queries (Default[int]): The maximum number of queries that will be sent to a replica of this deployment without receiving a response. Defaults to 100. From 0523be88a1720190c4892f4b44013d8636f72c0c Mon Sep 17 00:00:00 2001 From: Shreyas Krishnaswamy Date: Tue, 6 Sep 2022 17:29:10 -0700 Subject: [PATCH 03/37] Check for DEFAULT.VALUE in deployment decorator body Signed-off-by: Shreyas Krishnaswamy --- python/ray/serve/api.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/python/ray/serve/api.py b/python/ray/serve/api.py index 5669fab95a76..d42db9ee34fc 100644 --- a/python/ray/serve/api.py +++ b/python/ray/serve/api.py @@ -349,13 +349,13 @@ class constructor when starting up deployment replicas. These can if num_replicas == 0: raise ValueError("num_replicas is expected to larger than 0") - if num_replicas is not None and autoscaling_config is not None: + if num_replicas is not DEFAULT.VALUE and autoscaling_config is not DEFAULT.VALUE: raise ValueError( "Manually setting num_replicas is not allowed when " "autoscaling_config is provided." ) - if version is not None: + if version is not DEFAULT.VALUE: logger.warning( "DeprecationWarning: `version` in `@serve.deployment` has been deprecated. " "Explicitly specifying version will raise an error in the future!" From 0e245ee194be55675b5095a2a0bde8cf22fb09bf Mon Sep 17 00:00:00 2001 From: Shreyas Krishnaswamy Date: Tue, 6 Sep 2022 17:34:04 -0700 Subject: [PATCH 04/37] Update from_default to process DEFAULT.VALUE instead of None Signed-off-by: Shreyas Krishnaswamy --- python/ray/serve/config.py | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/python/ray/serve/config.py b/python/ray/serve/config.py index 88f6c813e176..4c9397e2c6e8 100644 --- a/python/ray/serve/config.py +++ b/python/ray/serve/config.py @@ -23,6 +23,7 @@ DEFAULT_HTTP_HOST, DEFAULT_HTTP_PORT, ) +from ray.serve._private.utils import DEFAULT from ray.serve.generated.serve_pb2 import ( DeploymentConfig as DeploymentConfigProto, DeploymentLanguage, @@ -233,16 +234,15 @@ def from_proto_bytes(cls, proto_bytes: bytes): return cls.from_proto(proto) @classmethod - def from_default(cls, ignore_none: bool = False, **kwargs): + def from_default(cls, **kwargs): """Creates a default DeploymentConfig and overrides it with kwargs. - Only accepts the same keywords as the class. Passing in any other - keyword raises a ValueError. + Ignores any kwargs set to DEFAULT.VALUE. Args: - 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 + DEFAULT.VALUE are ignored, and their values stay default. + Invalid keywords still raise a TypeError. Raises: TypeError: when a keyword that's not an argument to the class is @@ -262,8 +262,7 @@ def from_default(cls, ignore_none: bool = False, **kwargs): f"{list(valid_config_options)}." ) - if ignore_none: - kwargs = {key: val for key, val in kwargs.items() if val is not None} + kwargs = {key: val for key, val in kwargs.items() if val != DEFAULT.VALUE} for key, val in kwargs.items(): config.__setattr__(key, val) From 4e62084a38fef561a6eef8ec618b98fc7d049c2b Mon Sep 17 00:00:00 2001 From: Shreyas Krishnaswamy Date: Tue, 6 Sep 2022 17:42:58 -0700 Subject: [PATCH 05/37] Update from_default() tests Signed-off-by: Shreyas Krishnaswamy --- python/ray/serve/api.py | 1 - python/ray/serve/deployment.py | 1 - python/ray/serve/tests/test_config.py | 38 +++++++-------------------- 3 files changed, 10 insertions(+), 30 deletions(-) diff --git a/python/ray/serve/api.py b/python/ray/serve/api.py index d42db9ee34fc..c7d12883390c 100644 --- a/python/ray/serve/api.py +++ b/python/ray/serve/api.py @@ -362,7 +362,6 @@ class constructor when starting up deployment replicas. These can ) config = DeploymentConfig.from_default( - ignore_none=True, num_replicas=num_replicas, user_config=user_config, max_concurrent_queries=max_concurrent_queries, diff --git a/python/ray/serve/deployment.py b/python/ray/serve/deployment.py index 6d4e216f0f15..632aa827364c 100644 --- a/python/ray/serve/deployment.py +++ b/python/ray/serve/deployment.py @@ -517,7 +517,6 @@ def schema_to_deployment(s: DeploymentSchema) -> Deployment: ray_actor_options = s.ray_actor_options.dict(exclude_unset=True) config = DeploymentConfig.from_default( - ignore_none=True, num_replicas=s.num_replicas, user_config=s.user_config, max_concurrent_queries=s.max_concurrent_queries, diff --git a/python/ray/serve/tests/test_config.py b/python/ray/serve/tests/test_config.py index a7a1f7e58599..34c2d2206e85 100644 --- a/python/ray/serve/tests/test_config.py +++ b/python/ray/serve/tests/test_config.py @@ -10,6 +10,7 @@ ReplicaConfig, ) from ray.serve.config import AutoscalingConfig +from ray.serve._private.utils import DEFAULT def test_autoscaling_config_validation(): @@ -66,51 +67,32 @@ def test_deployment_config_update(self): with pytest.raises(ValidationError): b.num_replicas = -1 - @pytest.mark.parametrize("ignore_none", [True, False]) - def test_from_default(self, ignore_none): + def test_from_default(self): """Check from_default() method behavior.""" # Valid parameters - dc = DeploymentConfig.from_default( - ignore_none=ignore_none, num_replicas=5, is_cross_language=True - ) + dc = DeploymentConfig.from_default(num_replicas=5, is_cross_language=True) assert dc.num_replicas == 5 assert dc.is_cross_language is True # Invalid parameters should raise TypeError with pytest.raises(TypeError): - DeploymentConfig.from_default( - ignore_none=ignore_none, num_replicas=5, is_xlang=True - ) + DeploymentConfig.from_default(num_replicas=5, is_xlang=True) # Validation should still be performed with pytest.raises(ValidationError): - DeploymentConfig.from_default( - ignore_none=ignore_none, num_replicas="hello world" - ) + DeploymentConfig.from_default(num_replicas="hello world") - def test_from_default_ignore_none(self): - """Check from_default()'s ignore_none parameter""" + def test_from_default_ignore_default(self): + """Check that from_default() ignores DEFAULT.VALUE kwargs.""" default = DeploymentConfig() - # Valid parameter with None passed in should be ignored - dc = DeploymentConfig.from_default(ignore_none=True, num_replicas=None) - - # Invalid parameter should raise TypeError no matter what - with pytest.raises(TypeError): - DeploymentConfig.from_default(ignore_none=True, fake=5) - with pytest.raises(TypeError): - DeploymentConfig.from_default(ignore_none=False, fake=5) + # Valid parameter with DEFAULT.VALUE passed in should be ignored + dc = DeploymentConfig.from_default(num_replicas=DEFAULT.VALUE) # Validators should run no matter what - dc = DeploymentConfig.from_default( - ignore_none=True, max_concurrent_queries=None - ) - assert dc.max_concurrent_queries == default.max_concurrent_queries - dc = DeploymentConfig.from_default( - ignore_none=False, max_concurrent_queries=None - ) + dc = DeploymentConfig.from_default(max_concurrent_queries=None) assert dc.max_concurrent_queries is not None assert dc.max_concurrent_queries == default.max_concurrent_queries From fc57c3ea322fa0294666b9bce5d09ae7b5e90919 Mon Sep 17 00:00:00 2001 From: Shreyas Krishnaswamy Date: Tue, 6 Sep 2022 18:24:32 -0700 Subject: [PATCH 06/37] Use DEFAULT.VALUE in schema defaults Signed-off-by: Shreyas Krishnaswamy --- python/ray/serve/schema.py | 37 +++++++++++++++++++------------------ 1 file changed, 19 insertions(+), 18 deletions(-) diff --git a/python/ray/serve/schema.py b/python/ray/serve/schema.py index 46922378cd61..4a8fa3f93dbb 100644 --- a/python/ray/serve/schema.py +++ b/python/ray/serve/schema.py @@ -14,14 +14,14 @@ @PublicAPI(stability="beta") class RayActorOptionsSchema(BaseModel, extra=Extra.forbid): runtime_env: dict = Field( - default={}, + default=DEFAULT.VALUE, description=( "This deployment's runtime_env. working_dir and " "py_modules may contain only remote URIs." ), ) num_cpus: float = Field( - default=None, + default=DEFAULT.VALUE, description=( "The number of CPUs required by the deployment's " "application per replica. This is the same as a ray " @@ -30,7 +30,7 @@ class RayActorOptionsSchema(BaseModel, extra=Extra.forbid): ge=0, ) num_gpus: float = Field( - default=None, + default=DEFAULT.VALUE, description=( "The number of GPUs required by the deployment's " "application per replica. This is the same as a ray " @@ -39,14 +39,14 @@ class RayActorOptionsSchema(BaseModel, extra=Extra.forbid): ge=0, ) memory: float = Field( - default=None, + default=DEFAULT.VALUE, description=( "Restrict the heap memory usage of each replica. Uses a default if null." ), ge=0, ) object_store_memory: float = Field( - default=None, + default=DEFAULT.VALUE, description=( "Restrict the object store memory used per replica when " "creating objects. Uses a default if null." @@ -54,10 +54,11 @@ class RayActorOptionsSchema(BaseModel, extra=Extra.forbid): ge=0, ) resources: Dict = Field( - default={}, description=("The custom resources required by each replica.") + default=DEFAULT.VALUE, + description=("The custom resources required by each replica."), ) accelerator_type: str = Field( - default=None, + default=DEFAULT.VALUE, description=( "Forces replicas to run on nodes with the specified accelerator type." ), @@ -67,7 +68,7 @@ class RayActorOptionsSchema(BaseModel, extra=Extra.forbid): def runtime_env_contains_remote_uris(cls, v): # Ensure that all uris in py_modules and working_dir are remote - if v is None: + if v is DEFAULT.VALUE: return uris = v.get("py_modules", []) @@ -89,14 +90,14 @@ class DeploymentSchema( ..., description=("Globally-unique name identifying this deployment.") ) num_replicas: int = Field( - default=None, + default=DEFAULT.VALUE, description=( "The number of processes that handle requests to this " "deployment. Uses a default if null." ), gt=0, ) - route_prefix: Union[str, None, DEFAULT] = Field( + route_prefix: Union[str, None] = Field( default=DEFAULT.VALUE, description=( "Requests to paths under this HTTP path " @@ -112,7 +113,7 @@ class DeploymentSchema( ), ) max_concurrent_queries: int = Field( - default=None, + default=DEFAULT.VALUE, description=( "The max number of pending queries in a single replica. " "Uses a default if null." @@ -120,7 +121,7 @@ class DeploymentSchema( gt=0, ) user_config: Dict = Field( - default=None, + default=DEFAULT.VALUE, description=( "Config to pass into this deployment's " "reconfigure method. This can be updated dynamically " @@ -128,7 +129,7 @@ class DeploymentSchema( ), ) autoscaling_config: Dict = Field( - default=None, + default=DEFAULT.VALUE, description=( "Config specifying autoscaling " "parameters for the deployment's number of replicas. " @@ -138,7 +139,7 @@ class DeploymentSchema( ), ) graceful_shutdown_wait_loop_s: float = Field( - default=None, + default=DEFAULT.VALUE, description=( "Duration that deployment replicas will wait until there " "is no more work to be done before shutting down. Uses a " @@ -147,7 +148,7 @@ class DeploymentSchema( ge=0, ) graceful_shutdown_timeout_s: float = Field( - default=None, + default=DEFAULT.VALUE, description=( "Serve controller waits for this duration before " "forcefully killing the replica for shutdown. Uses a " @@ -156,7 +157,7 @@ class DeploymentSchema( ge=0, ) health_check_period_s: float = Field( - default=None, + default=DEFAULT.VALUE, description=( "Frequency at which the controller will health check " "replicas. Uses a default if null." @@ -164,7 +165,7 @@ class DeploymentSchema( gt=0, ) health_check_timeout_s: float = Field( - default=None, + default=DEFAULT.VALUE, description=( "Timeout that the controller will wait for a response " "from the replica's health check before marking it " @@ -173,7 +174,7 @@ class DeploymentSchema( gt=0, ) ray_actor_options: RayActorOptionsSchema = Field( - default=None, description="Options set for each replica actor." + default=DEFAULT.VALUE, description="Options set for each replica actor." ) @root_validator From d09e71beea7028c5c84ffe2810d2513af186fabc Mon Sep 17 00:00:00 2001 From: Shreyas Krishnaswamy Date: Tue, 6 Sep 2022 18:27:47 -0700 Subject: [PATCH 07/37] Use DEFAULT.VALUE in schema validators Signed-off-by: Shreyas Krishnaswamy --- python/ray/serve/schema.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/python/ray/serve/schema.py b/python/ray/serve/schema.py index 4a8fa3f93dbb..cbf2736d6a36 100644 --- a/python/ray/serve/schema.py +++ b/python/ray/serve/schema.py @@ -180,8 +180,8 @@ class DeploymentSchema( @root_validator def num_replicas_and_autoscaling_config_mutually_exclusive(cls, values): if ( - values.get("num_replicas", None) is not None - and values.get("autoscaling_config", None) is not None + values.get("num_replicas", DEFAULT.VALUE) is not DEFAULT.VALUE + and values.get("autoscaling_config", DEFAULT.VALUE) is not DEFAULT.VALUE ): raise ValueError( "Manually setting num_replicas is not allowed " From 80a8a85a66876d239fb7d7cce942469526b1bc1e Mon Sep 17 00:00:00 2001 From: Shreyas Krishnaswamy Date: Sat, 10 Sep 2022 17:50:26 -0700 Subject: [PATCH 08/37] Add user_configured_options to DeploymentConfig Signed-off-by: Shreyas Krishnaswamy --- python/ray/serve/config.py | 10 +++++++++- src/ray/protobuf/serve.proto | 2 ++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/python/ray/serve/config.py b/python/ray/serve/config.py index 14c8d66acb58..7528a8a31a91 100644 --- a/python/ray/serve/config.py +++ b/python/ray/serve/config.py @@ -1,7 +1,7 @@ import inspect import json from enum import Enum -from typing import Any, Callable, Dict, List, Optional, Tuple, Union +from typing import Any, Callable, Dict, List, Optional, Tuple, Union, Set import pydantic from google.protobuf.json_format import MessageToDict @@ -123,6 +123,8 @@ class DeploymentConfig(BaseModel): health_check_timeout_s (Optional[float]): Timeout that the controller will wait for a response from the replica's health check before marking it unhealthy. + user_configured_options (Set[str]): + The options manually configured by the user. """ num_replicas: NonNegativeInt = 1 @@ -151,6 +153,9 @@ class DeploymentConfig(BaseModel): version: Optional[str] = None + # Contains the deployment options manually set by the user + user_configured_options: Set[str] = set() + class Config: validate_assignment = True extra = "forbid" @@ -190,6 +195,7 @@ def to_proto(self): data["autoscaling_config"] = AutoscalingConfigProto( **data["autoscaling_config"] ) + data["user_configured_options"] = list(data["user_configured_options"]) return DeploymentConfigProto(**data) def to_proto_bytes(self): @@ -226,6 +232,8 @@ def from_proto(cls, proto: DeploymentConfigProto): if "version" in data: if data["version"] == "": data["version"] = None + if "user_configured_options" in data: + data["user_configured_options"] = set(data["user_configured_options"]) return cls(**data) @classmethod diff --git a/src/ray/protobuf/serve.proto b/src/ray/protobuf/serve.proto index 34fa910aad9c..fde3a255e317 100644 --- a/src/ray/protobuf/serve.proto +++ b/src/ray/protobuf/serve.proto @@ -88,6 +88,8 @@ message DeploymentConfig { AutoscalingConfig autoscaling_config = 10; string version = 11; + + repeated string user_configured_options = 12; } // Deployment language. From 411bd266df06c508c16a540905bd3c224516af38 Mon Sep 17 00:00:00 2001 From: Shreyas Krishnaswamy Date: Sat, 10 Sep 2022 18:04:28 -0700 Subject: [PATCH 09/37] Handle user_configured_options in decorator Signed-off-by: Shreyas Krishnaswamy --- python/ray/serve/api.py | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/python/ray/serve/api.py b/python/ray/serve/api.py index c7d12883390c..3f9df9856bb4 100644 --- a/python/ray/serve/api.py +++ b/python/ray/serve/api.py @@ -344,6 +344,13 @@ class constructor when starting up deployment replicas. These can Deployment """ + # Create list of all user-configured options from keyword args + user_configured_options = [ + option + for option, value in locals() + if value != DEFAULT.VALUE and option != "_func_or_class" + ] + # Num of replicas should not be 0. # TODO(Sihan) seperate num_replicas attribute from internal and api if num_replicas == 0: @@ -371,17 +378,20 @@ class constructor when starting up deployment replicas. These can health_check_period_s=health_check_period_s, health_check_timeout_s=health_check_timeout_s, ) + config.user_configured_options = set(user_configured_options) def decorator(_func_or_class): return Deployment( _func_or_class, - name if name is not None else _func_or_class.__name__, + name if name != DEFAULT.VALUE else _func_or_class.__name__, config, - version=version, - init_args=init_args, - init_kwargs=init_kwargs, + version=(version if version != DEFAULT.VALUE else None), + init_args=(init_args if init_args != DEFAULT.VALUE else None), + init_kwargs=(init_kwargs if init_kwargs != DEFAULT.VALUE else None), route_prefix=route_prefix, - ray_actor_options=ray_actor_options, + ray_actor_options=( + ray_actor_options if ray_actor_options != DEFAULT.VALUE else None + ), _internal=True, ) From fa5f4a6fc54e0b9a742205dd5e098f2037f051d7 Mon Sep 17 00:00:00 2001 From: Shreyas Krishnaswamy Date: Sat, 10 Sep 2022 18:31:07 -0700 Subject: [PATCH 10/37] Handle defaults in options() Signed-off-by: Shreyas Krishnaswamy --- python/ray/serve/api.py | 10 +-- python/ray/serve/deployment.py | 119 ++++++++++++++++++++------------- 2 files changed, 77 insertions(+), 52 deletions(-) diff --git a/python/ray/serve/api.py b/python/ray/serve/api.py index 3f9df9856bb4..d2af76fb2dee 100644 --- a/python/ray/serve/api.py +++ b/python/ray/serve/api.py @@ -383,14 +383,14 @@ class constructor when starting up deployment replicas. These can def decorator(_func_or_class): return Deployment( _func_or_class, - name if name != DEFAULT.VALUE else _func_or_class.__name__, + name if name is not DEFAULT.VALUE else _func_or_class.__name__, config, - version=(version if version != DEFAULT.VALUE else None), - init_args=(init_args if init_args != DEFAULT.VALUE else None), - init_kwargs=(init_kwargs if init_kwargs != DEFAULT.VALUE else None), + version=(version if version is not DEFAULT.VALUE else None), + init_args=(init_args if init_args is not DEFAULT.VALUE else None), + init_kwargs=(init_kwargs if init_kwargs is not DEFAULT.VALUE else None), route_prefix=route_prefix, ray_actor_options=( - ray_actor_options if ray_actor_options != DEFAULT.VALUE else None + ray_actor_options if ray_actor_options is not DEFAULT.VALUE else None ), _internal=True, ) diff --git a/python/ray/serve/deployment.py b/python/ray/serve/deployment.py index 632aa827364c..2cba574d77df 100644 --- a/python/ray/serve/deployment.py +++ b/python/ray/serve/deployment.py @@ -1,4 +1,4 @@ -from copy import copy +from copy import copy, deepcopy import inspect import logging from typing import ( @@ -20,7 +20,7 @@ ) from ray.serve._private.constants import SERVE_LOGGER_NAME, MIGRATION_MESSAGE from ray.serve.handle import RayServeHandle, RayServeSyncHandle -from ray.serve._private.utils import DEFAULT, guarded_deprecation_warning +from ray.serve._private.utils import DEFAULT, Default, guarded_deprecation_warning from ray.util.annotations import Deprecated, PublicAPI from ray.serve.schema import ( RayActorOptionsSchema, @@ -295,30 +295,55 @@ def _get_handle( def options( self, func_or_class: Optional[Callable] = None, - name: Optional[str] = None, - version: Optional[str] = None, - init_args: Optional[Tuple[Any]] = None, - init_kwargs: Optional[Dict[Any, Any]] = None, - route_prefix: Union[str, None, DEFAULT] = DEFAULT.VALUE, - num_replicas: Optional[int] = None, - ray_actor_options: Optional[Dict] = None, - user_config: Optional[Any] = None, - max_concurrent_queries: Optional[int] = None, - autoscaling_config: Optional[Union[Dict, AutoscalingConfig]] = None, - graceful_shutdown_wait_loop_s: Optional[float] = None, - graceful_shutdown_timeout_s: Optional[float] = None, - health_check_period_s: Optional[float] = None, - health_check_timeout_s: Optional[float] = None, + name: Default[str] = DEFAULT.VALUE, + version: Default[str] = DEFAULT.VALUE, + init_args: Default[Tuple[Any]] = DEFAULT.VALUE, + init_kwargs: Default[Dict[Any, Any]] = DEFAULT.VALUE, + route_prefix: Default[Union[str, None]] = DEFAULT.VALUE, + num_replicas: Default[int] = DEFAULT.VALUE, + ray_actor_options: Default[Dict] = DEFAULT.VALUE, + user_config: Default[Any] = DEFAULT.VALUE, + max_concurrent_queries: Default[int] = DEFAULT.VALUE, + autoscaling_config: Default[Union[Dict, AutoscalingConfig]] = DEFAULT.VALUE, + graceful_shutdown_wait_loop_s: Default[float] = DEFAULT.VALUE, + graceful_shutdown_timeout_s: Default[float] = DEFAULT.VALUE, + health_check_period_s: Default[float] = DEFAULT.VALUE, + health_check_timeout_s: Default[float] = DEFAULT.VALUE, _internal: bool = False, ) -> "Deployment": """Return a copy of this deployment with updated options. Only those options passed in will be updated, all others will remain unchanged from the existing deployment. + + Args: + Refer to @serve.deployment decorator docstring for all non-private + arguments. + + _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. """ - new_config = self._config.copy() - if num_replicas is not None and autoscaling_config is not None: + # Create list of all user-configured options from keyword args + user_configured_options = [ + option + for option, value in locals() + if value != DEFAULT.VALUE + and option not in {"self", "_func_or_class", "_internal"} + ] + + new_config = deepcopy(self._config) + if not _internal: + new_config.user_configured_options.update(user_configured_options) + + if ( + num_replicas is not DEFAULT.VALUE + and autoscaling_config is not DEFAULT.VALUE + ): raise ValueError( "Manually setting num_replicas is not allowed when " "autoscaling_config is provided." @@ -327,55 +352,55 @@ def options( if num_replicas == 0: raise ValueError("num_replicas is expected to larger than 0") - if not _internal and version is not None: + if not _internal and version is DEFAULT.VALUE: logger.warning( "DeprecationWarning: `version` in `Deployment.options()` has been " "deprecated. Explicitly specifying version will raise an error in the " "future!" ) - if num_replicas is not None: + if num_replicas is not DEFAULT.VALUE: new_config.num_replicas = num_replicas - if user_config is not None: + if user_config is not DEFAULT.VALUE: new_config.user_config = user_config - if max_concurrent_queries is not None: + if max_concurrent_queries is not DEFAULT.VALUE: new_config.max_concurrent_queries = max_concurrent_queries - if func_or_class is None: + if func_or_class is not None: func_or_class = self._func_or_class - if name is None: + if name is DEFAULT.VALUE: name = self._name - if version is None: + if version is DEFAULT.VALUE: version = self._version - if init_args is None: + if init_args is DEFAULT.VALUE: init_args = self._init_args - if init_kwargs is None: + if init_kwargs is DEFAULT.VALUE: init_kwargs = self._init_kwargs if route_prefix is DEFAULT.VALUE: # Default is to keep the previous value route_prefix = self._route_prefix - if ray_actor_options is None: + if ray_actor_options is DEFAULT.VALUE: ray_actor_options = self._ray_actor_options - if autoscaling_config is not None: + if autoscaling_config is not DEFAULT.VALUE: new_config.autoscaling_config = autoscaling_config - if graceful_shutdown_wait_loop_s is not None: + if graceful_shutdown_wait_loop_s is not DEFAULT.VALUE: new_config.graceful_shutdown_wait_loop_s = graceful_shutdown_wait_loop_s - if graceful_shutdown_timeout_s is not None: + if graceful_shutdown_timeout_s is not DEFAULT.VALUE: new_config.graceful_shutdown_timeout_s = graceful_shutdown_timeout_s - if health_check_period_s is not None: + if health_check_period_s is not DEFAULT.VALUE: new_config.health_check_period_s = health_check_period_s - if health_check_timeout_s is not None: + if health_check_timeout_s is not DEFAULT.VALUE: new_config.health_check_timeout_s = health_check_timeout_s return Deployment( @@ -394,20 +419,20 @@ def options( def set_options( self, func_or_class: Optional[Callable] = None, - name: Optional[str] = None, - version: Optional[str] = None, - init_args: Optional[Tuple[Any]] = None, - init_kwargs: Optional[Dict[Any, Any]] = None, - route_prefix: Union[str, None, DEFAULT] = DEFAULT.VALUE, - num_replicas: Optional[int] = None, - ray_actor_options: Optional[Dict] = None, - user_config: Optional[Any] = None, - max_concurrent_queries: Optional[int] = None, - autoscaling_config: Optional[Union[Dict, AutoscalingConfig]] = None, - graceful_shutdown_wait_loop_s: Optional[float] = None, - graceful_shutdown_timeout_s: Optional[float] = None, - health_check_period_s: Optional[float] = None, - health_check_timeout_s: Optional[float] = None, + name: Default[str] = DEFAULT.VALUE, + version: Default[str] = DEFAULT.VALUE, + init_args: Default[Tuple[Any]] = DEFAULT.VALUE, + init_kwargs: Default[Dict[Any, Any]] = DEFAULT.VALUE, + route_prefix: Default[Union[str, None]] = DEFAULT.VALUE, + num_replicas: Default[int] = DEFAULT.VALUE, + ray_actor_options: Default[Dict] = DEFAULT.VALUE, + user_config: Default[Any] = DEFAULT.VALUE, + max_concurrent_queries: Default[int] = DEFAULT.VALUE, + autoscaling_config: Default[Union[Dict, AutoscalingConfig]] = DEFAULT.VALUE, + graceful_shutdown_wait_loop_s: Default[float] = DEFAULT.VALUE, + graceful_shutdown_timeout_s: Default[float] = DEFAULT.VALUE, + health_check_period_s: Default[float] = DEFAULT.VALUE, + health_check_timeout_s: Default[float] = DEFAULT.VALUE, _internal: bool = False, ) -> None: """Overwrite this deployment's options. Mutates the deployment. From 2658d7ecc2d37f27f8f7bcc5a15447a8d7713740 Mon Sep 17 00:00:00 2001 From: Shreyas Krishnaswamy Date: Sat, 10 Sep 2022 18:36:08 -0700 Subject: [PATCH 11/37] Remove defaults in RayActorOptionsSchema Signed-off-by: Shreyas Krishnaswamy --- python/ray/serve/schema.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/python/ray/serve/schema.py b/python/ray/serve/schema.py index cbf2736d6a36..dd8fb4047053 100644 --- a/python/ray/serve/schema.py +++ b/python/ray/serve/schema.py @@ -14,14 +14,14 @@ @PublicAPI(stability="beta") class RayActorOptionsSchema(BaseModel, extra=Extra.forbid): runtime_env: dict = Field( - default=DEFAULT.VALUE, + default={}, description=( "This deployment's runtime_env. working_dir and " "py_modules may contain only remote URIs." ), ) num_cpus: float = Field( - default=DEFAULT.VALUE, + default=None, description=( "The number of CPUs required by the deployment's " "application per replica. This is the same as a ray " @@ -30,7 +30,7 @@ class RayActorOptionsSchema(BaseModel, extra=Extra.forbid): ge=0, ) num_gpus: float = Field( - default=DEFAULT.VALUE, + default=None, description=( "The number of GPUs required by the deployment's " "application per replica. This is the same as a ray " @@ -39,14 +39,14 @@ class RayActorOptionsSchema(BaseModel, extra=Extra.forbid): ge=0, ) memory: float = Field( - default=DEFAULT.VALUE, + default=None, description=( "Restrict the heap memory usage of each replica. Uses a default if null." ), ge=0, ) object_store_memory: float = Field( - default=DEFAULT.VALUE, + default=None, description=( "Restrict the object store memory used per replica when " "creating objects. Uses a default if null." @@ -54,11 +54,11 @@ class RayActorOptionsSchema(BaseModel, extra=Extra.forbid): ge=0, ) resources: Dict = Field( - default=DEFAULT.VALUE, + default={}, description=("The custom resources required by each replica."), ) accelerator_type: str = Field( - default=DEFAULT.VALUE, + default=None, description=( "Forces replicas to run on nodes with the specified accelerator type." ), @@ -68,7 +68,7 @@ class RayActorOptionsSchema(BaseModel, extra=Extra.forbid): def runtime_env_contains_remote_uris(cls, v): # Ensure that all uris in py_modules and working_dir are remote - if v is DEFAULT.VALUE: + if v is None: return uris = v.get("py_modules", []) From dffaa2c8d5eb16bc214040404148144c544d133e Mon Sep 17 00:00:00 2001 From: Shreyas Krishnaswamy Date: Sat, 10 Sep 2022 18:52:04 -0700 Subject: [PATCH 12/37] Handle defaults in schema helpers Signed-off-by: Shreyas Krishnaswamy --- python/ray/serve/deployment.py | 54 ++++++++++++++++++---------------- python/ray/serve/schema.py | 10 ++++++- 2 files changed, 38 insertions(+), 26 deletions(-) diff --git a/python/ray/serve/deployment.py b/python/ray/serve/deployment.py index 2cba574d77df..9b97d08e770d 100644 --- a/python/ray/serve/deployment.py +++ b/python/ray/serve/deployment.py @@ -495,36 +495,39 @@ def __repr__(self): def deployment_to_schema(d: Deployment) -> DeploymentSchema: - """Converts a live deployment object to a corresponding structured schema. + """Converts a live deployment object to a corresponding structured schema.""" - If the deployment has a class or function, it will be attemptetd to be - converted to a valid corresponding import path. - - init_args and init_kwargs must also be JSON-serializable or this call will - fail. - """ if d.ray_actor_options is not None: ray_actor_options_schema = RayActorOptionsSchema.parse_obj(d.ray_actor_options) else: ray_actor_options_schema = None - return DeploymentSchema( - name=d.name, - # TODO(Sihan) DeploymentConfig num_replicas and auto_config can be set together - # because internally we use these two field for autoscale and deploy. - # We can improve the code after we separate the user faced deployment config and - # internal deployment config. - num_replicas=None if d._config.autoscaling_config else d.num_replicas, - route_prefix=d.route_prefix, - max_concurrent_queries=d.max_concurrent_queries, - user_config=d.user_config, - autoscaling_config=d._config.autoscaling_config, - graceful_shutdown_wait_loop_s=d._config.graceful_shutdown_wait_loop_s, - graceful_shutdown_timeout_s=d._config.graceful_shutdown_timeout_s, - health_check_period_s=d._config.health_check_period_s, - health_check_timeout_s=d._config.health_check_timeout_s, - ray_actor_options=ray_actor_options_schema, - ) + deployment_options = { + "name": d.name, + "num_replicas": None if d._config.autoscaling_config else d.num_replicas, + "route_prefix": d.route_prefix, + "max_concurrent_queries": d.max_concurrent_queries, + "user_config": d.user_config, + "autoscaling_config": d._config.autoscaling_config, + "graceful_shutdown_wait_loop_s": d._config.graceful_shutdown_wait_loop_s, + "graceful_shutdown_timeout_s": d._config.graceful_shutdown_timeout_s, + "health_check_period_s": d._config.health_check_period_s, + "health_check_timeout_s": d._config.health_check_timeout_s, + "ray_actor_options": ray_actor_options_schema, + } + + # Pass DEFAULT.VALUE directly to non-user-configured options. If the schema + # is converted back to a deployment, this lets Serve continue tracking + # which options were set by the user. + for option in deployment_options: + if option not in d._config.user_configured_options: + deployment_options[option] = DEFAULT.VALUE + + # TODO(Sihan) DeploymentConfig num_replicas and auto_config can be set together + # because internally we use these two field for autoscale and deploy. + # We can improve the code after we separate the user faced deployment config and + # internal deployment config. + return DeploymentSchema(**deployment_options) def schema_to_deployment(s: DeploymentSchema) -> Deployment: @@ -536,7 +539,7 @@ def schema_to_deployment(s: DeploymentSchema) -> Deployment: before the deployment can be deployed. """ - if s.ray_actor_options is None: + if s.ray_actor_options is DEFAULT.VALUE: ray_actor_options = None else: ray_actor_options = s.ray_actor_options.dict(exclude_unset=True) @@ -551,6 +554,7 @@ def schema_to_deployment(s: DeploymentSchema) -> Deployment: health_check_period_s=s.health_check_period_s, health_check_timeout_s=s.health_check_timeout_s, ) + config.user_configured_options = s.get_user_configured_options() return Deployment( func_or_class="", diff --git a/python/ray/serve/schema.py b/python/ray/serve/schema.py index dd8fb4047053..2be862bbc30d 100644 --- a/python/ray/serve/schema.py +++ b/python/ray/serve/schema.py @@ -1,5 +1,5 @@ from pydantic import BaseModel, Field, Extra, root_validator, validator -from typing import Union, List, Dict +from typing import Union, List, Dict, Set from ray._private.runtime_env.packaging import parse_uri from ray.serve._private.common import ( DeploymentStatusInfo, @@ -223,6 +223,14 @@ def route_prefix_format(cls, v): return v + def get_user_configured_options(self) -> Set[str]: + """Get set of all user-configured options. + + Any field not set to DEFAULT.VALUE is considered user-configured options. + """ + + return {field for field, value in self.dict() if value is not DEFAULT.VALUE} + @PublicAPI(stability="beta") class ServeApplicationSchema(BaseModel, extra=Extra.forbid): From 62a12587ce374eb7d6c85a57e52a2ded1ccb2e6e Mon Sep 17 00:00:00 2001 From: Shreyas Krishnaswamy Date: Sat, 10 Sep 2022 18:53:20 -0700 Subject: [PATCH 13/37] Iterate through locals items() instead of locals directly Signed-off-by: Shreyas Krishnaswamy --- python/ray/serve/api.py | 2 +- python/ray/serve/deployment.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/python/ray/serve/api.py b/python/ray/serve/api.py index d2af76fb2dee..fc8c6307d44c 100644 --- a/python/ray/serve/api.py +++ b/python/ray/serve/api.py @@ -347,7 +347,7 @@ class constructor when starting up deployment replicas. These can # Create list of all user-configured options from keyword args user_configured_options = [ option - for option, value in locals() + for option, value in locals().items() if value != DEFAULT.VALUE and option != "_func_or_class" ] diff --git a/python/ray/serve/deployment.py b/python/ray/serve/deployment.py index 9b97d08e770d..83fb5f072950 100644 --- a/python/ray/serve/deployment.py +++ b/python/ray/serve/deployment.py @@ -331,7 +331,7 @@ def options( # Create list of all user-configured options from keyword args user_configured_options = [ option - for option, value in locals() + for option, value in locals().items() if value != DEFAULT.VALUE and option not in {"self", "_func_or_class", "_internal"} ] From e4a0d8b6dd83d424f69989d59195e999e8678916 Mon Sep 17 00:00:00 2001 From: Shreyas Krishnaswamy Date: Sat, 10 Sep 2022 18:57:03 -0700 Subject: [PATCH 14/37] Flip order in list comprehensions Signed-off-by: Shreyas Krishnaswamy --- python/ray/serve/api.py | 2 +- python/ray/serve/deployment.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/python/ray/serve/api.py b/python/ray/serve/api.py index fc8c6307d44c..479454c117ea 100644 --- a/python/ray/serve/api.py +++ b/python/ray/serve/api.py @@ -348,7 +348,7 @@ class constructor when starting up deployment replicas. These can user_configured_options = [ option for option, value in locals().items() - if value != DEFAULT.VALUE and option != "_func_or_class" + if option != "_func_or_class" and value is not DEFAULT.VALUE ] # Num of replicas should not be 0. diff --git a/python/ray/serve/deployment.py b/python/ray/serve/deployment.py index 83fb5f072950..30ae3a97662c 100644 --- a/python/ray/serve/deployment.py +++ b/python/ray/serve/deployment.py @@ -332,8 +332,8 @@ def options( user_configured_options = [ option for option, value in locals().items() - if value != DEFAULT.VALUE - and option not in {"self", "_func_or_class", "_internal"} + if option not in {"self", "_func_or_class", "_internal"} + and value is not DEFAULT.VALUE ] new_config = deepcopy(self._config) From 5ffa7a746fb6776e7cb27294df050ca7f9e7eea8 Mon Sep 17 00:00:00 2001 From: Shreyas Krishnaswamy Date: Sat, 10 Sep 2022 19:06:09 -0700 Subject: [PATCH 15/37] Add missing not Signed-off-by: Shreyas Krishnaswamy --- python/ray/serve/deployment.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/python/ray/serve/deployment.py b/python/ray/serve/deployment.py index 30ae3a97662c..132db9c7b53c 100644 --- a/python/ray/serve/deployment.py +++ b/python/ray/serve/deployment.py @@ -366,7 +366,7 @@ def options( if max_concurrent_queries is not DEFAULT.VALUE: new_config.max_concurrent_queries = max_concurrent_queries - if func_or_class is not None: + if func_or_class is None: func_or_class = self._func_or_class if name is DEFAULT.VALUE: From a3f0e767eef20be628f632b9301c0aa14661229f Mon Sep 17 00:00:00 2001 From: Shreyas Krishnaswamy Date: Sat, 10 Sep 2022 22:09:37 -0700 Subject: [PATCH 16/37] Fix dictionary iterations Signed-off-by: Shreyas Krishnaswamy --- python/ray/serve/deployment.py | 6 +++--- python/ray/serve/schema.py | 4 +++- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/python/ray/serve/deployment.py b/python/ray/serve/deployment.py index 132db9c7b53c..87c243fc88ad 100644 --- a/python/ray/serve/deployment.py +++ b/python/ray/serve/deployment.py @@ -516,12 +516,12 @@ def deployment_to_schema(d: Deployment) -> DeploymentSchema: "ray_actor_options": ray_actor_options_schema, } - # Pass DEFAULT.VALUE directly to non-user-configured options. If the schema + # Let non-user-configured options be set to defaults. If the schema # is converted back to a deployment, this lets Serve continue tracking # which options were set by the user. - for option in deployment_options: + for option in list(deployment_options.keys()): if option not in d._config.user_configured_options: - deployment_options[option] = DEFAULT.VALUE + del deployment_options[option] # TODO(Sihan) DeploymentConfig num_replicas and auto_config can be set together # because internally we use these two field for autoscale and deploy. diff --git a/python/ray/serve/schema.py b/python/ray/serve/schema.py index 2be862bbc30d..747f62490329 100644 --- a/python/ray/serve/schema.py +++ b/python/ray/serve/schema.py @@ -229,7 +229,9 @@ def get_user_configured_options(self) -> Set[str]: Any field not set to DEFAULT.VALUE is considered user-configured options. """ - return {field for field, value in self.dict() if value is not DEFAULT.VALUE} + return { + field for field, value in self.dict().items() if value is not DEFAULT.VALUE + } @PublicAPI(stability="beta") From 8f84682785d2f2d434ba44afaf0e960e3686b94d Mon Sep 17 00:00:00 2001 From: Shreyas Krishnaswamy Date: Sat, 10 Sep 2022 22:42:07 -0700 Subject: [PATCH 17/37] Keep name field when creating schema Signed-off-by: Shreyas Krishnaswamy --- python/ray/serve/deployment.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/python/ray/serve/deployment.py b/python/ray/serve/deployment.py index 87c243fc88ad..6819c5f190ed 100644 --- a/python/ray/serve/deployment.py +++ b/python/ray/serve/deployment.py @@ -518,9 +518,10 @@ def deployment_to_schema(d: Deployment) -> DeploymentSchema: # Let non-user-configured options be set to defaults. If the schema # is converted back to a deployment, this lets Serve continue tracking - # which options were set by the user. + # which options were set by the user. Name is a required field in the + # schema, so it should be passed in explicitly. for option in list(deployment_options.keys()): - if option not in d._config.user_configured_options: + if option != "name" and option not in d._config.user_configured_options: del deployment_options[option] # TODO(Sihan) DeploymentConfig num_replicas and auto_config can be set together From c1f83f927a42101adcd56d71b2fcd51ddca84962 Mon Sep 17 00:00:00 2001 From: Shreyas Krishnaswamy Date: Sun, 11 Sep 2022 00:19:06 -0700 Subject: [PATCH 18/37] Specify optional arguments Signed-off-by: Shreyas Krishnaswamy --- python/ray/serve/api.py | 21 ++++++++------ python/ray/serve/deployment.py | 30 +++++++++++--------- python/ray/serve/schema.py | 17 ++++++------ python/ray/serve/tests/test_schema.py | 40 +-------------------------- 4 files changed, 38 insertions(+), 70 deletions(-) diff --git a/python/ray/serve/api.py b/python/ray/serve/api.py index 479454c117ea..65b67838a291 100644 --- a/python/ray/serve/api.py +++ b/python/ray/serve/api.py @@ -277,14 +277,14 @@ def deployment( _func_or_class: Optional[Callable] = None, name: Default[str] = DEFAULT.VALUE, version: Default[str] = DEFAULT.VALUE, - num_replicas: Default[int] = DEFAULT.VALUE, + num_replicas: Default[Optional[int]] = DEFAULT.VALUE, init_args: Default[Tuple[Any]] = DEFAULT.VALUE, 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[Any] = DEFAULT.VALUE, + ray_actor_options: Default[Optional[Dict]] = DEFAULT.VALUE, + user_config: Default[Optional[Any]] = DEFAULT.VALUE, max_concurrent_queries: Default[int] = DEFAULT.VALUE, - autoscaling_config: Default[Union[Dict, AutoscalingConfig]] = DEFAULT.VALUE, + autoscaling_config: Default[Union[Dict, AutoscalingConfig, None]] = DEFAULT.VALUE, graceful_shutdown_wait_loop_s: Default[float] = DEFAULT.VALUE, graceful_shutdown_timeout_s: Default[float] = DEFAULT.VALUE, health_check_period_s: Default[float] = DEFAULT.VALUE, @@ -301,7 +301,7 @@ def deployment( is re-deployed with a version change, a rolling update of the replicas will be performed. If not provided, every deployment will be treated as a new version. - num_replicas (Default[int]): The number of processes to start up that + num_replicas (Default[Optional[int]]): The number of processes to start up that will handle requests to this deployment. Defaults to 1. init_args (Default[Tuple[Any]]): Positional args to be passed to the class constructor when starting up deployment replicas. These can @@ -318,9 +318,9 @@ class constructor when starting up deployment replicas. These can to '/a/b', '/a/b/', and '/a/b/c' go to B. Routes must not end with a '/' unless they're the root (just '/'), which acts as a catch-all. - ray_actor_options (Default[Dict]): Options to be passed to each + ray_actor_options (Default[Optional[Dict]]): Options to be passed to each replica Ray actor's constructor, such as resource requirements. - user_config (Default[Any]): Config to pass to the + user_config (Default[Optional[Any]]): Config to pass to the reconfigure method of the deployment. This can be updated dynamically without changing the version of the deployment and restarting its replicas. The user_config must be json-serializable @@ -356,7 +356,10 @@ class constructor when starting up deployment replicas. These can if num_replicas == 0: raise ValueError("num_replicas is expected to larger than 0") - if num_replicas is not DEFAULT.VALUE and autoscaling_config is not DEFAULT.VALUE: + if num_replicas not in [DEFAULT.VALUE, None] and autoscaling_config not in [ + DEFAULT.VALUE, + None, + ]: raise ValueError( "Manually setting num_replicas is not allowed when " "autoscaling_config is provided." @@ -369,7 +372,7 @@ class constructor when starting up deployment replicas. These can ) config = DeploymentConfig.from_default( - num_replicas=num_replicas, + num_replicas=num_replicas if num_replicas is not None else 1, user_config=user_config, max_concurrent_queries=max_concurrent_queries, autoscaling_config=autoscaling_config, diff --git a/python/ray/serve/deployment.py b/python/ray/serve/deployment.py index 6819c5f190ed..7b86102af8dd 100644 --- a/python/ray/serve/deployment.py +++ b/python/ray/serve/deployment.py @@ -297,14 +297,16 @@ def options( func_or_class: Optional[Callable] = None, name: Default[str] = DEFAULT.VALUE, version: Default[str] = DEFAULT.VALUE, + num_replicas: Default[Optional[int]] = DEFAULT.VALUE, init_args: Default[Tuple[Any]] = DEFAULT.VALUE, init_kwargs: Default[Dict[Any, Any]] = DEFAULT.VALUE, route_prefix: Default[Union[str, None]] = DEFAULT.VALUE, - num_replicas: Default[int] = DEFAULT.VALUE, - ray_actor_options: Default[Dict] = DEFAULT.VALUE, - user_config: Default[Any] = DEFAULT.VALUE, + ray_actor_options: Default[Optional[Dict]] = DEFAULT.VALUE, + user_config: Default[Optional[Any]] = DEFAULT.VALUE, max_concurrent_queries: Default[int] = DEFAULT.VALUE, - autoscaling_config: Default[Union[Dict, AutoscalingConfig]] = DEFAULT.VALUE, + autoscaling_config: Default[ + Union[Dict, AutoscalingConfig, None] + ] = DEFAULT.VALUE, graceful_shutdown_wait_loop_s: Default[float] = DEFAULT.VALUE, graceful_shutdown_timeout_s: Default[float] = DEFAULT.VALUE, health_check_period_s: Default[float] = DEFAULT.VALUE, @@ -340,10 +342,10 @@ def options( if not _internal: new_config.user_configured_options.update(user_configured_options) - if ( - num_replicas is not DEFAULT.VALUE - and autoscaling_config is not DEFAULT.VALUE - ): + if num_replicas not in [DEFAULT.VALUE, None] and autoscaling_config not in [ + DEFAULT.VALUE, + None, + ]: raise ValueError( "Manually setting num_replicas is not allowed when " "autoscaling_config is provided." @@ -359,7 +361,7 @@ def options( "future!" ) - if num_replicas is not DEFAULT.VALUE: + if num_replicas not in [DEFAULT.VALUE, None]: new_config.num_replicas = num_replicas if user_config is not DEFAULT.VALUE: new_config.user_config = user_config @@ -421,14 +423,16 @@ def set_options( func_or_class: Optional[Callable] = None, name: Default[str] = DEFAULT.VALUE, version: Default[str] = DEFAULT.VALUE, + num_replicas: Default[Optional[int]] = DEFAULT.VALUE, init_args: Default[Tuple[Any]] = DEFAULT.VALUE, init_kwargs: Default[Dict[Any, Any]] = DEFAULT.VALUE, route_prefix: Default[Union[str, None]] = DEFAULT.VALUE, - num_replicas: Default[int] = DEFAULT.VALUE, - ray_actor_options: Default[Dict] = DEFAULT.VALUE, - user_config: Default[Any] = DEFAULT.VALUE, + ray_actor_options: Default[Optional[Dict]] = DEFAULT.VALUE, + user_config: Default[Optional[Any]] = DEFAULT.VALUE, max_concurrent_queries: Default[int] = DEFAULT.VALUE, - autoscaling_config: Default[Union[Dict, AutoscalingConfig]] = DEFAULT.VALUE, + autoscaling_config: Default[ + Union[Dict, AutoscalingConfig, None] + ] = DEFAULT.VALUE, graceful_shutdown_wait_loop_s: Default[float] = DEFAULT.VALUE, graceful_shutdown_timeout_s: Default[float] = DEFAULT.VALUE, health_check_period_s: Default[float] = DEFAULT.VALUE, diff --git a/python/ray/serve/schema.py b/python/ray/serve/schema.py index 747f62490329..73f3ae843c45 100644 --- a/python/ray/serve/schema.py +++ b/python/ray/serve/schema.py @@ -1,5 +1,5 @@ from pydantic import BaseModel, Field, Extra, root_validator, validator -from typing import Union, List, Dict, Set +from typing import Union, List, Dict, Set, Optional from ray._private.runtime_env.packaging import parse_uri from ray.serve._private.common import ( DeploymentStatusInfo, @@ -89,7 +89,7 @@ class DeploymentSchema( name: str = Field( ..., description=("Globally-unique name identifying this deployment.") ) - num_replicas: int = Field( + num_replicas: Optional[int] = Field( default=DEFAULT.VALUE, description=( "The number of processes that handle requests to this " @@ -120,7 +120,7 @@ class DeploymentSchema( ), gt=0, ) - user_config: Dict = Field( + user_config: Optional[Dict] = Field( default=DEFAULT.VALUE, description=( "Config to pass into this deployment's " @@ -128,7 +128,7 @@ class DeploymentSchema( "without restarting replicas" ), ) - autoscaling_config: Dict = Field( + autoscaling_config: Optional[Dict] = Field( default=DEFAULT.VALUE, description=( "Config specifying autoscaling " @@ -173,16 +173,15 @@ class DeploymentSchema( ), gt=0, ) - ray_actor_options: RayActorOptionsSchema = Field( + ray_actor_options: Optional[RayActorOptionsSchema] = Field( default=DEFAULT.VALUE, description="Options set for each replica actor." ) @root_validator def num_replicas_and_autoscaling_config_mutually_exclusive(cls, values): - if ( - values.get("num_replicas", DEFAULT.VALUE) is not DEFAULT.VALUE - and values.get("autoscaling_config", DEFAULT.VALUE) is not DEFAULT.VALUE - ): + if values.get("num_replicas", None) not in [DEFAULT.VALUE, None] and values.get( + "autoscaling_config", None + ) not in [DEFAULT.VALUE, None]: raise ValueError( "Manually setting num_replicas is not allowed " "when autoscaling_config is provided." diff --git a/python/ray/serve/tests/test_schema.py b/python/ray/serve/tests/test_schema.py index 51b81cfd65f3..60f248598924 100644 --- a/python/ray/serve/tests/test_schema.py +++ b/python/ray/serve/tests/test_schema.py @@ -204,27 +204,7 @@ class TestDeploymentSchema: def get_minimal_deployment_schema(self): # Generate a DeploymentSchema with the fewest possible attributes set - return { - "name": "deep", - "num_replicas": None, - "route_prefix": None, - "max_concurrent_queries": None, - "user_config": None, - "autoscaling_config": None, - "graceful_shutdown_wait_loop_s": None, - "graceful_shutdown_timeout_s": None, - "health_check_period_s": None, - "health_check_timeout_s": None, - "ray_actor_options": { - "runtime_env": {}, - "num_cpus": None, - "num_gpus": None, - "memory": None, - "object_store_memory": None, - "resources": {}, - "accelerator_type": None, - }, - } + return {"name": "deep"} def test_valid_deployment_schema(self): # Ensure a valid DeploymentSchema can be generated @@ -403,24 +383,6 @@ def get_valid_serve_application_schema(self): }, { "name": "deep", - "num_replicas": None, - "route_prefix": None, - "max_concurrent_queries": None, - "user_config": None, - "autoscaling_config": None, - "graceful_shutdown_wait_loop_s": None, - "graceful_shutdown_timeout_s": None, - "health_check_period_s": None, - "health_check_timeout_s": None, - "ray_actor_options": { - "runtime_env": {}, - "num_cpus": None, - "num_gpus": None, - "memory": None, - "object_store_memory": None, - "resources": {}, - "accelerator_type": None, - }, }, ], } From 190491e18c7bf399a2ff877911fac52d63f70acc Mon Sep 17 00:00:00 2001 From: Shreyas Krishnaswamy Date: Sun, 11 Sep 2022 00:49:18 -0700 Subject: [PATCH 19/37] Remove defaults from serve build Signed-off-by: Shreyas Krishnaswamy --- .../modules/serve/tests/test_serve_agent.py | 8 ++++---- .../_private/deployment_function_node.py | 1 + .../serve/_private/deployment_graph_build.py | 2 ++ python/ray/serve/deployment.py | 2 +- python/ray/serve/scripts.py | 20 +++++++++++++------ 5 files changed, 22 insertions(+), 11 deletions(-) diff --git a/dashboard/modules/serve/tests/test_serve_agent.py b/dashboard/modules/serve/tests/test_serve_agent.py index 6a3f91d3cdbf..cc26b9483b6f 100644 --- a/dashboard/modules/serve/tests/test_serve_agent.py +++ b/dashboard/modules/serve/tests/test_serve_agent.py @@ -27,7 +27,7 @@ def deploy_and_check_config(config: Dict): print("GET request returned correct config.") -@pytest.mark.skipif(sys.platform == "darwin", reason="Flaky on OSX.") +# @pytest.mark.skipif(sys.platform == "darwin", reason="Flaky on OSX.") def test_put_get(ray_start_stop): config1 = { "import_path": ( @@ -93,7 +93,7 @@ def test_put_get(ray_start_stop): print("Deployments are live and reachable over HTTP.\n") -@pytest.mark.skipif(sys.platform == "darwin", reason="Flaky on OSX.") +# @pytest.mark.skipif(sys.platform == "darwin", reason="Flaky on OSX.") def test_delete(ray_start_stop): config = { "import_path": "dir.subdir.a.add_and_sub.serve_dag", @@ -159,7 +159,7 @@ def test_delete(ray_start_stop): print("Deployments have been deleted and are not reachable.\n") -@pytest.mark.skipif(sys.platform == "darwin", reason="Flaky on OSX.") +# @pytest.mark.skipif(sys.platform == "darwin", reason="Flaky on OSX.") def test_get_status(ray_start_stop): print("Checking status info before any deployments.") @@ -209,7 +209,7 @@ def test_get_status(ray_start_stop): print("Serve app status is correct.") -@pytest.mark.skipif(sys.platform == "darwin", reason="Flaky on OSX.") +# @pytest.mark.skipif(sys.platform == "darwin", reason="Flaky on OSX.") def test_serve_namespace(ray_start_stop): """ Check that the Dashboard's Serve can interact with the Python API diff --git a/python/ray/serve/_private/deployment_function_node.py b/python/ray/serve/_private/deployment_function_node.py index 03a7e6f6e092..be47328d80c1 100644 --- a/python/ray/serve/_private/deployment_function_node.py +++ b/python/ray/serve/_private/deployment_function_node.py @@ -58,6 +58,7 @@ def __init__( init_args=(), init_kwargs={}, route_prefix=route_prefix, + _internal=True, ) else: self._deployment: Deployment = Deployment( diff --git a/python/ray/serve/_private/deployment_graph_build.py b/python/ray/serve/_private/deployment_graph_build.py index 7bf7d8e008e6..a7a0affbf149 100644 --- a/python/ray/serve/_private/deployment_graph_build.py +++ b/python/ray/serve/_private/deployment_graph_build.py @@ -211,6 +211,7 @@ def replace_with_handle(node): init_args=replaced_deployment_init_args, init_kwargs=replaced_deployment_init_kwargs, route_prefix=route_prefix, + _internal=True, ) return DeploymentNode( @@ -377,6 +378,7 @@ def replace_with_handle(node): return original_driver_deployment.options( init_args=replaced_deployment_init_args, init_kwargs=replaced_deployment_init_kwargs, + _internal=True, ) diff --git a/python/ray/serve/deployment.py b/python/ray/serve/deployment.py index 7b86102af8dd..4c0b4d1999ca 100644 --- a/python/ray/serve/deployment.py +++ b/python/ray/serve/deployment.py @@ -354,7 +354,7 @@ def options( if num_replicas == 0: raise ValueError("num_replicas is expected to larger than 0") - if not _internal and version is DEFAULT.VALUE: + if not _internal and version is not DEFAULT.VALUE: logger.warning( "DeprecationWarning: `version` in `Deployment.options()` has been " "deprecated. Explicitly specifying version will raise an error in the " diff --git a/python/ray/serve/scripts.py b/python/ray/serve/scripts.py index 18888cbd865b..da651e87a4bd 100644 --- a/python/ray/serve/scripts.py +++ b/python/ray/serve/scripts.py @@ -462,12 +462,17 @@ def build(import_path: str, app_dir: str, output_path: Optional[str]): app = build_app(node) - config = ServeApplicationSchema( - deployments=[deployment_to_schema(d) for d in app.deployments.values()] - ).dict() - config["import_path"] = import_path - config["host"] = "0.0.0.0" - config["port"] = 8000 + config = { + "import_path": import_path, + "runtime_env": {}, + "host": "0.0.0.0", + "port": 8000, + } + config.update( + ServeApplicationSchema( + deployments=[deployment_to_schema(d) for d in app.deployments.values()] + ).dict(exclude_defaults=True) + ) config_str = ( "# This file was generated using the `serve build` command " @@ -477,6 +482,9 @@ def build(import_path: str, app_dir: str, output_path: Optional[str]): config, Dumper=ServeBuildDumper, default_flow_style=False, sort_keys=False ) + # Remove extraneous newline + config_str = config_str[:-1] + with open(output_path, "w") if output_path else sys.stdout as f: f.write(config_str) From a64a65187c1b11a04b5ecaee328a99f0ee9c2392 Mon Sep 17 00:00:00 2001 From: Shreyas Krishnaswamy Date: Sun, 11 Sep 2022 00:52:29 -0700 Subject: [PATCH 20/37] Disable flaky serve agent tests on mac Signed-off-by: Shreyas Krishnaswamy --- dashboard/modules/serve/tests/test_serve_agent.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/dashboard/modules/serve/tests/test_serve_agent.py b/dashboard/modules/serve/tests/test_serve_agent.py index cc26b9483b6f..6a3f91d3cdbf 100644 --- a/dashboard/modules/serve/tests/test_serve_agent.py +++ b/dashboard/modules/serve/tests/test_serve_agent.py @@ -27,7 +27,7 @@ def deploy_and_check_config(config: Dict): print("GET request returned correct config.") -# @pytest.mark.skipif(sys.platform == "darwin", reason="Flaky on OSX.") +@pytest.mark.skipif(sys.platform == "darwin", reason="Flaky on OSX.") def test_put_get(ray_start_stop): config1 = { "import_path": ( @@ -93,7 +93,7 @@ def test_put_get(ray_start_stop): print("Deployments are live and reachable over HTTP.\n") -# @pytest.mark.skipif(sys.platform == "darwin", reason="Flaky on OSX.") +@pytest.mark.skipif(sys.platform == "darwin", reason="Flaky on OSX.") def test_delete(ray_start_stop): config = { "import_path": "dir.subdir.a.add_and_sub.serve_dag", @@ -159,7 +159,7 @@ def test_delete(ray_start_stop): print("Deployments have been deleted and are not reachable.\n") -# @pytest.mark.skipif(sys.platform == "darwin", reason="Flaky on OSX.") +@pytest.mark.skipif(sys.platform == "darwin", reason="Flaky on OSX.") def test_get_status(ray_start_stop): print("Checking status info before any deployments.") @@ -209,7 +209,7 @@ def test_get_status(ray_start_stop): print("Serve app status is correct.") -# @pytest.mark.skipif(sys.platform == "darwin", reason="Flaky on OSX.") +@pytest.mark.skipif(sys.platform == "darwin", reason="Flaky on OSX.") def test_serve_namespace(ray_start_stop): """ Check that the Dashboard's Serve can interact with the Python API From 1fc6cbf7d3393200c90ddfd6fad260ddfaced565 Mon Sep 17 00:00:00 2001 From: Shreyas Krishnaswamy Date: Sun, 11 Sep 2022 01:03:02 -0700 Subject: [PATCH 21/37] Exclude defaults during serialization Signed-off-by: Shreyas Krishnaswamy --- python/ray/serve/_private/json_serde.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/python/ray/serve/_private/json_serde.py b/python/ray/serve/_private/json_serde.py index e1e157b4daed..50d938e22f08 100644 --- a/python/ray/serve/_private/json_serde.py +++ b/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), } elif isinstance(obj, RayServeHandle): return _serve_handle_to_json_dict(obj) From 7ba9676d18a703133e57c818c7e0cad31748bc85 Mon Sep 17 00:00:00 2001 From: Shreyas Krishnaswamy Date: Tue, 13 Sep 2022 15:28:34 -0700 Subject: [PATCH 22/37] Introduce test_deployment.py Signed-off-by: Shreyas Krishnaswamy --- python/ray/serve/BUILD | 8 +++++ python/ray/serve/tests/test_deployment.py | 36 +++++++++++++++++++++++ 2 files changed, 44 insertions(+) create mode 100644 python/ray/serve/tests/test_deployment.py diff --git a/python/ray/serve/BUILD b/python/ray/serve/BUILD index 17e7a7432a8b..546760a84d31 100644 --- a/python/ray/serve/BUILD +++ b/python/ray/serve/BUILD @@ -49,6 +49,14 @@ py_test( deps = [":serve_lib"], ) +py_test( + name = "test_deployment", + size = "size", + srcs = serve_tests_srcs, + tags = ["exclusive", "team:serve"], + deps = [":serve_lib"], +) + py_test( name = "test_healthcheck", size = "medium", diff --git a/python/ray/serve/tests/test_deployment.py b/python/ray/serve/tests/test_deployment.py new file mode 100644 index 000000000000..468457291ccd --- /dev/null +++ b/python/ray/serve/tests/test_deployment.py @@ -0,0 +1,36 @@ +import sys +import pytest + + +class TestDeploymentOptions: + + deployment_options = [ + "func_or_class", + "name", + "version", + "num_replicas", + "init_args", + "init_kwargs", + "route_prefix", + "ray_actor_options", + "user_config", + "max_concurrent_queries", + "autoscaling_config", + "graceful_shutdown_wait_loop_s", + "graceful_shutdown_timeout_s", + "health_check_period_s", + "health_check_timeout_s", + ] + + # Options that can be None + nullable_options = [ + "num_replicas", + "route_prefix", + "autoscaling_config", + "user_config", + "ray_actor_options", + ] + + +if __name__ == "__main__": + sys.exit(pytest.main(["-v", "-s", __file__])) From aa075b68b4b181c152cf15a08473a880e9c5100d Mon Sep 17 00:00:00 2001 From: Shreyas Krishnaswamy Date: Tue, 13 Sep 2022 15:47:12 -0700 Subject: [PATCH 23/37] Make ray_actor_options not nullable Signed-off-by: Shreyas Krishnaswamy --- python/ray/serve/api.py | 4 ++-- python/ray/serve/schema.py | 2 +- python/ray/serve/tests/test_deployment.py | 1 - 3 files changed, 3 insertions(+), 4 deletions(-) diff --git a/python/ray/serve/api.py b/python/ray/serve/api.py index 65b67838a291..01c185458284 100644 --- a/python/ray/serve/api.py +++ b/python/ray/serve/api.py @@ -281,7 +281,7 @@ def deployment( init_args: Default[Tuple[Any]] = DEFAULT.VALUE, init_kwargs: Default[Dict[Any, Any]] = DEFAULT.VALUE, route_prefix: Default[Union[str, None]] = DEFAULT.VALUE, - ray_actor_options: Default[Optional[Dict]] = DEFAULT.VALUE, + ray_actor_options: Default[Dict] = DEFAULT.VALUE, user_config: Default[Optional[Any]] = DEFAULT.VALUE, max_concurrent_queries: Default[int] = DEFAULT.VALUE, autoscaling_config: Default[Union[Dict, AutoscalingConfig, None]] = DEFAULT.VALUE, @@ -318,7 +318,7 @@ class constructor when starting up deployment replicas. These can to '/a/b', '/a/b/', and '/a/b/c' go to B. Routes must not end with a '/' unless they're the root (just '/'), which acts as a catch-all. - ray_actor_options (Default[Optional[Dict]]): Options to be passed to each + ray_actor_options (Default[Dict]): Options to be passed to each replica Ray actor's constructor, such as resource requirements. user_config (Default[Optional[Any]]): Config to pass to the reconfigure method of the deployment. This can be updated diff --git a/python/ray/serve/schema.py b/python/ray/serve/schema.py index 73f3ae843c45..d26e1cb9b9f6 100644 --- a/python/ray/serve/schema.py +++ b/python/ray/serve/schema.py @@ -173,7 +173,7 @@ class DeploymentSchema( ), gt=0, ) - ray_actor_options: Optional[RayActorOptionsSchema] = Field( + ray_actor_options: RayActorOptionsSchema = Field( default=DEFAULT.VALUE, description="Options set for each replica actor." ) diff --git a/python/ray/serve/tests/test_deployment.py b/python/ray/serve/tests/test_deployment.py index 468457291ccd..8c2c7fb44665 100644 --- a/python/ray/serve/tests/test_deployment.py +++ b/python/ray/serve/tests/test_deployment.py @@ -28,7 +28,6 @@ class TestDeploymentOptions: "route_prefix", "autoscaling_config", "user_config", - "ray_actor_options", ] From b7c34c017d4a8834b8ae6fa83099e06bba3964a3 Mon Sep 17 00:00:00 2001 From: Shreyas Krishnaswamy Date: Tue, 13 Sep 2022 17:51:44 -0700 Subject: [PATCH 24/37] Add tests for user_configured_options Signed-off-by: Shreyas Krishnaswamy --- python/ray/serve/config.py | 4 +- python/ray/serve/tests/test_deployment.py | 182 ++++++++++++++++++++-- 2 files changed, 168 insertions(+), 18 deletions(-) diff --git a/python/ray/serve/config.py b/python/ray/serve/config.py index 7528a8a31a91..6f524422bd53 100644 --- a/python/ray/serve/config.py +++ b/python/ray/serve/config.py @@ -188,7 +188,8 @@ def needs_pickle(self): def to_proto(self): data = self.dict() - if data.get("user_config"): + print(data["user_configured_options"]) + if data.get("user_config") is not None: if self.needs_pickle(): data["user_config"] = cloudpickle.dumps(data["user_config"]) if data.get("autoscaling_config"): @@ -196,6 +197,7 @@ def to_proto(self): **data["autoscaling_config"] ) data["user_configured_options"] = list(data["user_configured_options"]) + # print(data, type(data["user_configured_options"])) return DeploymentConfigProto(**data) def to_proto_bytes(self): diff --git a/python/ray/serve/tests/test_deployment.py b/python/ray/serve/tests/test_deployment.py index 8c2c7fb44665..c5a9ae5b5fc6 100644 --- a/python/ray/serve/tests/test_deployment.py +++ b/python/ray/serve/tests/test_deployment.py @@ -1,26 +1,100 @@ import sys import pytest +import random +import itertools +from typing import Dict, List + +from ray import serve +from ray.serve.config import DeploymentConfig +from ray.serve.deployment import deployment_to_schema, schema_to_deployment + + +def get_random_dict_combos(d: Dict, n: int) -> List[Dict]: + """Gets n random combinations of dictionary d. + + Returns: + List of dictionary combinations of lengths from 0 to len(d). List + contains n random combinations of d's elements. + """ + + # Shuffle dictionary without modifying original dictionary + d = dict(random.sample(list(d.items()), len(d))) + + combos = [] + + # Sample random combos of random size + subset_sizes = list(range(len(d) + 1)) + random.shuffle(subset_sizes) + + for subset_size in subset_sizes: + subset_combo_iterator = map( + dict, itertools.combinations(d.items(), subset_size) + ) + if len(combos) < n: + subset_combos = list( + itertools.islice(subset_combo_iterator, n - len(combos)) + ) + combos.extend(subset_combos) + else: + break + + return combos + + +class TestGetDictCombos: + def test_empty(self): + assert get_random_dict_combos({}, 1) == [{}] + + def test_basic(self): + d = {"a": 1, "b": 2, "c": 3} + combos = get_random_dict_combos(d, 8) + + # Sort combos for comparison (sort by length, break ties by value sum) + combos.sort(key=lambda d: len(d) * 100 + sum(d.values())) + + assert combos == [ + # Dictionaries of length 0 + {}, + # Dictionaries of length 1 + *({"a": 1}, {"b": 2}, {"c": 3}), + # Dictionaries of length 2 + *({"a": 1, "b": 2}, {"a": 1, "c": 3}, {"b": 2, "c": 3}), + # Dictionaries of length 3 + {"a": 1, "b": 2, "c": 3}, + ] + + def test_len(self): + d = {i: i + 1 for i in range(50)} + assert len(get_random_dict_combos(d, 1000)) == 1000 + + def test_randomness(self): + d = {i: i + 1 for i in range(1000)} + combo1 = get_random_dict_combos(d, 1000)[0] + combo2 = get_random_dict_combos(d, 1000)[0] + assert combo1 != combo2 class TestDeploymentOptions: - deployment_options = [ - "func_or_class", - "name", - "version", - "num_replicas", - "init_args", - "init_kwargs", - "route_prefix", - "ray_actor_options", - "user_config", - "max_concurrent_queries", - "autoscaling_config", - "graceful_shutdown_wait_loop_s", - "graceful_shutdown_timeout_s", - "health_check_period_s", - "health_check_timeout_s", - ] + # Deployment options mapped to sample input + deployment_options = { + "name": "test", + "version": "abcd", + "num_replicas": 1, + "init_args": (), + "init_kwargs": {}, + "route_prefix": "/", + "ray_actor_options": {}, + "user_config": {}, + "max_concurrent_queries": 10, + "autoscaling_config": None, + "graceful_shutdown_wait_loop_s": 10, + "graceful_shutdown_timeout_s": 10, + "health_check_period_s": 10, + "health_check_timeout_s": 10, + } + + deployment_option_combos = get_random_dict_combos(deployment_options, 1000) # Options that can be None nullable_options = [ @@ -30,6 +104,80 @@ class TestDeploymentOptions: "user_config", ] + @pytest.mark.parametrize("options", deployment_option_combos) + def test_user_configured_options(self, options: Dict): + """Check that user_configured_options tracks the correct options. + + Args: + options: Maps deployment option strings (e.g. "name", + "num_replicas", etc.) to sample inputs. Pairs come from + TestDeploymentOptions.deployment_options. + """ + + @serve.deployment(**options) + def f(): + pass + + assert f._config.user_configured_options == set(options.keys()) + + @pytest.mark.parametrize("options", deployment_option_combos) + def test_user_configured_options_schematized(self, options: Dict): + """Check user_configured_options after schematization. + + Args: + options: Maps deployment option strings (e.g. "name", + "num_replicas", etc.) to sample inputs. Pairs come from + TestDeploymentOptions.deployment_options. + """ + + # Some options won't be considered user-configured after schematization + # since the schema doesn't track them. + untracked_options = ["name", "version", "init_args", "init_kwargs"] + + for option in untracked_options: + if option in options: + del options[option] + + @serve.deployment(**options) + def f(): + pass + + schematized_deployment = deployment_to_schema(f) + deschematized_deployment = schema_to_deployment(schematized_deployment) + + # Don't track name in the deschematized deployment since it's optional + # in deployment decorator but required in schema, leading to + # inconsistent behavior. + if "name" in deschematized_deployment._config.user_configured_options: + deschematized_deployment._config.user_configured_options.remove("name") + + assert deschematized_deployment._config.user_configured_options == set( + options.keys() + ) + + @pytest.mark.parametrize("options", deployment_option_combos) + def test_user_configured_options_serialized(self, options: Dict): + """Check user_configured_options after serialization. + + Args: + options: Maps deployment option strings (e.g. "name", + "num_replicas", etc.) to sample inputs. Pairs come from + TestDeploymentOptions.deployment_options. + """ + + # init_kwargs requires independent serialization, so we omit it. + if "init_kwargs" in options: + del options["init_kwargs"] + + @serve.deployment(**options) + def f(): + pass + + serialized_config = f._config.to_proto_bytes() + deserialized_config = DeploymentConfig.from_proto_bytes(serialized_config) + + assert deserialized_config.user_configured_options == set(options.keys()) + if __name__ == "__main__": sys.exit(pytest.main(["-v", "-s", __file__])) From ba0892e0f6e92a0a392e1584a709df250c399731 Mon Sep 17 00:00:00 2001 From: Shreyas Krishnaswamy Date: Tue, 13 Sep 2022 18:03:42 -0700 Subject: [PATCH 25/37] Add test for empty user_config Signed-off-by: Shreyas Krishnaswamy --- python/ray/serve/config.py | 2 -- python/ray/serve/tests/test_api.py | 16 ++++++++++++++++ 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/python/ray/serve/config.py b/python/ray/serve/config.py index 6f524422bd53..6f6cb4a5eaff 100644 --- a/python/ray/serve/config.py +++ b/python/ray/serve/config.py @@ -188,7 +188,6 @@ def needs_pickle(self): def to_proto(self): data = self.dict() - print(data["user_configured_options"]) if data.get("user_config") is not None: if self.needs_pickle(): data["user_config"] = cloudpickle.dumps(data["user_config"]) @@ -197,7 +196,6 @@ def to_proto(self): **data["autoscaling_config"] ) data["user_configured_options"] = list(data["user_configured_options"]) - # print(data, type(data["user_configured_options"])) return DeploymentConfigProto(**data) def to_proto_bytes(self): diff --git a/python/ray/serve/tests/test_api.py b/python/ray/serve/tests/test_api.py index ebf43efd6e4a..ebb009293d3e 100644 --- a/python/ray/serve/tests/test_api.py +++ b/python/ray/serve/tests/test_api.py @@ -212,6 +212,22 @@ def check(val, num_replicas): wait_for_condition(lambda: check("456", 3)) +def test_user_config_empty(serve_instance): + @serve.deployment(user_config={}) + class Counter: + def __init__(self): + self.count = 0 + + def __call__(self, *args): + return self.count + + def reconfigure(self, config): + self.count += 1 + + handle = serve.run(Counter.bind()) + assert ray.get(handle.remote()) == 1 + + def test_scaling_replicas(serve_instance): @serve.deployment(name="counter", num_replicas=2) class Counter: From 9b807f675bc4331c1b4b9da1e4b0fef1074225e0 Mon Sep 17 00:00:00 2001 From: Shreyas Krishnaswamy Date: Tue, 13 Sep 2022 18:12:51 -0700 Subject: [PATCH 26/37] Create test_nullable_options Signed-off-by: Shreyas Krishnaswamy --- python/ray/serve/tests/test_deployment.py | 36 ++++++++++++++++++----- 1 file changed, 28 insertions(+), 8 deletions(-) diff --git a/python/ray/serve/tests/test_deployment.py b/python/ray/serve/tests/test_deployment.py index c5a9ae5b5fc6..5499600d462a 100644 --- a/python/ray/serve/tests/test_deployment.py +++ b/python/ray/serve/tests/test_deployment.py @@ -96,14 +96,6 @@ class TestDeploymentOptions: deployment_option_combos = get_random_dict_combos(deployment_options, 1000) - # Options that can be None - nullable_options = [ - "num_replicas", - "route_prefix", - "autoscaling_config", - "user_config", - ] - @pytest.mark.parametrize("options", deployment_option_combos) def test_user_configured_options(self, options: Dict): """Check that user_configured_options tracks the correct options. @@ -178,6 +170,34 @@ def f(): assert deserialized_config.user_configured_options == set(options.keys()) + @pytest.mark.parametrize( + "option", + [ + "num_replicas", + "route_prefix", + "autoscaling_config", + "user_config", + ], + ) + def test_nullable_options(self, option: str): + """Check that nullable options can be set to None.""" + deployment_options = {option: None} + + # One of "num_replicas" or "autoscaling_config" must be provided. + if option == "num_replicas": + deployment_options["autoscaling_config"] = { + "min_replicas": 1, + "max_replicas": 5, + "target_num_ongoing_requests_per_replica": 5, + } + elif option == "autoscaling_config": + deployment_options["num_replicas"] = 5 + + # Deployment should be created without error. + @serve.deployment(**deployment_options) + def f(): + pass + if __name__ == "__main__": sys.exit(pytest.main(["-v", "-s", __file__])) From 90a15dd4ef33b8c218c799f49b4e5105afa179dd Mon Sep 17 00:00:00 2001 From: Shreyas Krishnaswamy Date: Tue, 13 Sep 2022 18:17:11 -0700 Subject: [PATCH 27/37] Add test for nullable options to schema Signed-off-by: Shreyas Krishnaswamy --- python/ray/serve/tests/test_deployment.py | 1 + python/ray/serve/tests/test_schema.py | 27 +++++++++++++++++++++++ 2 files changed, 28 insertions(+) diff --git a/python/ray/serve/tests/test_deployment.py b/python/ray/serve/tests/test_deployment.py index 5499600d462a..af68c4f8f6c3 100644 --- a/python/ray/serve/tests/test_deployment.py +++ b/python/ray/serve/tests/test_deployment.py @@ -181,6 +181,7 @@ def f(): ) def test_nullable_options(self, option: str): """Check that nullable options can be set to None.""" + deployment_options = {option: None} # One of "num_replicas" or "autoscaling_config" must be provided. diff --git a/python/ray/serve/tests/test_schema.py b/python/ray/serve/tests/test_schema.py index 60f248598924..25558593e20d 100644 --- a/python/ray/serve/tests/test_schema.py +++ b/python/ray/serve/tests/test_schema.py @@ -342,6 +342,33 @@ def test_extra_fields_invalid_deployment_schema(self): with pytest.raises(ValidationError): DeploymentSchema.parse_obj(deployment_schema) + @pytest.mark.parametrize( + "option", + [ + "num_replicas", + "route_prefix", + "autoscaling_config", + "user_config", + ], + ) + def test_nullable_options(self, option: str): + """Check that nullable options can be set to None.""" + + deployment_options = {"name": "test", option: None} + + # One of "num_replicas" or "autoscaling_config" must be provided. + if option == "num_replicas": + deployment_options["autoscaling_config"] = { + "min_replicas": 1, + "max_replicas": 5, + "target_num_ongoing_requests_per_replica": 5, + } + elif option == "autoscaling_config": + deployment_options["num_replicas"] = 5 + + # Schema should be created without error. + DeploymentSchema.parse_obj(deployment_options) + class TestServeApplicationSchema: def get_valid_serve_application_schema(self): From af68e2e499914b99003fa6ffea159b1d4d520ab5 Mon Sep 17 00:00:00 2001 From: Shreyas Krishnaswamy Date: Tue, 13 Sep 2022 18:28:59 -0700 Subject: [PATCH 28/37] Remove route_prefixes from serve build output Signed-off-by: Shreyas Krishnaswamy --- python/ray/serve/_private/deployment_graph_build.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/python/ray/serve/_private/deployment_graph_build.py b/python/ray/serve/_private/deployment_graph_build.py index a7a0affbf149..b0251f689ea2 100644 --- a/python/ray/serve/_private/deployment_graph_build.py +++ b/python/ray/serve/_private/deployment_graph_build.py @@ -424,7 +424,9 @@ def process_ingress_deployment_in_serve_dag( if ingress_deployment.route_prefix in [None, f"/{ingress_deployment.name}"]: # Override default prefix to "/" on the ingress deployment, if user # didn't provide anything in particular. - new_ingress_deployment = ingress_deployment.options(route_prefix="/") + new_ingress_deployment = ingress_deployment.options( + route_prefix="/", _internal=True + ) deployments[-1] = new_ingress_deployment # Erase all non ingress deployment route prefix @@ -440,8 +442,8 @@ def process_ingress_deployment_in_serve_dag( "serve DAG. " ) else: - # Earse all default prefix to None for non-ingress deployments to + # Erase all default prefix to None for non-ingress deployments to # disable HTTP - deployments[i] = deployment.options(route_prefix=None) + deployments[i] = deployment.options(route_prefix=None, _internal=True) return deployments From 696ae581034469418623869ad1c38c1027e82aee Mon Sep 17 00:00:00 2001 From: Shreyas Krishnaswamy Date: Tue, 13 Sep 2022 22:37:16 -0700 Subject: [PATCH 29/37] Make size 'small' Signed-off-by: Shreyas Krishnaswamy --- python/ray/serve/BUILD | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/python/ray/serve/BUILD b/python/ray/serve/BUILD index 546760a84d31..326a34613d66 100644 --- a/python/ray/serve/BUILD +++ b/python/ray/serve/BUILD @@ -51,7 +51,7 @@ py_test( py_test( name = "test_deployment", - size = "size", + size = "small", srcs = serve_tests_srcs, tags = ["exclusive", "team:serve"], deps = [":serve_lib"], From 0463c2e69f0a5a9029cbc8e655863743cddea70b Mon Sep 17 00:00:00 2001 From: Shreyas Krishnaswamy Date: Tue, 20 Sep 2022 14:18:33 -0700 Subject: [PATCH 30/37] Remove unused arg from docstring in from_default Signed-off-by: Shreyas Krishnaswamy --- python/ray/serve/config.py | 5 ----- 1 file changed, 5 deletions(-) diff --git a/python/ray/serve/config.py b/python/ray/serve/config.py index 6f6cb4a5eaff..ed979bb0d446 100644 --- a/python/ray/serve/config.py +++ b/python/ray/serve/config.py @@ -247,11 +247,6 @@ def from_default(cls, **kwargs): Ignores any kwargs set to DEFAULT.VALUE. - Args: - ignore_default: When True, any valid keywords with value - DEFAULT.VALUE are ignored, and their values stay default. - Invalid keywords still raise a TypeError. - Raises: TypeError: when a keyword that's not an argument to the class is passed in. From 2fba994c875b21c2579c568595a4b65dadce29f5 Mon Sep 17 00:00:00 2001 From: Shreyas Krishnaswamy Date: Tue, 20 Sep 2022 14:20:38 -0700 Subject: [PATCH 31/37] Add comment about flag Signed-off-by: Shreyas Krishnaswamy --- python/ray/serve/_private/json_serde.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/python/ray/serve/_private/json_serde.py b/python/ray/serve/_private/json_serde.py index 50d938e22f08..6b8a1d5e7e6c 100644 --- a/python/ray/serve/_private/json_serde.py +++ b/python/ray/serve/_private/json_serde.py @@ -88,6 +88,9 @@ def default(self, obj): if isinstance(obj, DeploymentSchema): return { DAGNODE_TYPE_KEY: "DeploymentSchema", + # The schema's default values are Python enums that aren't + # JSON-serializable. exclude_defaults omits these, so the + # return value can be JSON-serialized. "schema": obj.dict(exclude_defaults=True), } elif isinstance(obj, RayServeHandle): From 67fca53dd486044a4ad4a5f11b2b9431d3968160 Mon Sep 17 00:00:00 2001 From: Shreyas Krishnaswamy Date: Tue, 20 Sep 2022 14:22:25 -0700 Subject: [PATCH 32/37] Rename test_deployment to test_deployment_class Signed-off-by: Shreyas Krishnaswamy --- python/ray/serve/BUILD | 2 +- .../tests/{test_deployment.py => test_deployment_class.py} | 0 2 files changed, 1 insertion(+), 1 deletion(-) rename python/ray/serve/tests/{test_deployment.py => test_deployment_class.py} (100%) diff --git a/python/ray/serve/BUILD b/python/ray/serve/BUILD index 326a34613d66..252000b22311 100644 --- a/python/ray/serve/BUILD +++ b/python/ray/serve/BUILD @@ -50,7 +50,7 @@ py_test( ) py_test( - name = "test_deployment", + name = "test_deployment_class", size = "small", srcs = serve_tests_srcs, tags = ["exclusive", "team:serve"], diff --git a/python/ray/serve/tests/test_deployment.py b/python/ray/serve/tests/test_deployment_class.py similarity index 100% rename from python/ray/serve/tests/test_deployment.py rename to python/ray/serve/tests/test_deployment_class.py From 6f5502c265efcd4f3af4697534775db6ea13103a Mon Sep 17 00:00:00 2001 From: Shreyas Krishnaswamy Date: Tue, 27 Sep 2022 14:27:02 -0700 Subject: [PATCH 33/37] Address comments Signed-off-by: Shreyas Krishnaswamy --- python/ray/serve/_private/json_serde.py | 4 ++-- python/ray/serve/deployment.py | 10 ++++------ python/ray/serve/scripts.py | 4 ++-- 3 files changed, 8 insertions(+), 10 deletions(-) diff --git a/python/ray/serve/_private/json_serde.py b/python/ray/serve/_private/json_serde.py index 6b8a1d5e7e6c..72d3f4077193 100644 --- a/python/ray/serve/_private/json_serde.py +++ b/python/ray/serve/_private/json_serde.py @@ -89,8 +89,8 @@ def default(self, obj): return { DAGNODE_TYPE_KEY: "DeploymentSchema", # The schema's default values are Python enums that aren't - # JSON-serializable. exclude_defaults omits these, so the - # return value can be JSON-serialized. + # JSON-serializable by design. exclude_defaults omits these, + # so the return value can be JSON-serialized. "schema": obj.dict(exclude_defaults=True), } elif isinstance(obj, RayServeHandle): diff --git a/python/ray/serve/deployment.py b/python/ray/serve/deployment.py index 4c0b4d1999ca..1974bab42dff 100644 --- a/python/ray/serve/deployment.py +++ b/python/ray/serve/deployment.py @@ -322,12 +322,10 @@ def options( Refer to @serve.deployment decorator docstring for all non-private arguments. - _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. + _internal: If True, this function won't log deprecation warnings + and won't update this deployment's config's + user_configured_options. It should only be True when used + internally by Serve. It should be False when called by users. """ # Create list of all user-configured options from keyword args diff --git a/python/ray/serve/scripts.py b/python/ray/serve/scripts.py index c07708b48d1d..a1673507c814 100644 --- a/python/ray/serve/scripts.py +++ b/python/ray/serve/scripts.py @@ -486,8 +486,8 @@ def build(import_path: str, app_dir: str, output_path: Optional[str]): config, Dumper=ServeBuildDumper, default_flow_style=False, sort_keys=False ) - # Remove extraneous newline - config_str = config_str[:-1] + # Ensure file ends with only one newline + config_str = config_str.rstrip("\n") + "\n" with open(output_path, "w") if output_path else sys.stdout as f: f.write(config_str) From 842ceee14345f253a49a97669c303d8deb444335 Mon Sep 17 00:00:00 2001 From: Shreyas Krishnaswamy Date: Tue, 27 Sep 2022 14:43:56 -0700 Subject: [PATCH 34/37] Rename user_configured_options to user_configured_option_names Signed-off-by: Shreyas Krishnaswamy --- python/ray/serve/api.py | 4 ++-- python/ray/serve/config.py | 18 +++++++++------ python/ray/serve/deployment.py | 10 ++++----- python/ray/serve/schema.py | 6 ++--- .../ray/serve/tests/test_deployment_class.py | 22 +++++++++---------- src/ray/protobuf/serve.proto | 2 +- 6 files changed, 33 insertions(+), 29 deletions(-) diff --git a/python/ray/serve/api.py b/python/ray/serve/api.py index 371dec6f0a14..b7223605631b 100644 --- a/python/ray/serve/api.py +++ b/python/ray/serve/api.py @@ -347,7 +347,7 @@ class constructor when starting up deployment replicas. These can """ # Create list of all user-configured options from keyword args - user_configured_options = [ + user_configured_option_names = [ option for option, value in locals().items() if option != "_func_or_class" and value is not DEFAULT.VALUE @@ -383,7 +383,7 @@ class constructor when starting up deployment replicas. These can health_check_period_s=health_check_period_s, health_check_timeout_s=health_check_timeout_s, ) - config.user_configured_options = set(user_configured_options) + config.user_configured_option_names = set(user_configured_option_names) def decorator(_func_or_class): return Deployment( diff --git a/python/ray/serve/config.py b/python/ray/serve/config.py index 5f1431be770a..98bcc039ff23 100644 --- a/python/ray/serve/config.py +++ b/python/ray/serve/config.py @@ -123,8 +123,8 @@ class DeploymentConfig(BaseModel): health_check_timeout_s (Optional[float]): Timeout that the controller will wait for a response from the replica's health check before marking it unhealthy. - user_configured_options (Set[str]): - The options manually configured by the user. + user_configured_option_names (Set[str]): + The names of options manually configured by the user. """ num_replicas: NonNegativeInt = 1 @@ -153,8 +153,8 @@ class DeploymentConfig(BaseModel): version: Optional[str] = None - # Contains the deployment options manually set by the user - user_configured_options: Set[str] = set() + # Contains the names of deployment options manually set by the user + user_configured_option_names: Set[str] = set() class Config: validate_assignment = True @@ -195,7 +195,9 @@ def to_proto(self): data["autoscaling_config"] = AutoscalingConfigProto( **data["autoscaling_config"] ) - data["user_configured_options"] = list(data["user_configured_options"]) + data["user_configured_option_names"] = list( + data["user_configured_option_names"] + ) return DeploymentConfigProto(**data) def to_proto_bytes(self): @@ -232,8 +234,10 @@ def from_proto(cls, proto: DeploymentConfigProto): if "version" in data: if data["version"] == "": data["version"] = None - if "user_configured_options" in data: - data["user_configured_options"] = set(data["user_configured_options"]) + if "user_configured_option_names" in data: + data["user_configured_option_names"] = set( + data["user_configured_option_names"] + ) return cls(**data) @classmethod diff --git a/python/ray/serve/deployment.py b/python/ray/serve/deployment.py index 1974bab42dff..fadf8c377ccc 100644 --- a/python/ray/serve/deployment.py +++ b/python/ray/serve/deployment.py @@ -324,12 +324,12 @@ def options( _internal: If True, this function won't log deprecation warnings and won't update this deployment's config's - user_configured_options. It should only be True when used + user_configured_option_names. It should only be True when used internally by Serve. It should be False when called by users. """ # Create list of all user-configured options from keyword args - user_configured_options = [ + user_configured_option_names = [ option for option, value in locals().items() if option not in {"self", "_func_or_class", "_internal"} @@ -338,7 +338,7 @@ def options( new_config = deepcopy(self._config) if not _internal: - new_config.user_configured_options.update(user_configured_options) + new_config.user_configured_option_names.update(user_configured_option_names) if num_replicas not in [DEFAULT.VALUE, None] and autoscaling_config not in [ DEFAULT.VALUE, @@ -523,7 +523,7 @@ def deployment_to_schema(d: Deployment) -> DeploymentSchema: # which options were set by the user. Name is a required field in the # schema, so it should be passed in explicitly. for option in list(deployment_options.keys()): - if option != "name" and option not in d._config.user_configured_options: + if option != "name" and option not in d._config.user_configured_option_names: del deployment_options[option] # TODO(Sihan) DeploymentConfig num_replicas and auto_config can be set together @@ -557,7 +557,7 @@ def schema_to_deployment(s: DeploymentSchema) -> Deployment: health_check_period_s=s.health_check_period_s, health_check_timeout_s=s.health_check_timeout_s, ) - config.user_configured_options = s.get_user_configured_options() + config.user_configured_option_names = s.get_user_configured_option_names() return Deployment( func_or_class="", diff --git a/python/ray/serve/schema.py b/python/ray/serve/schema.py index d26e1cb9b9f6..aa71e426eb00 100644 --- a/python/ray/serve/schema.py +++ b/python/ray/serve/schema.py @@ -222,10 +222,10 @@ def route_prefix_format(cls, v): return v - def get_user_configured_options(self) -> Set[str]: - """Get set of all user-configured options. + def get_user_configured_option_names(self) -> Set[str]: + """Get set of names for all user-configured options. - Any field not set to DEFAULT.VALUE is considered user-configured options. + Any field not set to DEFAULT.VALUE is considered a user-configured option. """ return { diff --git a/python/ray/serve/tests/test_deployment_class.py b/python/ray/serve/tests/test_deployment_class.py index af68c4f8f6c3..0c41d776b1f8 100644 --- a/python/ray/serve/tests/test_deployment_class.py +++ b/python/ray/serve/tests/test_deployment_class.py @@ -97,8 +97,8 @@ class TestDeploymentOptions: deployment_option_combos = get_random_dict_combos(deployment_options, 1000) @pytest.mark.parametrize("options", deployment_option_combos) - def test_user_configured_options(self, options: Dict): - """Check that user_configured_options tracks the correct options. + def test_user_configured_option_names(self, options: Dict): + """Check that user_configured_option_names tracks the correct options. Args: options: Maps deployment option strings (e.g. "name", @@ -110,11 +110,11 @@ def test_user_configured_options(self, options: Dict): def f(): pass - assert f._config.user_configured_options == set(options.keys()) + assert f._config.user_configured_option_names == set(options.keys()) @pytest.mark.parametrize("options", deployment_option_combos) - def test_user_configured_options_schematized(self, options: Dict): - """Check user_configured_options after schematization. + def test_user_configured_option_names_schematized(self, options: Dict): + """Check user_configured_option_names after schematization. Args: options: Maps deployment option strings (e.g. "name", @@ -140,16 +140,16 @@ def f(): # Don't track name in the deschematized deployment since it's optional # in deployment decorator but required in schema, leading to # inconsistent behavior. - if "name" in deschematized_deployment._config.user_configured_options: - deschematized_deployment._config.user_configured_options.remove("name") + if "name" in deschematized_deployment._config.user_configured_option_names: + deschematized_deployment._config.user_configured_option_names.remove("name") - assert deschematized_deployment._config.user_configured_options == set( + assert deschematized_deployment._config.user_configured_option_names == set( options.keys() ) @pytest.mark.parametrize("options", deployment_option_combos) - def test_user_configured_options_serialized(self, options: Dict): - """Check user_configured_options after serialization. + def test_user_configured_option_names_serialized(self, options: Dict): + """Check user_configured_option_names after serialization. Args: options: Maps deployment option strings (e.g. "name", @@ -168,7 +168,7 @@ def f(): serialized_config = f._config.to_proto_bytes() deserialized_config = DeploymentConfig.from_proto_bytes(serialized_config) - assert deserialized_config.user_configured_options == set(options.keys()) + assert deserialized_config.user_configured_option_names == set(options.keys()) @pytest.mark.parametrize( "option", diff --git a/src/ray/protobuf/serve.proto b/src/ray/protobuf/serve.proto index fde3a255e317..f4f912094e93 100644 --- a/src/ray/protobuf/serve.proto +++ b/src/ray/protobuf/serve.proto @@ -89,7 +89,7 @@ message DeploymentConfig { string version = 11; - repeated string user_configured_options = 12; + repeated string user_configured_option_names = 12; } // Deployment language. From f871ef09345de18bcb2dc99748fa1f0934dc72fd Mon Sep 17 00:00:00 2001 From: Shreyas Krishnaswamy Date: Wed, 28 Sep 2022 15:34:45 -0700 Subject: [PATCH 35/37] Add test for options Signed-off-by: Shreyas Krishnaswamy --- python/ray/serve/deployment.py | 2 +- .../ray/serve/tests/test_deployment_class.py | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/python/ray/serve/deployment.py b/python/ray/serve/deployment.py index fadf8c377ccc..a8f74b555ba9 100644 --- a/python/ray/serve/deployment.py +++ b/python/ray/serve/deployment.py @@ -332,7 +332,7 @@ def options( user_configured_option_names = [ option for option, value in locals().items() - if option not in {"self", "_func_or_class", "_internal"} + if option not in {"self", "func_or_class", "_internal"} and value is not DEFAULT.VALUE ] diff --git a/python/ray/serve/tests/test_deployment_class.py b/python/ray/serve/tests/test_deployment_class.py index 0c41d776b1f8..b70119b83aa3 100644 --- a/python/ray/serve/tests/test_deployment_class.py +++ b/python/ray/serve/tests/test_deployment_class.py @@ -199,6 +199,24 @@ def test_nullable_options(self, option: str): def f(): pass + @pytest.mark.parametrize("options", deployment_option_combos) + def test_options(self, options): + """Check that updating options also updates user_configured_options_names.""" + + @serve.deployment + def f(): + pass + + f = f.options(**options) + assert f._config.user_configured_option_names == set(options.keys()) + + @serve.deployment + def g(): + pass + + g.set_options(**options) + assert g._config.user_configured_option_names == set(options.keys()) + if __name__ == "__main__": sys.exit(pytest.main(["-v", "-s", __file__])) From 1bfb3331141e4cfd228228ca56957407d172d310 Mon Sep 17 00:00:00 2001 From: Shreyas Krishnaswamy Date: Thu, 29 Sep 2022 13:08:27 -0700 Subject: [PATCH 36/37] Add note about where user_configured_options should be defined Signed-off-by: Shreyas Krishnaswamy --- python/ray/serve/api.py | 3 +++ python/ray/serve/deployment.py | 3 +++ 2 files changed, 6 insertions(+) diff --git a/python/ray/serve/api.py b/python/ray/serve/api.py index b7223605631b..9ff4285a82e0 100644 --- a/python/ray/serve/api.py +++ b/python/ray/serve/api.py @@ -346,6 +346,9 @@ class constructor when starting up deployment replicas. These can Deployment """ + # NOTE: The user_configured_option_names should be the first thing that's + # defined in this function. It depends on the locals() dictionary storing + # only the function args/kwargs. # Create list of all user-configured options from keyword args user_configured_option_names = [ option diff --git a/python/ray/serve/deployment.py b/python/ray/serve/deployment.py index a8f74b555ba9..be4f1ae08a33 100644 --- a/python/ray/serve/deployment.py +++ b/python/ray/serve/deployment.py @@ -328,6 +328,9 @@ def options( internally by Serve. It should be False when called by users. """ + # NOTE: The user_configured_option_names should be the first thing that's + # defined in this method. It depends on the locals() dictionary storing + # only the function args/kwargs. # Create list of all user-configured options from keyword args user_configured_option_names = [ option From b41a42c49590d9b231aad0ab27f09507612ea072 Mon Sep 17 00:00:00 2001 From: Shreyas Krishnaswamy Date: Thu, 29 Sep 2022 13:16:18 -0700 Subject: [PATCH 37/37] Make test_working_dir_scale_up_in_new_driver use deployment.deploy() Signed-off-by: Shreyas Krishnaswamy --- python/ray/serve/tests/test_runtime_env.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/python/ray/serve/tests/test_runtime_env.py b/python/ray/serve/tests/test_runtime_env.py index 875648ffca19..14c8c5e3552c 100644 --- a/python/ray/serve/tests/test_runtime_env.py +++ b/python/ray/serve/tests/test_runtime_env.py @@ -96,6 +96,8 @@ def __call__(self, *args): run_string_as_driver(driver2) +# NOTE: This test uses deployment.deploy() instead of serve.run() to preserve +# the cached runtime_env that's returned by serve.get_deployment(). @pytest.mark.skipif(sys.platform == "win32", reason="Fail to create temp dir.") def test_working_dir_scale_up_in_new_driver(ray_start, tmp_dir): with open("hello", "w") as f: @@ -109,13 +111,15 @@ def test_working_dir_scale_up_in_new_driver(ray_start, tmp_dir): job_config = ray.job_config.JobConfig(runtime_env={"working_dir": "."}) ray.init(address="auto", namespace="serve", job_config=job_config) +serve.start(detached=True) @serve.deployment(version="1") class Test: def __call__(self, *args): return os.getpid(), open("hello").read() -handle = serve.run(Test.bind()) +Test.deploy() +handle = Test.get_handle() assert ray.get(handle.remote())[1] == "world" """ @@ -130,9 +134,11 @@ def __call__(self, *args): job_config = ray.job_config.JobConfig(runtime_env={"working_dir": "."}) ray.init(address="auto", namespace="serve", job_config=job_config) +serve.start(detached=True) Test = serve.get_deployment("Test") -handle = serve.run(Test.options(num_replicas=2).bind()) +Test.options(num_replicas=2).deploy() +handle = Test.get_handle() results = ray.get([handle.remote() for _ in range(1000)]) print(set(results)) assert all(r[1] == "world" for r in results), (