-
Notifications
You must be signed in to change notification settings - Fork 836
feat: Adding local execution example notebook #2907
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
feat: Adding local execution example notebook #2907
Conversation
Signed-off-by: Brian Gallagher <[email protected]>
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
809ef62 to
b62f2d4
Compare
|
Thanks @Fiona-Waters this is awesome! /lgtm |
kramaranya
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, @Fiona-Waters!
Could you please run this notebook and keep the output of the cells?
| "# %pip install -U kubeflow[docker] # For Docker\n", | ||
| "# %pip install -U kubeflow[podman] # For 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.
Why not using ! as in other examples?
| "# %pip install -U kubeflow[docker] # For Docker\n", | |
| "# %pip install -U kubeflow[podman] # For Podman" | |
| "# !pip install -U kubeflow[docker] # For Docker\n", | |
| "# !pip install -U kubeflow[podman] # For 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.
b62f2d4 to
e0010b0
Compare
In my humble opinion, I think that including the output of the cells creates quite a cluttered example and it's not necessary - but I am open to other opinions here. @astefanutti @andreyvelich |
Yes, IMHO that's a matter of compromise and finding the right balance. I agree outputting entire log streams can clutter the examples, but when kept concise those outputs can add useful info when the notebooks are also used as documentation (not executed). |
Agreed, will update and try to find the right balance. |
I'd suggest aligning with other examples, for example https://github.com/kubeflow/trainer/blob/master/examples/pytorch/image-classification/mnist.ipynb |
Pull Request Test Coverage Report for Build 19111473416Details
💛 - Coveralls |
7908133 to
03b8791
Compare
03b8791 to
a644654
Compare
|
@andreyvelich @kramaranya @astefanutti I've updated the PR to include 2 notebooks - one for local process and one for container backend - both single node. I also updated the image-classification example as Andrey requested. |
|
/ok-to-test |
Co-authored-by Brian Gallagher <[email protected]> Signed-off-by: Fiona Waters <[email protected]>
a644654 to
c4e5015
Compare
|
/rerun |
|
Tests are failing because the options PR has been merged in the sdk and the container backend doesn't include the param. Need to drop off for a while but will try to update the sdk later this evening and then we can re-run - unless someone else has time to do it before me. @andreyvelich |
|
/retest |
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.
Looks to be working 🎉
Feel free to unhold when you are ready.
It would be also good to add those Notebooks to the SDK E2Es: https://github.com/kubeflow/sdk/blob/main/.github/workflows/test-e2e.yaml#L57-L65
/lgtm
/approve
/hold
/cc @astefanutti @kramaranya
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andreyvelich The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Thank you, @Fiona-Waters! |
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.
/hold cancel
|
/cherry-pick release-2.1 |
|
@andreyvelich: new pull request created: #2924 In response to this:
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. |
What this PR does / why we need it:
This PR adds 2 example notebooks, one for local process and one for container backend with podman/docker - both of them running single node training.
I have also updated the existing image-classification example to use the local trainer client.
I've added the 2 new notebooks to the e2e tests (not tested)
Also I have included cell output but in local-training-mnist.ipynb I think that the logs are too long to include and should be removed.
This PR is related.
Which issue(s) this PR fixes (optional, in
Fixes #<issue number>, #<issue number>, ...format, will close the issue(s) when PR gets merged):Fixes ##2868
Checklist: