[Serve] Refactor Autoscaler for better customizability. #4008
Conversation
|
bump for review @Michaelvll |
There was a problem hiding this comment.
Thanks for refactoring the autoscaler class @cblmemo! Left several comments.
sky/serve/autoscalers.py
Outdated
|
|
||
| # Case 2. when latest_nonterminal_replicas is less | ||
| class HysteresisAutoscaler(Autoscaler): |
There was a problem hiding this comment.
It is a bit weird calling it HystersisAutoscaler, as Hysteresis is just a technique to avoid keeping scaling up and down.
Also, in what case would this autoscaler ever scale? Is it just when a user updates the number of desired replicas? Or, is this just an internal utility class?
If it is a utility class, it would be good to make it clear in the comment and hide the class by pretending _ to the name.
There was a problem hiding this comment.
Good point! It is indeed an utility class. Changed name to _AutoscalerWithHysteresis. I also change the format from xxxAutoscaler to AutoscalerWithxxx. PTAL!
| def collect_request_information( | ||
| self, request_aggregator_info: Dict[str, Any]) -> None: | ||
| """Collect request information from aggregator for autoscaling.""" | ||
| raise NotImplementedError |
There was a problem hiding this comment.
For all the APIs a custom autoscaler needs to implement, we should keep them in the same section. We can try tot separate the utilities and APIs using:
# --- APIs for custom autoscaler ---
def api_1():
pass
def api_2():
pass
# --- Utilities functions ---
def ...There was a problem hiding this comment.
Good point! Done. PTAL again!
|
@Michaelvll bump on review for this as it is related to #4444 |
This PR refactored the inheritance relation of the Autoscalers we have, to minimize the number of functions users needs to override in a customize autoscaler.
TODO: Run all smoke tests.
Tested (run the relevant ones):
bash format.shpytest tests/test_smoke.pypytest tests/test_smoke.py::test_fill_in_the_nameconda deactivate; bash -i tests/backward_compatibility_tests.sh