Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Added test for create-pytorchjob.ipynb python notebook #2274

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open

Added test for create-pytorchjob.ipynb python notebook #2274

wants to merge 18 commits into from
+149 −10

Conversation

saileshd1402
Copy link
Contributor

@saileshd1402 saileshd1402 commented Sep 29, 2024 •
edited
Loading

What this PR does / why we need it:
This PR addresses the issue: #2246

Changes:

  • Added test for examples/pytorch/image-classification/create-pytorchjob.ipynb using papermill
  • Made it extendable so that new notebooks can be added easily
  • New notebook tests can be added to the test-example-notebooks.yaml file/workflow as jobs
  • Notebook tests should have the parameter kubeflow_python_sdk so that CI will install local Python SDK for testing
  • Updated gcr.io/kubeflow-ci/pytorch-dist-mnist-test:v1.0 to kubeflow/pytorch-dist-mnist:latest

Checklist:

  • Docs included if any changes are user facing

@review-notebook-app Review Notebook App
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

@google-oss-prow Google OSS Prow
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign andreyvelich for approval. For more information see the Kubernetes Code Review Process.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@coveralls
Copy link

coveralls commented Sep 29, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 11096923156

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%
Totals Coverage Status
Change from base Build 11001381410: 0.0%
Covered Lines: 66
Relevant Lines: 66
💛 - Coveralls

@saileshd1402
Added test for create-pytorchjob.ipynb
0256146 
Signed-off-by: sailesh duddupudi <[email protected]>
@saileshd1402
fix yaml syntax
222bf0e 
Signed-off-by: sailesh duddupudi <[email protected]>
@saileshd1402
Fix uses path
34be848 
Signed-off-by: sailesh duddupudi <[email protected]>
@saileshd1402
Add actions/checkout
107e5ed 
Signed-off-by: sailesh duddupudi <[email protected]>
@saileshd1402
Add bash to action.yaml
ec0f2d3 
Signed-off-by: sailesh duddupudi <[email protected]>
@saileshd1402
Install pip dependencies step
bb00e73 
Signed-off-by: sailesh duddupudi <[email protected]>
@saileshd1402
Add quotes for args
21c32ee 
Signed-off-by: sailesh duddupudi <[email protected]>
@saileshd1402
Add jupyter
a7a15aa 
Signed-off-by: sailesh duddupudi <[email protected]>
@saileshd1402
Add nbformat_minor: 5 to fix invalid format error
95422f7 
Signed-off-by: sailesh duddupudi <[email protected]>
@saileshd1402
Fix job name
2632690 
Signed-off-by: sailesh duddupudi <[email protected]>
@saileshd1402
test papermill-args-yaml
67a6b83 
Signed-off-by: sailesh duddupudi <[email protected]>
@saileshd1402
testing multi line args
22ceaa4 
Signed-off-by: sailesh duddupudi <[email protected]>
@saileshd1402
testing multi line args1
32a4160 
Signed-off-by: sailesh duddupudi <[email protected]>
@saileshd1402
testing multi line args2
1042ea2 
Signed-off-by: sailesh duddupudi <[email protected]>
@saileshd1402
testing multi line args3
9e78f0e 
Signed-off-by: sailesh duddupudi <[email protected]>
@saileshd1402
Parameterize sdk install
51b3f09 
Signed-off-by: sailesh duddupudi <[email protected]>
@saileshd1402
Remove unnecessary output
7188ccc 
Signed-off-by: sailesh duddupudi <[email protected]>
@saileshd1402
nbformat normailze
50e9ade 
Signed-off-by: sailesh duddupudi <[email protected]>
@saileshd1402 saileshd1402 changed the title [WIP] Added test for create-pytorchjob.ipynb Added test for create-pytorchjob.ipynb Sep 29, 2024
saileshd1402
saileshd1402 commented Sep 30, 2024
View reviewed changes
examples/pytorch/image-classification/create-pytorchjob.ipynb
},
"outputs": [],
"source": [
"kubeflow_python_sdk=\"git+https://github.com/kubeflow/training-operator.git#subdirectory=sdk/python\"\n",
Copy link
Contributor Author

@saileshd1402 saileshd1402 Sep 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For papermill, all the parameters should be in one cell. But this seems to be affecting the readability of the user. How should we improve this?
cc: @andreyvelich @tenzen-y @Electronic-Waste

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@akshaychitneni @shravan-achar Any thoughts on papermill usage ?
We can pass parameters using parameters_yaml only to the single Notebook cell ?

@saileshd1402 saileshd1402 changed the title Added test for create-pytorchjob.ipynb Added test for create-pytorchjob.ipynb python notebook Sep 30, 2024
@andreyvelich andreyvelich mentioned this pull request Oct 11, 2024
Open
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andreyvelich andreyvelich andreyvelich left review comments

@jinchihe jinchihe Awaiting requested review from jinchihe

@kuizhiqing kuizhiqing Awaiting requested review from kuizhiqing

Assignees
No one assigned
Labels
size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@saileshd1402 @coveralls @andreyvelich