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

saileshd1402 · 2024-09-29T21:20:44Z

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 · 2024-09-29T21:20:48Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

coveralls · 2024-09-29T21:25:29Z

Pull Request Test Coverage Report for Build 12236369721

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

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
Change from base Build 12088918617:	0.0%
Covered Lines:	77
Relevant Lines:	77

💛 - Coveralls

saileshd1402 · 2024-09-30T00:03:12Z

examples/pytorch/image-classification/create-pytorchjob.ipynb

+   },
+   "outputs": [],
+   "source": [
+    "kubeflow_python_sdk=\"git+https://github.com/kubeflow/training-operator.git#subdirectory=sdk/python\"\n",


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

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

@andreyvelich We can pass parameters to multiple Notebook cells. But they should have metadata parameters declared and only have parameter definition statements in case of miss injections:)

Electronic-Waste

Basically LGTM. Thanks for your great contributions! @saileshd1402

I will leave it for @kubeflow/wg-training-leads.

/lgtm

Electronic-Waste · 2024-11-16T07:49:24Z

examples/pytorch/image-classification/create-pytorchjob.ipynb

+   },
+   "outputs": [],
+   "source": [
+    "kubeflow_python_sdk=\"git+https://github.com/kubeflow/training-operator.git#subdirectory=sdk/python\"\n",


@andreyvelich We can pass parameters to multiple Notebook cells. But they should have metadata parameters declared and only have parameter definition statements in case of miss injections:)

andreyvelich

Thank you for this effort @saileshd1402 and sorry for the late reply!
Can you update the Notebook so it can be rendered in the GitHub view ?

andreyvelich · 2024-11-26T19:42:04Z

.github/workflows/template-notebook-test/action.yaml

+    - name: Run Jupyter Notebook with Papermill
+      shell: bash
+      run: |
+        papermill ${{ inputs.notebook-input }} ${{ inputs.notebook-output }} -p kubeflow_python_sdk "./sdk/python" --parameters_yaml "${{ inputs.papermill-args-yaml }}"


Should we create a bash script that execute our Notebooks ? In that case, you don't need to have this template action, but rather just have another GitHub action that runs this bash script.
I think that would be useful for folks, who want to tests it locally in their environment.
I was thinking that for V2, we can put similar script to execute example Notebooks under /test/e2e/notebooks.

WDYT @saileshd1402 @Electronic-Waste @kubeflow/wg-training-leads @saileshd1402 @seanlaii @varshaprasad96?

cc @akshaychitneni

I wanted to check which steps we should run in the bash script and which ones in github actions. For example: creating kind cluster, building, deploying training operator, etc is specific to CI. I think it would be a good idea to keep all the setup steps in github actions itself and the script can install papermill and run the notebooks.

We can also make a "Setup E2E" github action template with steps from integration-tests.yaml so that we can use it for notebooks as well and other e2e tests in the future

@saileshd1402 I think, what @andreyvelich meant is that we can replace the papermill <arg1> <arg2> command in "Run Jupyter Notebook with Papermill" step with a bash script like ./test-notebook.sh <arg1> <arg2>. In this way, users can run this script directly with just a few necessary args passed into the script.

Okay yes, just wanted to make sure. thanks!

creating kind cluster, building, deploying training operator, etc is specific to CI. I think it would be a good idea to keep all the setup steps in github actions itself and the script can install papermill and run the notebooks.

Yes, I agree with you @saileshd1402. Let's use the GitHub action to configure cluster and deploy Training Operator.
We should re-use the same template as we use for our e2e tests as you said.

Electronic-Waste

Basically LGTM! I left some initial comments for you! @saileshd1402

Electronic-Waste · 2024-12-02T13:39:32Z

scripts/run-notebook.sh

+# Install Python dependencies to run Jupyter Notebooks with papermill
+pip install jupyter ipykernel papermill==2.6.0


I guess, it might be better if we could move these dependencies to test-example-notebooks.yaml because users might have installed these dependencies and it would be better if we could maintain the version of these packages in CI. WDYT @saileshd1402 @kubeflow/wg-training-leads ?

Makes sense, I'll pin the versions. And set requirements as arguments

I think maybe installing them in the test-example-notebooks.yaml is better, since we assume that users already have jupyter installed if they want to run the example. As for the papermill, I think we can notify them of the dependency on papermill with README file in the future, just FYR.

Signed-off-by: sailesh duddupudi <[email protected]>

andreyvelich

Thanks for the update @saileshd1402!

andreyvelich · 2024-12-02T15:11:57Z

.github/workflows/test-example-notebooks.yaml

+    strategy:
+      fail-fast: false
+      matrix:
+        kubernetes-version: ["v1.28.7"]


@kubeflow/wg-training-leads @Electronic-Waste @saileshd1402 Do we want to run our Notebooks on all supported Kubernetes version similar to E2E tests:

training-operator/.github/workflows/integration-tests.yaml

Line 28 in 0ee9ca5

include:

?

Yeah, I agree with you. We should expand the tests to all supported Kubernetes versions.

andreyvelich · 2024-12-02T15:12:25Z

.github/workflows/test-example-notebooks.yaml

+        run: |
+          ./scripts/run-notebook.sh \
+          -i ./examples/pytorch/image-classification/create-pytorchjob.ipynb \
+          -o ./examples/pytorch/image-classification/create-pytorchjob-output.ipynb \


How are we going to use output in the tests ?

Yes, the output notebook is not being used, I'll not set it here

andreyvelich · 2024-12-02T15:14:34Z

scripts/run-notebook.sh

+  case "$opt" in
+    i) NOTEBOOK_INPUT="$OPTARG" ;;            # -i for notebook input path
+    o) NOTEBOOK_OUTPUT="$OPTARG" ;;           # -o for notebook output path
+    p) PAPERMILL_PARAMS+=("$OPTARG") ;;       # -p for papermill parameters


Since you name other papermill parameter as -k, should we name the namespace parameter as -n?

@saileshd1402 Please can you check it, so we can merge the PR ?

I added it already I think. Can you please check the latest commits once?

I think, you should remove -p parameter from the flags of this script since it is no longer needed

E.g. I mean this part:

training-operator/scripts/run-notebook.sh

Lines 61 to 63 in d3e9031

for param in "${PAPERMILL_PARAMS[@]}"; do

papermill_cmd="$papermill_cmd -p $param"

done

Oh understood, you are saying let's remove custom parameters of papermill in this script. I think we may use it in the future but I guess we can add if and when necessary. I'll remove it for this PR

Yeah, let's add them in the future once we need them.

andreyvelich · 2024-12-02T15:14:58Z

scripts/run-notebook.sh

+  echo "  -o  Output notebook (required)"
+  echo "  -p  Papermill parameters (optional), pass param name and value pair (in quotes whitespace separated)"
+  echo "  -y  Papermill parameters YAML file (optional)"
+  echo "  -k  Kubeflow Training Operator Python SDK (optional)"


Do you want to name it as -sdk to make it clearer ?

The current implementation using "getopts" accepts only one char argument names. I used this so the args parsing is short and clean. I can do longer names as well, but should I update the other args to have longer names?

Oh, I see. I think, it's fine to keep it as -k in that case.

andreyvelich · 2024-12-02T15:15:18Z

scripts/run-notebook.sh

+  echo "  -i  Input notebook (required)"
+  echo "  -o  Output notebook (required)"
+  echo "  -p  Papermill parameters (optional), pass param name and value pair (in quotes whitespace separated)"
+  echo "  -y  Papermill parameters YAML file (optional)"


Why do we need this ?

We can pass the parameters to papermill through yaml file as well, we are not using right now, but might use in the future

Should we introduce this parameter once we get feedback from users/developers that we need it ?

Sure okay, I will remove it for now

andreyvelich · 2024-12-02T15:16:08Z

scripts/run-notebook.sh

+NOTEBOOK_OUTPUT=""
+PAPERMILL_PARAMS=()
+PAPERMILL_PARAM_YAML=""
+TRAINING_PYTHON_SDK="git+https://github.com/kubeflow/training-operator.git#subdirectory=sdk/python"


Should we install the Python SDK directly from the repo by default, so developers can quickly test their SDK changes on one of the Notebooks?
Similar to how you run this script in the tests.

andreyvelich · 2024-12-02T15:16:54Z

scripts/run-notebook.sh

@@ -0,0 +1,82 @@
+#!/bin/bash
+
+# Copyright 2021 The Kubernetes Authors.


Please change the licence

andreyvelich · 2024-12-02T15:19:24Z

/assign @akshaychitneni

google-oss-prow · 2024-12-02T15:19:28Z

@andreyvelich: GitHub didn't allow me to assign the following users: akshaychitneni.

Note that only kubeflow members with read permissions, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @akshaychitneni

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.

Signed-off-by: sailesh duddupudi <[email protected]>

saileshd1402 · 2024-12-03T09:09:39Z

@andreyvelich @Electronic-Waste, Thank you for the review comments. I've addressed them, please have a look and let me know if they look good. Thanks!

andreyvelich · 2024-12-02T19:19:21Z

.github/workflows/setup-e2e-test/action.yaml

+inputs:
+  kubernetes-version:
+    required: true
+    description: kubernetes version
+  python-version:
+    required: true
+    description: Python version


Should we set the matrix with Kubernetes and Python versions as part of our setup-e2e-test template ?
Right now, we set it in the integration-tests.yaml.
So we can keep it consistent for our E2Es + Notebooks tests.
WDYT @tenzen-y @Electronic-Waste @saileshd1402 ?

One small thing is that if there are other steps that need some of these versions, for example gang-scheduler-name here, we will need to export them as environments variables to access in subsequent steps using GITHUB_ENV. Is there any other way or is this fine?

Oh, I see. I guess, we only use scheduler plugins for integration tests.
@kubeflow/wg-training-leads @saileshd1402 do we want to tests our Notebooks with various scheduling plugins as well ?
Or we want to limit the tests that we run with gang-scheduling ?

Should we set the matrix with Kubernetes and Python versions as part of our setup-e2e-test template ?
Right now, we set it in the integration-tests.yaml.
So we can keep it consistent for our E2Es + Notebooks tests.
WDYT @tenzen-y @Electronic-Waste @saileshd1402 ?

I think so. It will be better if we could execute e2e tests with multiple Kubernetes and Python versions.

I guess, maybe gang-scheduling could be limited to only intergration-test.yaml since it is a bit redundant to test them again in notebook tests.

@saileshd1402 Maybe to unblock this PR, we can just use GITHUB_ENV for now and set the gang-scheduler only for integration tests.
For the V2 tests, we can come back to this discussion.

I found out that we can't use matrix inside a single composite action. It can only be used in jobs/workflows files. This is because composite action avoids duplication of steps but can't be used to create more jobs like a workflow file. There is also Reusable Workflows but that can't be used in this case since it's spawns separate workflow to run the template, which means that we can't use it to setup environment of current job. Related docs: matrix strategies and composite actions.

I see, thanks for checking!

Electronic-Waste

Basically LGTM!

/lgtm

Signed-off-by: sailesh duddupudi <[email protected]>

andreyvelich

Thank you for this effort @saileshd1402!
/lgtm
/approve

google-oss-prow · 2024-12-09T13:32:59Z

[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:

~~OWNERS~~ [andreyvelich]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

google-oss-prow bot added the do-not-merge/work-in-progress label Sep 29, 2024

google-oss-prow bot requested review from jinchihe and kuizhiqing September 29, 2024 21:20

google-oss-prow bot added the size/L label Sep 29, 2024

saileshd1402 force-pushed the pytorch-job-notebook-test branch 2 times, most recently from 06c3b4b to 67a6b83 Compare September 29, 2024 23:00

saileshd1402 changed the title ~~[WIP] Added test for create-pytorchjob.ipynb~~ Added test for create-pytorchjob.ipynb Sep 29, 2024

google-oss-prow bot removed the do-not-merge/work-in-progress label Sep 29, 2024

saileshd1402 commented Sep 30, 2024

View reviewed changes

saileshd1402 changed the title ~~Added test for create-pytorchjob.ipynb~~ Added test for create-pytorchjob.ipynb python notebook Sep 30, 2024

andreyvelich mentioned this pull request Oct 11, 2024

[Test] E2e Tests for Notebook Examples kubeflow/katib#2417

Open

5 tasks

Electronic-Waste reviewed Nov 16, 2024

View reviewed changes

google-oss-prow bot assigned Electronic-Waste Nov 16, 2024

google-oss-prow bot added the lgtm label Nov 16, 2024

Electronic-Waste mentioned this pull request Nov 19, 2024

Sdk tests with papermill kubeflow/katib#2448

Draft

1 task

andreyvelich reviewed Nov 26, 2024

View reviewed changes

google-oss-prow bot removed the lgtm label Dec 2, 2024

Electronic-Waste reviewed Dec 2, 2024

View reviewed changes

saileshd1402 added 10 commits December 2, 2024 14:43

Added test for create-pytorchjob.ipynb

c90bbaf

Signed-off-by: sailesh duddupudi <[email protected]>

fix yaml syntax

f8fd24c

Signed-off-by: sailesh duddupudi <[email protected]>

Fix uses path

89023ce

Signed-off-by: sailesh duddupudi <[email protected]>

Add actions/checkout

62be575

Signed-off-by: sailesh duddupudi <[email protected]>

Add bash to action.yaml

9ea7155

Signed-off-by: sailesh duddupudi <[email protected]>

Install pip dependencies step

da99ec8

Signed-off-by: sailesh duddupudi <[email protected]>

Add quotes for args

4595f32

Signed-off-by: sailesh duddupudi <[email protected]>

Add jupyter

8b744b1

Signed-off-by: sailesh duddupudi <[email protected]>

Add nbformat_minor: 5 to fix invalid format error

c6d1925

Signed-off-by: sailesh duddupudi <[email protected]>

Fix job name

1124ee8

Signed-off-by: sailesh duddupudi <[email protected]>

saileshd1402 added 2 commits December 2, 2024 14:43

Update Usage

8c7f517

Signed-off-by: sailesh duddupudi <[email protected]>

Install dependencies in yaml

caeffab

Signed-off-by: sailesh duddupudi <[email protected]>

saileshd1402 force-pushed the pytorch-job-notebook-test branch from 4c135d5 to caeffab Compare December 2, 2024 14:43

google-oss-prow bot added size/XXL and removed size/L labels Dec 2, 2024

merge conflit fix

b545c80

Signed-off-by: sailesh duddupudi <[email protected]>

google-oss-prow bot added size/L and removed size/XXL labels Dec 2, 2024

saileshd1402 added 2 commits December 2, 2024 14:56

fix ipynb

87999f1

Signed-off-by: sailesh duddupudi <[email protected]>

set bash flags

0ee9ca5

Signed-off-by: sailesh duddupudi <[email protected]>

andreyvelich reviewed Dec 2, 2024

View reviewed changes

Update script args and add more kubernetes versions for tests

4ea4bde

Signed-off-by: sailesh duddupudi <[email protected]>

saileshd1402 force-pushed the pytorch-job-notebook-test branch from 1ee4d16 to 4ea4bde Compare December 2, 2024 16:17

saileshd1402 added 2 commits December 3, 2024 06:27

add gang-scheduler-name to template

72dd617

Signed-off-by: sailesh duddupudi <[email protected]>

move go setup to template

d3e9031

Signed-off-by: sailesh duddupudi <[email protected]>

andreyvelich reviewed Dec 3, 2024

View reviewed changes

Electronic-Waste reviewed Dec 5, 2024

View reviewed changes

google-oss-prow bot added the lgtm label Dec 5, 2024

remove -p parameter from script

21a6129

Signed-off-by: sailesh duddupudi <[email protected]>

google-oss-prow bot removed the lgtm label Dec 9, 2024

andreyvelich reviewed Dec 9, 2024

View reviewed changes

google-oss-prow bot assigned andreyvelich Dec 9, 2024

google-oss-prow bot added the lgtm label Dec 9, 2024

google-oss-prow bot added the approved label Dec 9, 2024

google-oss-prow bot merged commit 56cbe60 into kubeflow:master Dec 9, 2024
51 checks passed

andreyvelich mentioned this pull request Dec 9, 2024

[Test] E2e Tests for Notebook Examples #2246

Closed

		# Install Python dependencies to run Jupyter Notebooks with papermill
		pip install jupyter ipykernel papermill==2.6.0

	for param in "${PAPERMILL_PARAMS[@]}"; do
	papermill_cmd="$papermill_cmd -p $param"
	done

		@@ -0,0 +1,82 @@
		#!/bin/bash

		# Copyright 2021 The Kubernetes Authors.

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

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

Conversation

saileshd1402 commented Sep 29, 2024 • edited Loading

review-notebook-app bot commented Sep 29, 2024

coveralls commented Sep 29, 2024 • edited Loading

Pull Request Test Coverage Report for Build 12236369721

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

saileshd1402 Sep 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Electronic-Waste left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreyvelich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saileshd1402 Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreyvelich Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

Electronic-Waste left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saileshd1402 Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

Electronic-Waste Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

andreyvelich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saileshd1402 Dec 9, 2024 • edited Loading

Choose a reason for hiding this comment

andreyvelich Dec 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreyvelich commented Dec 2, 2024

google-oss-prow bot commented Dec 2, 2024

saileshd1402 commented Dec 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saileshd1402 Dec 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Electronic-Waste left a comment

Choose a reason for hiding this comment

andreyvelich left a comment

Choose a reason for hiding this comment

google-oss-prow bot commented Dec 9, 2024

saileshd1402 commented Sep 29, 2024 •

edited

Loading

coveralls commented Sep 29, 2024 •

edited

Loading

saileshd1402 Sep 30, 2024 •

edited

Loading

saileshd1402 Dec 2, 2024 •

edited

Loading

andreyvelich Dec 2, 2024 •

edited

Loading

saileshd1402 Dec 2, 2024 •

edited

Loading

Electronic-Waste Dec 2, 2024 •

edited

Loading

saileshd1402 Dec 9, 2024 •

edited

Loading

andreyvelich Dec 9, 2024 •

edited

Loading

saileshd1402 Dec 8, 2024 •

edited

Loading