[serve] Update version if import_path or runtime_env in config is changed #27498
Conversation
Signed-off-by: Cindy Zhang <[email protected]>
python/ray/serve/controller.py
Outdated
| updated_version_dict = _generate_new_version_config( | ||
| config_dict, last_config_dict, last_version_dict | ||
| ) | ||
| if last_config_dict.get("import_path") == config_dict.get( |
There was a problem hiding this comment.
Could we add a comment somewhere here explaining that if the graph import_path or runtime_env are changed, Serve considers that a code change?
There was a problem hiding this comment.
Checking this pr, it makes me feel the _generate_new_version_config function name is confusing. @shrekris-anyscale @zcin I thought it is always generate a new version of config... can we try to rename the function? And I feel we can hide these logics into the function _generate_new_version_config directly. WDYT?
There was a problem hiding this comment.
That's a good point– _generate_new_version_config makes it sound like we're creating a new version for the whole config. In reality, it generates new versions for each of the deployments.
I think we should rename the function to something like _generate_deployment_config_versions and keep the functionality as is, or we should rename it to _version_config and take in two configs. Since the next release is soon, we should probably pick whichever one gives us more confidence in being well-tested. @sihanwang41 @zcin what do you think?
There was a problem hiding this comment.
yeah, let use _generate_deployment_config_versions first.
I feel it worth refactoring remove the logics inside the deploy_app level, it is a little burden in deploy_app level to "know" when should i pass config_dict, last_config_dict, last_version_dict when should I pass config_dict, {}, {} since the _generate_new_version_config is to determines whether each deployment's version should be changed based on the updated options (from the function description). deploy_app doesn't need to know the trick here :)
But if too much for 2.0 release, we can figure out later.
There was a problem hiding this comment.
The current functionality also takes in two configs, right?
Perhaps we can rename to _update_deployment_config_versions?
@sihanwang41 Do you mean to hide all the logic, including the loading from kv_store, or just the new logic that checks import_path and runtime_env?
There was a problem hiding this comment.
_generate_new_version_config's current functionality takes in two dictionaries containing only the deployment configs. It doesn't take in two full configs that match the ServeApplicationSchema format (including the top-level import_path and runtime_env).
I think @sihanwang41 was saying we could pass in entire configs and do the top-level import_path and runtime_env handling in _generate_new_version_config. That way, we wouldn't have to pass in just the deployment options into the helper function.
There was a problem hiding this comment.
the goal is to hide burden for deploy_app to know whether i should pass last_config_dict or {}. we can keep the interface, but the logic in this pr can be hidden.
When checkpoint is None, we can directly pass _generate_new_version_config(config_dict) and set last_deployed_config and last_deployed_versions default value.
There was a problem hiding this comment.
Actually, I think currently _generate_new_version_config takes in full configs that match the ServeApplicationSchema format (albeit in dictionary form). Hiding the logic into the function _generate_new_version_config makes a lot of sense, though! I think I can make that change easily.
Signed-off-by: Cindy Zhang <[email protected]>
Signed-off-by: Cindy Zhang <[email protected]>
Signed-off-by: Cindy Zhang <[email protected]>
…nged (ray-project#27498) Previous PR that adds in lightweight config updates: ray-project#27000. It only tracks the config options for `deployments` (bumps version if certain deployment options are changed, but otherwise keeps versions the same). However we should bump the versions of all deployments if `import_path` or `runtime_env` is changed. Signed-off-by: Cindy Zhang <[email protected]>
…nged (ray-project#27498) Previous PR that adds in lightweight config updates: ray-project#27000. It only tracks the config options for `deployments` (bumps version if certain deployment options are changed, but otherwise keeps versions the same). However we should bump the versions of all deployments if `import_path` or `runtime_env` is changed. Signed-off-by: Stefan van der Kleij <[email protected]>
Signed-off-by: Cindy Zhang [email protected]
Why are these changes needed?
Previous PR that adds in lightweight config updates: #27000. It only tracks the config options for
deployments(bumps version if certain deployment options are changed, but otherwise keeps versions the same). However we should bump the versions of all deployments ifimport_pathorruntime_envis changed.Related issue number
Checks
git commit -s) in this PR.scripts/format.shto lint the changes in this PR.