ray icon indicating copy to clipboard operation
ray copied to clipboard

[runtime_env] unify `container` and (new) `image_uri` under plugin mechanism

Open zcin opened this issue 1 year ago • 3 comments

[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_path from the old API.

Removal of run_options:

  • We are intentionally removing the option to specify custom run_options because 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]

zcin avatar May 06 '24 16:05 zcin

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.

rynewang avatar May 22 '24 20:05 rynewang

cc: @jjyao also for API changes.

zcin avatar May 22 '24 22:05 zcin

@rynewang Could you review this when you get a chance?

zcin avatar May 24 '24 16:05 zcin

@rynewang Addressed comments, could you take another look?

zcin avatar May 28 '24 16:05 zcin

@jjyao Can you help merge this PR?

zcin avatar May 28 '24 21:05 zcin