Sdk tests with papermill #2448

yehudit1987 · 2024-10-27T10:51:42Z

What this PR does / why we need it:
This PR creates E2E tests for katib examples to run with papermill.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #2417

Checklist:

Docs included if any changes are user facing

Signed-off-by: Yehudit Kerido <yehudit.kerido@nokia.com>

review-notebook-app · 2024-10-27T10:51:47Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

google-oss-prow · 2024-10-27T10:51:58Z

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

OWNERS

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

Electronic-Waste · 2024-10-28T02:57:15Z

/rerun-all

Electronic-Waste · 2024-10-28T06:22:56Z

@yehudit1987 Can you please fix these CI errors?

Electronic-Waste · 2024-10-29T12:07:12Z

@yehudit1987 Can you sign your commits with git commit -s? The DCO checks failed due to this reason.

Electronic-Waste · 2024-10-29T12:08:26Z

FYI, you can check this reference: https://github.com/kubeflow/katib/pull/2448/checks?check_run_id=32215445282

Signed-off-by: Yehudit Kerido <yehudit.kerido@nokia.com>

… trigger it Signed-off-by: Yehudit Kerido <yehudit.kerido@nokia.com>

… trigger it 2 Signed-off-by: Yehudit Kerido <yehudit.kerido@nokia.com>

Signed-off-by: Yehudit Kerido <yehudit.kerido@nokia.com>

Electronic-Waste · 2024-10-31T05:13:54Z

/rerun-all

Electronic-Waste · 2024-11-06T07:46:30Z

/rerun-all

Electronic-Waste

Sorry for the late review @yehudit1987. I was very busy recently.

I left some comments for you. Thanks for your great contributions!

Electronic-Waste · 2024-11-14T03:34:29Z

examples/v1beta1/sdk/cmaes-and-resume-policies.ipynb

+    "from kubernetes import client, config\n",
+    "\n",
+    "# Initialize KatibClient\n",
+    "kclient = KatibClient(namespace=namespace)\n",
+    "\n",
+    "# Load kubeconfig\n",
+    "config.load_kube_config()\n",
+    "\n",
+    "# Kubernetes API for managing namespaces\n",
+    "core_v1_api = client.CoreV1Api()\n",
+    "\n",
+    "# Function to add label to namespace if it doesn't have the required one\n",
+    "def add_katib_label_to_namespace(namespace):\n",
+    "    ns = core_v1_api.read_namespace(namespace)\n",
+    "    labels = ns.metadata.labels or {}\n",
+    "    if labels.get(\"katib.kubeflow.org/metrics-collector-injection\") != \"enabled\":\n",
+    "        print(f\"Adding label to namespace {namespace}...\")\n",
+    "        labels[\"katib.kubeflow.org/metrics-collector-injection\"] = \"enabled\"\n",
+    "        body = {\"metadata\": {\"labels\": labels}}\n",
+    "        core_v1_api.patch_namespace(namespace, body)\n",
+    "        print(f\"Label added to namespace {namespace}.\")\n",
+    "    else:\n",
+    "        print(f\"Namespace {namespace} already has the required label.\")\n",
+    "\n",
+    "# Add the required label to the namespace\n",
+    "add_katib_label_to_namespace(namespace)\n",


Why do we need to add these lines?

Electronic-Waste · 2024-11-14T03:37:32Z

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

+  papermill-args-yaml:
+    description: 'Additional arguments to pass to Papermill in yaml format'
+    required: false
+    default: ""


It seems that we didn't pass parameters to these two notebook examples. Not sure if it meets our requirements

cc👀 @andreyvelich @tenzen-y @yehudit1987

Electronic-Waste · 2024-11-14T14:41:40Z

test/e2e/v1beta1/scripts/gh-actions/setup-katib.sh

@@ -65,10 +65,12 @@ echo "Deploying Katib"
 cd ../../../../../ && WITH_DATABASE_TYPE=$WITH_DATABASE_TYPE make deploy && cd -

 # Wait until all Katib pods is running.
-TIMEOUT=120s
+TIMEOUT=180s


Why do we need to change TIMEOUT to 180s?

Electronic-Waste · 2024-11-14T14:42:46Z

test/e2e/v1beta1/scripts/gh-actions/setup-katib.sh

-kubectl wait --for=condition=ContainersReady=True --timeout=${TIMEOUT} -l "katib.kubeflow.org/component in ($WITH_DATABASE_TYPE,controller,db-manager,ui)" -n kubeflow pod ||
-  (kubectl get pods -n kubeflow && kubectl describe pods -n kubeflow && exit 1)
+echo "Waiting for pods to be ready for $TIMEOUT seconds..."
+sleep $TIMEOUT


Is it necessary since we already have kubectl wait instruction? Please let me know your thought.

Electronic-Waste · 2024-11-14T14:43:41Z

test/e2e/v1beta1/scripts/gh-actions/setup-katib.sh

+if ! kubectl get namespaces | grep -q "kubeflow-user-example-com"; then
+  kubectl create namespace kubeflow-user-example-com
+fi
+


Maybe we can use default namespace instead of creating a new one:)

Electronic-Waste · 2024-11-14T14:54:12Z

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

+    - name: Install dependencies
+      shell: bash
+      run: |
+        python -m pip install --upgrade pip
+        pip install papermill kubeflow-katib jupyter ipykernel
+        python -m ipykernel install --user --name python3 --display-name "Python 3"
+
+    - name: Setup Minikube Cluster
+      shell: bash
+      run: ./test/e2e/v1beta1/scripts/gh-actions/setup-minikube.sh true true "" "" "cmaes"


Don't we need to create minikube cluster between these two steps?

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

Lines 39 to 48 in 2b41ae6

- name: Setup Minikube Cluster

uses: medyagh/setup-minikube@v0.0.18

with:

network-plugin: cni

cni: flannel

driver: none

kubernetes-version: ${{ inputs.kubernetes-version }}

minikube-version: 1.31.1

start-args: --wait-timeout=120s

Electronic-Waste · 2024-11-14T14:55:58Z

test/e2e/v1beta1/scripts/gh-actions/setup-minikube.sh

+function check_minikube() {
+  if minikube status >/dev/null 2>&1; then
+    echo "Minikube is already running."
+  else
+    echo "Minikube is not running. Starting Minikube..."
+    minikube start
+  fi
+}
+
+echo "Checking Minikube Kubernetes Cluster"
+check_minikube


I guess, we do not check the status of minikube cluster here.

Electronic-Waste · 2024-11-14T14:57:08Z

examples/v1beta1/sdk/tune-train-from-func.ipynb

It'll be better if you do not delete the original content.

And I think it's necessary to rewrite this example since TFJobClient() is outdated in the newest SDK in training-operator. WDYT👀 @kubeflow/wg-automl-leads

I didn't apply to many changes to that notebook beside as you mentioned replacing the TFJobClient with TrainingClient and specifying the job type. For some reason it looks like I rewrite the whole example. Anyway I fixed the "corrupted" notebook. Also I fixed the other issues you pointed at.

Regarding default namespace, it failed to create experiments without specifying the namespace.

Thanks @yehudit1987!

I know some code lines were generated by pycharm or vscode. The "original content" I meant is some output and images in the notebook, not those code lines.

As for the namespace, could we specify default namespace for the experiment like namespace=default? And it will be better if we could specify namespace for papermill like: kubeflow/training-operator#2274 .

Signed-off-by: Yehudit Kerido <yehudit.kerido@nokia.com>

Electronic-Waste

A lot of effort @yehudit1987 ! Thanks for your contribution.

I left some comments for you. cc👀 @kubeflow/wg-automl-leads

Electronic-Waste · 2024-11-19T09:55:41Z

examples/v1beta1/kubeflow-pipelines/kubeflow-e2e-mnist.ipynb

@@ -671,4 +671,4 @@
 },
 "nbformat": 4,
 "nbformat_minor": 4
-}
+}


Suggested change

}

}

Electronic-Waste · 2024-11-19T09:56:56Z

test/e2e/v1beta1/scripts/gh-actions/setup-katib.sh

-kubectl wait --for=condition=ContainersReady=True --timeout=${TIMEOUT} -l "katib.kubeflow.org/component in ($WITH_DATABASE_TYPE,controller,db-manager,ui)" -n kubeflow pod ||
-  (kubectl get pods -n kubeflow && kubectl describe pods -n kubeflow && exit 1)
+kubectl wait --for=condition=ContainersReady=True --timeout=${TIMEOUT} -l "katib.kubeflow.org/component in ($WITH_DATABASE_TYPE,controller,db-manager,ui)" -n kubeflow pod || (kubectl get pods -n kubeflow && kubectl describe pods -n kubeflow && exit 1)


I think it would be better if we could adjust the format of this line.

(Recover its original state)

Electronic-Waste · 2024-11-19T10:01:23Z

examples/v1beta1/sdk/cmaes-and-resume-policies.ipynb

+   "metadata": {
+    "pycharm": {
+     "name": "#%%\n"
+    }
+   },
   "outputs": [],
   "source": [
    "# Experiment name and namespace.\n",
-    "namespace = \"kubeflow-user-example-com\"\n",
+    "namespace = \"kubeflow\"\n",
    "experiment_name = \"cmaes-example\"\n",
    "\n",
    "metadata = V1ObjectMeta(\n",


Can we add parameters tag in metadata and allow args in papermill rewrite them like: kubeflow/training-operator#2274?

Electronic-Waste · 2024-11-19T10:02:28Z

examples/v1beta1/sdk/tune-train-from-func.ipynb

@@ -314,7 +342,8 @@
    "\n",
    "# Start the Katib Experiment.\n",
    "exp_name = \"tune-mnist\"\n",
-    "katib_client = katib.KatibClient()\n",
+    "namespace=\"kubeflow\"\n",


Electronic-Waste · 2024-11-19T10:06:11Z

examples/v1beta1/sdk/tune-train-from-func.ipynb

+    "import time\n",
+    "time.sleep(120)\n",
+    "status = katib_client.is_experiment_succeeded(exp_name, namespace=namespace)\n",


Can we replace fixed-time sleep with wait_for_experiment_condition()?

katib/sdk/python/v1beta1/kubeflow/katib/api/katib_client.py

Lines 1002 to 1010 in 2b41ae6

def wait_for_experiment_condition(

self,

name: str,

namespace: Optional[str] = None,

expected_condition: str = constants.EXPERIMENT_CONDITION_SUCCEEDED,

timeout: int = 600,

polling_interval: int = 15,

apiserver_timeout: int = constants.DEFAULT_TIMEOUT,

):

Yes, I think we can use this API here.

Electronic-Waste · 2024-11-19T10:09:33Z

/rerun-all

andreyvelich

Thank you for this effort and updating broken Notebooks in Katib @yehudit1987!
Let's finalize this PR once we design the testing script in the Training Operator.

andreyvelich · 2024-11-30T00:57:11Z

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

+    - name: Run Jupyter Notebook with Papermill
+      shell: bash
+      run: |
+        IFS=',' read -r -a NOTEBOOK_ARRAY <<< "${{ inputs.notebook-input }}"
+        # Loop through each notebook path
+        for NOTEBOOK in "${NOTEBOOK_ARRAY[@]}"; do
+          OUTPUT_FILE="${NOTEBOOK%.ipynb}_output.ipynb"
+          echo "Running notebook: $NOTEBOOK"
+          papermill "$NOTEBOOK" "$OUTPUT_FILE" --log-output --kernel python3 || {
+              echo "Papermill failed for notebook: $NOTEBOOK"
+              exit 1
+          }
+        done


As we discussed with @saileshd1402 in the Training Operator PR: kubeflow/training-operator#2274 (comment), we might want to create script to run those Notebooks with papermill rather than adding the script in the GitHub action directly.
I think, once we finalize it, we can use the same approach for Katib tests.

Hi @andreyvelich for now I fix all the previous comments.
One of the notebooks is again seems to be rewritten (even though using jupyter lab) as you suggest.
Anyway I will fix those notebooks together with the decision about using the script or not.
Please keep me update on that.

andreyvelich · 2024-11-30T00:59:22Z

examples/v1beta1/sdk/tune-train-from-func.ipynb

+    "import time\n",
+    "time.sleep(120)\n",
+    "status = katib_client.is_experiment_succeeded(exp_name, namespace=namespace)\n",


Yes, I think we can use this API here.

andreyvelich · 2024-11-30T01:01:50Z

examples/v1beta1/sdk/tune-train-from-func.ipynb

+    "pycharm": {
+     "name": "#%% md\n"
+    }


I would suggest to edit these Notebooks using JupyterLab directly.
In that case, the JSON format will be correctly rendered for every IDE.
E.g. you can just run JupyterLab locally to edit them:

pip install jupyterlab jupyter lab

Signed-off-by: Yehudit Kerido <yehudit.kerido@nokia.com>

Electronic-Waste · 2024-12-02T13:20:05Z

/rerun-all

Yehudit Kerido added 5 commits October 27, 2024 12:49

Running 2 experiments

79a27bd

Signed-off-by: Yehudit Kerido <yehudit.kerido@nokia.com>

Using action.yaml

0b44b8f

Signed-off-by: Yehudit Kerido <yehudit.kerido@nokia.com>

Using action.yaml - fix relative paths

6237428

Signed-off-by: Yehudit Kerido <yehudit.kerido@nokia.com>

Using action.yaml - fix relative paths2

113d8dd

Signed-off-by: Yehudit Kerido <yehudit.kerido@nokia.com>

Final test

cc17690

Signed-off-by: Yehudit Kerido <yehudit.kerido@nokia.com>

google-oss-prow bot added the size/XXL label Oct 27, 2024

google-oss-prow bot requested review from anencore94 and Electronic-Waste October 27, 2024 10:51

yehudit1987 marked this pull request as draft October 28, 2024 09:45

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

Yehudit Kerido added 10 commits October 29, 2024 21:29

Add other build step to flow

e074c65

Signed-off-by: Yehudit Kerido <yehudit.kerido@nokia.com>

Revert setup-minikube script and add debug step for mnist workflow to…

4fb0f37

… trigger it Signed-off-by: Yehudit Kerido <yehudit.kerido@nokia.com>

Revert setup-minikube script and add debug step for mnist workflow to…

ffa1c59

… trigger it 2 Signed-off-by: Yehudit Kerido <yehudit.kerido@nokia.com>

Add condition for mnist to run.

fedcf05

Signed-off-by: Yehudit Kerido <yehudit.kerido@nokia.com>

Fix ci issues

b7fa3ca

Signed-off-by: Yehudit Kerido <yehudit.kerido@nokia.com>

Fix ci issues

9560840

Signed-off-by: Yehudit Kerido <yehudit.kerido@nokia.com>

Fix ci issues

d69a29f

Signed-off-by: Yehudit Kerido <yehudit.kerido@nokia.com>

Fix CI issues

abadb73

Signed-off-by: Yehudit Kerido <yehudit.kerido@nokia.com>

Fix CI issues

885ba39

Signed-off-by: Yehudit Kerido <yehudit.kerido@nokia.com>

Fix CI issues

6633aa5

Signed-off-by: Yehudit Kerido <yehudit.kerido@nokia.com>

yehudit1987 force-pushed the sdk-tests-with-papermill branch from 963d367 to 6633aa5 Compare October 29, 2024 19:29

yehudit1987 marked this pull request as ready for review October 29, 2024 19:31

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

google-oss-prow bot added the size/XL label Nov 5, 2024

yehudit1987 marked this pull request as ready for review November 5, 2024 13:00

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

google-oss-prow bot requested a review from Electronic-Waste November 5, 2024 13:00

Electronic-Waste reviewed Nov 14, 2024

View reviewed changes

Fixing issues

4d332cc

Signed-off-by: Yehudit Kerido <yehudit.kerido@nokia.com>

yehudit1987 marked this pull request as draft November 17, 2024 14:45

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

Yehudit Kerido added 2 commits November 18, 2024 07:59

Fix review comments

5076f8b

Signed-off-by: Yehudit Kerido <yehudit.kerido@nokia.com>

Fix review comments

71ed401

Signed-off-by: Yehudit Kerido <yehudit.kerido@nokia.com>

yehudit1987 marked this pull request as ready for review November 19, 2024 09:33

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

google-oss-prow bot requested a review from Electronic-Waste November 19, 2024 09:33

Electronic-Waste reviewed Nov 19, 2024

View reviewed changes

andreyvelich reviewed Nov 30, 2024

View reviewed changes

yehudit1987 marked this pull request as draft December 1, 2024 10:48

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

Yehudit Kerido added 2 commits December 1, 2024 12:53

Test sending ns as paparmill params

ea44cdf

Signed-off-by: Yehudit Kerido <yehudit.kerido@nokia.com>

Add wait for experiment success condition and parameterized ns

dd45944

Signed-off-by: Yehudit Kerido <yehudit.kerido@nokia.com>

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

Add wait for experiment success condition and parameterized ns

7d4bff3

Signed-off-by: Yehudit Kerido <yehudit.kerido@nokia.com>

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

Add wait for experiment success condition and parameterized ns

2c0ce60

Signed-off-by: Yehudit Kerido <yehudit.kerido@nokia.com>

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sdk tests with papermill #2448

Sdk tests with papermill #2448

yehudit1987 commented Oct 27, 2024

review-notebook-app bot commented Oct 27, 2024

google-oss-prow bot commented Oct 27, 2024

Electronic-Waste commented Oct 28, 2024

Electronic-Waste commented Oct 28, 2024 •

edited

Loading

Electronic-Waste commented Oct 29, 2024

Electronic-Waste commented Oct 29, 2024

Electronic-Waste commented Oct 31, 2024

Electronic-Waste commented Nov 6, 2024

Electronic-Waste left a comment

Electronic-Waste Nov 14, 2024

Electronic-Waste Nov 14, 2024

Electronic-Waste Nov 14, 2024

Electronic-Waste Nov 14, 2024

Electronic-Waste Nov 14, 2024

Electronic-Waste Nov 14, 2024

Electronic-Waste Nov 14, 2024

Electronic-Waste Nov 14, 2024

yehudit1987 Nov 19, 2024

Electronic-Waste Nov 19, 2024

Electronic-Waste left a comment

Electronic-Waste Nov 19, 2024

Electronic-Waste Nov 19, 2024

Electronic-Waste Nov 19, 2024

Electronic-Waste Nov 19, 2024

Electronic-Waste Nov 19, 2024

Electronic-Waste Nov 19, 2024

andreyvelich Nov 30, 2024

Electronic-Waste commented Nov 19, 2024

andreyvelich left a comment

andreyvelich Nov 30, 2024

yehudit1987 Dec 3, 2024

andreyvelich Nov 30, 2024

andreyvelich Nov 30, 2024

Electronic-Waste commented Dec 2, 2024

	- name: Setup Minikube Cluster
	uses: medyagh/setup-minikube@v0.0.18
	with:
	network-plugin: cni
	cni: flannel
	driver: none
	kubernetes-version: ${{ inputs.kubernetes-version }}
	minikube-version: 1.31.1
	start-args: --wait-timeout=120s

	def wait_for_experiment_condition(
	self,
	name: str,
	namespace: Optional[str] = None,
	expected_condition: str = constants.EXPERIMENT_CONDITION_SUCCEEDED,
	timeout: int = 600,
	polling_interval: int = 15,
	apiserver_timeout: int = constants.DEFAULT_TIMEOUT,
	):

Sdk tests with papermill #2448

Are you sure you want to change the base?

Sdk tests with papermill #2448

Conversation

yehudit1987 commented Oct 27, 2024

review-notebook-app bot commented Oct 27, 2024

google-oss-prow bot commented Oct 27, 2024

Electronic-Waste commented Oct 28, 2024

Electronic-Waste commented Oct 28, 2024 • edited Loading

Electronic-Waste commented Oct 29, 2024

Electronic-Waste commented Oct 29, 2024

Electronic-Waste commented Oct 31, 2024

Electronic-Waste commented Nov 6, 2024

Electronic-Waste 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

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

Electronic-Waste 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

Choose a reason for hiding this comment

Electronic-Waste commented Nov 19, 2024

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

Electronic-Waste commented Dec 2, 2024

Electronic-Waste commented Oct 28, 2024 •

edited

Loading