[serve] Move DeploymentConfig out of UserCallableWrapper
#42039
Conversation
Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>
…-constructor-to-wrapper
…-constructor-to-wrapper
…-constructor-to-wrapper
…-constructor-to-wrapper
…-constructor-to-wrapper
| if logging_config_changed: | ||
| self._configure_logger_and_profilers(deployment_config.logging_config) |
There was a problem hiding this comment.
@sihanwang41 this is actually fixing a latent bug -- when I updated the replica log format, it was not updated in the logging_config update path because they were completely disjoint. This is an example of why on the original PR I asked if we could unify where we're setting logging config everywhere if possible.
There was a problem hiding this comment.
Should we add a test for this?
There was a problem hiding this comment.
Yes let me see what I can do. Probably just need to assert something about the log format in the existing tests.
There was a problem hiding this comment.
Ok I've added assertions to test_deploy_app's logging test that cover this.
| if logging_config_changed: | ||
| self._configure_logger_and_profilers(deployment_config.logging_config) |
There was a problem hiding this comment.
Should we add a test for this?
| @@ -371,17 +372,36 @@ async def reconfigure( | |||
| deployment_config: DeploymentConfig, | |||
| ) -> Tuple[DeploymentConfig, DeploymentVersion]: | |||
| try: | |||
| await self._user_callable_wrapper.reconfigure(deployment_config) | |||
| return await self._get_metadata() | |||
| user_config_changed = ( | |||
There was a problem hiding this comment.
Since UserCallableWrapper depends on autoscaling config, should reconfigure() also update autoscaling config?
There was a problem hiding this comment.
in theory it should; let me handle this in a separate PR where I am separating out the autoscaling metrics handling: #42043
Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>
…ject#42039) Continuation of: ray-project#42017. UserCallableWrapper no longer depends on deployment_config at all. --------- Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>
Why are these changes needed?
Continuation of: #42017.
UserCallableWrapperno longer depends ondeployment_configat all.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.