[runtime_env] unify `container` and (new) `image_uri` under plugin mechanism
[runtime_env] unify container and (new) image_uri under plugin mechanism
Currently, we create a custom ContainerManager class and invoke it specially with container_manager.setup(). This doesn't conform with the runtime env plugin pattern we use for every other runtime env field.
This PR removes ContainerManager and moves the container code to ContainerPlugin which extends the base RuntimeEnvPlugin class. It also introduces the ImageURIPlugin which is the same implementation, but uses a different, new API.
ContainerPlugin API:
runtime_env:
container:
image: ...
worker_path: ...
run_options: ...
ImageURIPlugin API:
runtime_env:
image_uri: ...
Removal of worker_path:
- In a followup PR, I will add the support for automatically inferring the worker path in both plugins. I will then also remove
worker_pathfrom the old API.
Removal of run_options:
- We are intentionally removing the option to specify custom
run_optionsbecause it is too freeform; instead, we should explicitly add support for specific options as the need arises or as users submit feature requests.
Signed-off-by: Cindy Zhang [email protected]
The name image_uri, and its arguments being a plain string is IMO too specific. If later we have some more options, for example some CPU and mem limits, the name will be awkward.
Also, it seems we are just trying to restrict container for a few fields. Is it possible we just use container name and fire warnings (and later errors) on fields worker_path (if != auto detected path) and run_options (if any no-longer-allowed options)? This way, if a user only used image they can live on.
I will review the implementation first, since either UI we choose, the impl will be the same.
cc: @jjyao also for API changes.
@rynewang Could you review this when you get a chance?
@rynewang Addressed comments, could you take another look?
@jjyao Can you help merge this PR?