Fix Runner service defaults#474
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the Runner API to automatically derive service statefulness and autoscaling behavior based on the type of the execution object, removing the explicit stateful parameter. Functions, builtins, and classes are now strictly stateless and default to autoscaling, while object instances are stateful, fixed-size, and restrict warmup values to 0 or 1. The feedback highlights a discrepancy in how function-like objects are identified between RunnerService and RunnerContext (which would break support for functools.partial), and recommends adding validation for non-negative warmup values along with corresponding unit tests.
| is_function = inspect.isroutine(self.execution_object) | ||
| is_class = inspect.isclass(self.execution_object) | ||
| is_object = not is_function and not is_class |
There was a problem hiding this comment.
There is a discrepancy between how RunnerService (in runner.py) and RunnerContext (in types.py) identify function-like execution objects.
In runner.py (line 223), an execution object is treated as a function if it is callable and not a class:
callable(self._execution_object) and not inspect.isclass(self._execution_object)However, in types.py (line 99), RunnerContext uses inspect.isroutine:
is_function = inspect.isroutine(self.execution_object)Because inspect.isroutine returns False for callable objects like functools.partial or custom functors, RunnerContext will incorrectly classify them as object instances (is_object = True). This forces them to be stateful and fixed-size (autoscale = False), whereas RunnerService will wrap them as functions.
To resolve this discrepancy and support common callable wrappers like functools.partial, RunnerContext should use the same callable check as RunnerService.
| is_function = inspect.isroutine(self.execution_object) | |
| is_class = inspect.isclass(self.execution_object) | |
| is_object = not is_function and not is_class | |
| is_function = callable(self.execution_object) and not inspect.isclass(self.execution_object) | |
| is_class = inspect.isclass(self.execution_object) | |
| is_object = not is_function and not is_class |
| def __post_init__(self) -> None: | ||
| """Compute min/max instances and validate configuration.""" |
There was a problem hiding this comment.
Negative values for warmup are currently not validated and are silently treated as 0 (which defaults min_instances to 0 or 1). We should explicitly validate that warmup is a non-negative integer to prevent invalid configurations.
| def __post_init__(self) -> None: | |
| """Compute min/max instances and validate configuration.""" | |
| def __post_init__(self) -> None: | |
| """Compute min/max instances and validate configuration.""" | |
| if self.warmup < 0: | |
| raise ValueError("warmup must be a non-negative integer.") |
| contexts = [ | ||
| RunnerContext(sample_func), | ||
| RunnerContext(len), | ||
| RunnerContext(SampleService), | ||
| RunnerContext(SampleService, autoscale=False, warmup=2), | ||
| RunnerContext(SampleService()), | ||
| RunnerContext(SampleService(), warmup=1), | ||
| ] | ||
|
|
||
| assert [(ctx.stateful, ctx.autoscale, ctx.warmup, ctx.min_instances, ctx.max_instances) for ctx in contexts] == [ | ||
| (False, True, 0, 0, None), | ||
| (False, True, 0, 0, None), | ||
| (False, True, 0, 0, None), | ||
| (False, False, 2, 2, 2), | ||
| (True, False, 0, 1, 1), | ||
| (True, False, 1, 1, 1), | ||
| ] |
There was a problem hiding this comment.
Add a test case for functools.partial to verify that callable wrappers are correctly identified as stateless, autoscaling function-like services.
| contexts = [ | |
| RunnerContext(sample_func), | |
| RunnerContext(len), | |
| RunnerContext(SampleService), | |
| RunnerContext(SampleService, autoscale=False, warmup=2), | |
| RunnerContext(SampleService()), | |
| RunnerContext(SampleService(), warmup=1), | |
| ] | |
| assert [(ctx.stateful, ctx.autoscale, ctx.warmup, ctx.min_instances, ctx.max_instances) for ctx in contexts] == [ | |
| (False, True, 0, 0, None), | |
| (False, True, 0, 0, None), | |
| (False, True, 0, 0, None), | |
| (False, False, 2, 2, 2), | |
| (True, False, 0, 1, 1), | |
| (True, False, 1, 1, 1), | |
| ] | |
| import functools | |
| contexts = [ | |
| RunnerContext(sample_func), | |
| RunnerContext(functools.partial(sample_func, x=1)), | |
| RunnerContext(len), | |
| RunnerContext(SampleService), | |
| RunnerContext(SampleService, autoscale=False, warmup=2), | |
| RunnerContext(SampleService()), | |
| RunnerContext(SampleService(), warmup=1), | |
| ] | |
| assert [(ctx.stateful, ctx.autoscale, ctx.warmup, ctx.min_instances, ctx.max_instances) for ctx in contexts] == [ | |
| (False, True, 0, 0, None), | |
| (False, True, 0, 0, None), | |
| (False, True, 0, 0, None), | |
| (False, True, 0, 0, None), | |
| (False, False, 2, 2, 2), | |
| (True, False, 0, 1, 1), | |
| (True, False, 1, 1, 1), | |
| ] |
| @pytest.mark.parametrize( | ||
| ("execution_object", "kwargs", "message"), | ||
| [ | ||
| (lambda: None, {"stateful": True}, "always stateless"), | ||
| (str, {"stateful": True}, "always stateless"), | ||
| (object(), {"stateful": False}, "always stateful"), | ||
| (object(), {"autoscale": True}, "always fixed"), | ||
| (object(), {"warmup": 2}, "only support warmup=0 or warmup=1"), | ||
| ], | ||
| ) |
There was a problem hiding this comment.
Add a test case to verify that negative warmup values are rejected with a ValueError.
@pytest.mark.parametrize(
("execution_object", "kwargs", "message"),
[
(lambda: None, {"stateful": True}, "always stateless"),
(str, {"stateful": True}, "always stateless"),
(object(), {"stateful": False}, "always stateful"),
(object(), {"autoscale": True}, "always fixed"),
(object(), {"warmup": 2}, "only support warmup=0 or warmup=1"),
(lambda: None, {"warmup": -1}, "warmup must be a non-negative integer"),
],
)
Summary
Verification