Fixed the YAML schema validation for replicas
#1055
Conversation
There was a problem hiding this comment.
Apparently, to avoid the type checking and code duplication issues (see comments below), we manipulate the JSON schema directly in other places where Range is used, see ResourcesSpec. I'd suggest we try the same thing here with replicas. Or we can merge as is and come back to it later
| replicas: Annotated[Range[int], Field(description="The range ")] = Range[int](min=1, max=1) | ||
| replicas: Annotated[ | ||
| Union[conint(ge=1), constr(regex=r"^[0-9]+..[1-9][0-9]*$"), Range[int]], | ||
| Field(description="The range "), |
There was a problem hiding this comment.
(off-topic) Not very descriptive
| raise ValueError("The minimum number of replicas must be greater than or equal to 0") | ||
| if v.max < v.min: | ||
| raise ValueError( | ||
| "The maximum number of replicas must be greater than the minium number of replicas" |
There was a problem hiding this comment.
(nit) Technically, greater than or equal
| @@ -233,7 +233,10 @@ class ServiceConfiguration(BaseConfiguration): | |||
| Field(description="Mapping of the model for the OpenAI-compatible endpoint"), | |||
| ] = None | |||
| auth: Annotated[bool, Field(description="Enable the authorization")] = True | |||
| replicas: Annotated[Range[int], Field(description="The range ")] = Range[int](min=1, max=1) | |||
| replicas: Annotated[ | |||
| Union[conint(ge=1), constr(regex=r"^[0-9]+..[1-9][0-9]*$"), Range[int]], | |||
| @@ -247,25 +250,30 @@ def convert_port(cls, v) -> PortMapping: | |||
| return v | |||
|
|
|||
| @validator("replicas") | |||
| def convert_replicas(cls, v: Range[int]) -> Range[int]: | |||
| def convert_replicas(cls, v: Any) -> Range[int]: | |||
| if isinstance(v, str) and ".." in v: | |||
There was a problem hiding this comment.
It's not perfect that we duplicate range parsing logic. It is already implemented in Range
Yeah, I was also thinking of it. The problem is we have a mess there already. Look at |
|
@jvstme I've added the test. Let me know if it's OK to merge |
| ).replicas == Range(min=0, max=10) | ||
| with pytest.raises( | ||
| ConfigurationError, | ||
| match="When you set `replicas` to a range, ensure to specify `scaling`", |
There was a problem hiding this comment.
(nit) I think the primary error here is unexpected value; permitted: 'rps'. The ensure to specify scaling error also appears, but as a consequence of the primary error
|
@peterschmidt85, thank you! I think it's ok to merge |
Previously, the YAML schema showed errors if
replicaswas set to astrorint