[serve] Add support for deploy refactor #34845
Conversation
Signed-off-by: Cindy Zhang <[email protected]>
Signed-off-by: Cindy Zhang <[email protected]>
Signed-off-by: Cindy Zhang <[email protected]>
Signed-off-by: Cindy Zhang <[email protected]>
Signed-off-by: Cindy Zhang <[email protected]>
Signed-off-by: Cindy Zhang <[email protected]>
| app_state_manager.deployment_state_manager.set_deployment_statuses_healthy(0) | ||
| app_state_manager._deployment_state_manager.set_deployment_statuses_healthy(0) |
There was a problem hiding this comment.
ugly that we need to reach into a private field (but not a huge deal)
is there a better way to structure the test?
There was a problem hiding this comment.
Stored the mock deployment state manager into a variable before passing to app state manager and used that instead in the tests - lmk if that looks better
| cls, info: DeploymentInfo, *, deleting: bool = False | ||
| cls, | ||
| info: DeploymentInfo, | ||
| autoscaled_num_replicas: int = None, |
There was a problem hiding this comment.
can you please add a docstring that describes what this field is and how it interacts with the deployment info?
There was a problem hiding this comment.
Per Sihan's suggestion, removed it and placed into deployment info
| def _set_target_state(self, target_info: DeploymentInfo) -> None: | ||
| def _set_target_state( | ||
| self, target_info: DeploymentInfo, autoscaled_num_replicas: int = None | ||
| ) -> None: |
There was a problem hiding this comment.
can you write comment what does it mean when autoscaled_num_replicas is None?
There was a problem hiding this comment.
nit: targeted_num_replicas.
There was a problem hiding this comment.
I prefer having autoscale in the name to make it clear it's only used for autoscaling. Also moved the variable into deployment info and added comments.
Signed-off-by: Cindy Zhang <[email protected]>
Signed-off-by: Cindy Zhang <[email protected]>
Signed-off-by: Cindy Zhang <[email protected]>
Signed-off-by: Cindy Zhang <[email protected]>
Signed-off-by: Cindy Zhang <[email protected]>
Signed-off-by: Cindy Zhang <[email protected]>
Signed-off-by: Cindy Zhang <[email protected]>
|
@edoakes Addressed comments, PTAL |
|
serving doc test timed out repeatedly |
Signed-off-by: Cindy Zhang <[email protected]>
|
@edoakes tests look good now. The current failures are failing on master and unrelated:
|
Some preparation for upcoming deploy refactor PRs. - Rename fields of `ApplicationState` as private fields - Add route prefix to deployment info (it will be used by application state to deploy deployments in a reconciler loop) - Stop setting num replicas from autoscaling in `deployment_config.num_replicas`, instead put it in a separate field of deployment info. - determine initial autoscaled num replicas in deployment state manager .deploy()
Why are these changes needed?
Some preparation for upcoming deploy refactor PRs.
ApplicationStateas private fieldsdeployment_config.num_replicas, instead put it in a separate field of deployment info.Related issue number
Checks
git commit -s) in this PR.scripts/format.shto lint the changes in this PR.method in Tune, I've added it in
doc/source/tune/api/under thecorresponding
.rstfile.