-
Notifications
You must be signed in to change notification settings - Fork 875
trainer: Adding local mode guides #4221
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: master
Are you sure you want to change the base?
Conversation
|
Hi @Fiona-Waters. Thanks for your PR. I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
🚫 This command cannot be processed. Only organization members or owners can use the commands. |
dadef0b to
cba13a5
Compare
Arhell
left a comment
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.
/ok-to-test
|
Thanks @Fiona-Waters awesome work, very useful! /lgtm |
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.
@Fiona-Waters Thanks for this awesome work, I will take a look soon!
Shall we merge it once the Container PR is complete: kubeflow/sdk#119?
/hold
@andreyvelich yes this can be merged after the Container PR. I will have a PR in trainer repo for examples soon too, which can also be merged after. Thanks |
cba13a5 to
02fdc04
Compare
02fdc04 to
62b2611
Compare
Signed-off-by: Fiona Waters <[email protected]> Co-authored-by: Saad Zaher <[email protected]>
62b2611 to
b20b122
Compare
|
Thanks @Fiona-Waters! /lgtm |
andreyvelich
left a comment
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!
Overall looks great, I would move common sections to the overview page to reduce duplicated content.
cc @kubeflow/kubeflow-sdk-team @kubeflow/kubeflow-trainer-team
content/en/docs/components/trainer/user-guides/local-execution-mode/_index.md
Outdated
Show resolved
Hide resolved
content/en/docs/components/trainer/user-guides/local-execution-mode/_index.md
Outdated
Show resolved
Hide resolved
content/en/docs/components/trainer/user-guides/local-execution-mode/docker.md
Outdated
Show resolved
Hide resolved
content/en/docs/components/trainer/user-guides/local-execution-mode/docker.md
Outdated
Show resolved
Hide resolved
content/en/docs/components/trainer/user-guides/local-execution-mode/docker.md
Outdated
Show resolved
Hide resolved
content/en/docs/components/trainer/user-guides/local-execution-mode/local_process.md
Outdated
Show resolved
Hide resolved
| rm -rf /tmp/a1b2c3d4e5f_xyz/ | ||
| ``` | ||
|
|
||
| ## Architecture |
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.
We might need to re-draw it as a diagram in https://www.drawio.com/
we can do it later.
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.
I think I'll remove this section for now and create a follow on issue to create and add a proper diagram. wdyt?
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.
Sure, thank you!
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.
content/en/docs/components/trainer/user-guides/local-execution-mode/local_process.md
Outdated
Show resolved
Hide resolved
content/en/docs/components/trainer/user-guides/local-execution-mode/local_process.md
Outdated
Show resolved
Hide resolved
content/en/docs/components/trainer/user-guides/local-execution-mode/podman.md
Outdated
Show resolved
Hide resolved
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 |
|
@andreyvelich thanks for the review - I've addressed your feedback, please take a look. Thanks! |
andreyvelich
left a comment
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.
Thanks for this great effort @Fiona-Waters!
/lgtm
/assign @kramaranya @szaher @astefanutti
| from kubeflow.trainer import CustomTrainer, TrainerClient | ||
| from kubeflow.trainer.backends.container.types import ContainerBackendConfig |
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.
| from kubeflow.trainer import CustomTrainer, TrainerClient | |
| from kubeflow.trainer.backends.container.types import ContainerBackendConfig | |
| from kubeflow.trainer import CustomTrainer, TrainerClient, ContainerBackendConfig |
| - **Python 3.9+** | ||
| - **Kubeflow SDK**: Install with Docker support: | ||
| ```bash | ||
| pip install kubeflow[docker] |
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.
| pip install kubeflow[docker] | |
| pip install "kubeflow[docker]" |
To avoid some issues with shell expansion like with ZSH.
| from kubeflow.trainer import CustomTrainer, TrainerClient | ||
| from kubeflow.trainer.backends.container.types import ContainerBackendConfig |
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.
| from kubeflow.trainer import CustomTrainer, TrainerClient | |
| from kubeflow.trainer.backends.container.types import ContainerBackendConfig | |
| from kubeflow.trainer import CustomTrainer, TrainerClient, ContainerBackendConfig |
| from kubeflow.trainer import CustomTrainer, TrainerClient | ||
| from kubeflow.trainer.backends.localprocess import LocalProcessBackendConfig |
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.
| from kubeflow.trainer import CustomTrainer, TrainerClient | |
| from kubeflow.trainer.backends.localprocess import LocalProcessBackendConfig | |
| from kubeflow.trainer import CustomTrainer, TrainerClient, LocalProcessBackendConfig |
| **Example:** | ||
|
|
||
| ```python | ||
| from kubeflow.trainer.backends.localprocess import LocalProcessBackendConfig |
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.
| from kubeflow.trainer.backends.localprocess import LocalProcessBackendConfig | |
| from kubeflow.trainer import LocalProcessBackendConfig |
| - **Python 3.9+** | ||
| - **Kubeflow SDK**: Install with Podman support: | ||
| ```bash | ||
| pip install kubeflow[podman] |
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.
| pip install kubeflow[podman] | |
| pip install "kubeflow[podman]" |
|
|
||
| ```bash | ||
| # Install Podman | ||
| brew install podman |
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.
Maybe we want follow the official recommendation:
Though not recommended, Podman can also be obtained through Homebrew
Also maybe linking to the official installation instructions would be simpler.
| from kubeflow.trainer import CustomTrainer, TrainerClient | ||
| from kubeflow.trainer.backends.container.types import ContainerBackendConfig |
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.
| from kubeflow.trainer import CustomTrainer, TrainerClient | |
| from kubeflow.trainer.backends.container.types import ContainerBackendConfig | |
| from kubeflow.trainer import CustomTrainer, TrainerClient, ContainerBackendConfig |
| from kubeflow.trainer import CustomTrainer, TrainerClient | ||
| from kubeflow.trainer.backends.container.types import ContainerBackendConfig |
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.
| from kubeflow.trainer import CustomTrainer, TrainerClient | |
| from kubeflow.trainer.backends.container.types import ContainerBackendConfig | |
| from kubeflow.trainer import CustomTrainer, TrainerClient, ContainerBackendConfig |
| ```python | ||
| backend_config = ContainerBackendConfig( | ||
| container_runtime="podman", | ||
| auto_remove=False # Containers remain after job completion | ||
| ) |
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.
"```" is missing
Description of Changes
Added 3 user-guides for local process, docker and podman local mode backends. This PR should not be merged until after kubeflow/sdk#119 is merged.
Related Issues
Closes: #
Related: # kubeflow/sdk#95 (PR) The details in this PR have been included in this PR as per this comment.
Checklist