-
Notifications
You must be signed in to change notification settings - Fork 38
feat: Add ContainerBackend with Docker and Podman #119
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Brian Gallagher <[email protected]>
Signed-off-by: Fiona Waters <[email protected]>
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this @Fiona-Waters!
As we discussed here: #111 (comment), can we consolidate Podman and Docker under single container backend ?
Given that those backend should have similar APIs, I think it would be better to consolidate them, similar to KFP: https://www.kubeflow.org/docs/components/pipelines/user-guides/core-functions/execute-kfp-pipelines-locally/#runner-dockerrunner
Thanks @andreyvelich I will look at updating the implementation. |
@andreyvelich @astefanutti regarding comments on this PR and #111 this is what I propose: We have 3 backends:
For the Local Container backend we automatically try Docker first, then Podman and then fallback to Subprocess if neither runtime is available. We use the adapter pattern with a container client adapter unified interface, and docker and podman specific calls are implemented in separate adapter classes. |
Sure, that looks great @Fiona-Waters!
Why do we need to fallback to subprocess ?
In the ContainerBackend users can select:
|
@Fiona-Waters that sounds to me. I agree the fallback logic may really apply to choose the default container runtime. Other than that, I'd be inclined to drop the "Local" prefix entirely. Even Kubernetes could run local with KinD, and I doubt the SDK will ever do remote process. |
Understood. Let me see what I can do. Thank you for the swift reply! |
Ok cool. Let me see what I can do. Thank you! |
1f7c066
to
bdde877
Compare
Signed-off-by: Fiona Waters <[email protected]>
bdde877
to
d1288b2
Compare
@andreyvelich @astefanutti @briangallagher |
What this PR does / why we need it:
This PR introduces a unified ContainerBackend that automatically detects and uses either Docker or Podman for local training execution. This replaces the previous separate LocalDockerBackend and LocalPodmanBackend implementations with a single, cleaner abstraction. You can see the Docker and Podman implementations in separate commits.
This implementation tries Docker first, then falls back to Podman if Docker is unavailable. This can be overridden via ContainerBackendConfig.runtime to force a specific runtime ("docker" or "podman"). An error is raised if neither runtime is available.
Unit tests for the backend implementation have also been added. Examples for using Docker and Podman will be added to the Trainer repo later.
Manually testing on Mac I had to specify the container_host like so:
Docker via Colima
container_host=f"unix://{os.path.expanduser('~')}/.colima/default/docker.sock"
Podman Desktop
container_host=f"unix://{os.path.expanduser('~')}/.local/share/containers/podman/machine/podman.sock"
Which issue(s) this PR fixes (optional, in
Fixes #<issue number>, #<issue number>, ...
format, will close the issue(s) when PR gets merged):Fixes ##114 and #108
Checklist:
I need to look at adding docs. A README has been included.