Added test for create-pytorchjob.ipynb python notebook #2274
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| name: Setup E2E test template | ||
| description: A composite action to setup e2e tests | ||
|
|
||
| inputs: | ||
| kubernetes-version: | ||
| required: true | ||
| description: kubernetes version | ||
| python-version: | ||
| required: true | ||
| description: Python version | ||
|
|
||
| runs: | ||
| using: composite | ||
| steps: | ||
| - name: Free-Up Disk Space | ||
| uses: ./.github/workflows/free-up-disk-space | ||
|
|
||
| - name: Setup Python | ||
| uses: actions/setup-python@v5 | ||
| with: | ||
| python-version: ${{ inputs.python-version }} | ||
|
|
||
| - name: Create k8s Kind Cluster | ||
| uses: helm/kind-action@9fdad0686e6f19fcd572f62516f5e0436f562ee7 | ||
| with: | ||
| node_image: kindest/node:${{ inputs.kubernetes-version }} | ||
| cluster_name: training-operator-cluster | ||
| kubectl_version: ${{ inputs.kubernetes-version }} | ||
|
|
||
| - name: Build training-operator | ||
| shell: bash | ||
| run: | | ||
| ./scripts/gha/build-image.sh | ||
| env: | ||
| TRAINING_CI_IMAGE: kubeflowtraining/training-operator:test | ||
|
|
||
| - name: Deploy training operator | ||
| shell: bash | ||
| run: | | ||
| ./scripts/gha/setup-training-operator.sh | ||
| docker system prune -a -f | ||
| docker system df | ||
| df -h | ||
| env: | ||
| KIND_CLUSTER: training-operator-cluster | ||
| TRAINING_CI_IMAGE: kubeflowtraining/training-operator:test | ||
| GANG_SCHEDULER_NAME: "none" | ||
| KUBERNETES_VERSION: ${{ inputs.kubernetes-version }} | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| name: Test example notebooks | ||
|
|
||
| on: | ||
| - pull_request | ||
|
|
||
| concurrency: | ||
| group: ${{ github.workflow }}-${{ github.ref }} | ||
| cancel-in-progress: true | ||
|
|
||
| jobs: | ||
| create-pytorchjob-notebook-test: | ||
| runs-on: ubuntu-latest | ||
| timeout-minutes: 30 | ||
| strategy: | ||
| fail-fast: false | ||
| matrix: | ||
| kubernetes-version: ["v1.28.7", "v1.29.2", "v1.30.6"] | ||
| python-version: ["3.9", "3.10", "3.11"] | ||
| steps: | ||
| - name: Checkout | ||
| uses: actions/checkout@v4 | ||
|
|
||
| - name: Setup E2E Tests | ||
| uses: ./.github/workflows/setup-e2e-test | ||
| with: | ||
| kubernetes-version: ${{ matrix.kubernetes-version }} | ||
| python-version: ${{ matrix.python-version }} | ||
|
|
||
| - name: Install Python Dependencies | ||
| run: | | ||
| pip install papermill==2.6.0 jupyter==1.1.1 ipykernel==6.29.5 | ||
| - name: Run Jupyter Notebook with Papermill | ||
| shell: bash | ||
| run: | | ||
| ./scripts/run-notebook.sh \ | ||
| -i ./examples/pytorch/image-classification/create-pytorchjob.ipynb \ | ||
| -n default \ | ||
| -k ./sdk/python |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,78 @@ | ||
| #!/bin/bash | ||
|
|
||
| # Copyright 2024 The Kubeflow Authors. | ||
| # | ||
| # Licensed under the Apache License, Version 2.0 (the "License"); | ||
| # you may not use this file except in compliance with the License. | ||
| # You may obtain a copy of the License at | ||
| # | ||
| # http://www.apache.org/licenses/LICENSE-2.0 | ||
| # | ||
| # Unless required by applicable law or agreed to in writing, software | ||
| # distributed under the License is distributed on an "AS IS" BASIS, | ||
| # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
|
|
||
| # This bash script is used to run the example notebooks | ||
|
|
||
| set -o errexit | ||
| set -o nounset | ||
| set -o pipefail | ||
|
|
||
| NOTEBOOK_INPUT="" | ||
| NOTEBOOK_OUTPUT="-" # outputs to console | ||
| PAPERMILL_PARAMS=() | ||
| NAMESPACE="default" | ||
| TRAINING_PYTHON_SDK="./sdk/python" | ||
|
|
||
| usage() { | ||
| echo "Usage: $0 -i <input_notebook> -o <output_notebook> [-p \"<param> <value>\"...] [-y <params.yaml>]" | ||
| echo "Options:" | ||
| 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 " -k Kubeflow Training Operator Python SDK (optional)" | ||
|
There was a problem hiding this comment. Do you want to name it as There was a problem hiding this comment. 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? There was a problem hiding this comment. Oh, I see. I think, it's fine to keep it as -k in that case. |
||
| echo " -n Kubernetes namespace used by tests" | ||
| echo " -h Show this help message" | ||
| echo "NOTE: papermill, jupyter and ipykernel are required Python dependencies to run Notebooks" | ||
| exit 1 | ||
| } | ||
|
|
||
| while getopts "i:o:p:k:n:r:d:h:" opt; do | ||
| 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 | ||
|
There was a problem hiding this comment. Since you name other papermill parameter as There was a problem hiding this comment. @saileshd1402 Please can you check it, so we can merge the PR ? There was a problem hiding this comment. I added it already I think. Can you please check the latest commits once? There was a problem hiding this comment. I think, you should remove There was a problem hiding this comment. E.g. I mean this part: training-operator/scripts/run-notebook.sh Lines 61 to 63 in d3e9031 There was a problem hiding this comment. 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 There was a problem hiding this comment. Yeah, let's add them in the future once we need them. |
||
| k) TRAINING_PYTHON_SDK="$OPTARG" ;; # -k for training operator python sdk | ||
| n) NAMESPACE="$OPTARG" ;; # -n for kubernetes namespace used by tests | ||
| h) usage ;; # -h for help (usage) | ||
| *) usage; exit 1 ;; | ||
| esac | ||
| done | ||
|
|
||
| if [ -z "$NOTEBOOK_INPUT" ]; then | ||
| echo "Error: -i notebook input path is required." | ||
| exit 1 | ||
| fi | ||
|
|
||
| papermill_cmd="papermill $NOTEBOOK_INPUT $NOTEBOOK_OUTPUT -p training_python_sdk $TRAINING_PYTHON_SDK -p namespace $NAMESPACE" | ||
| # Add papermill parameters (param name and value) | ||
| for param in "${PAPERMILL_PARAMS[@]}"; do | ||
| papermill_cmd="$papermill_cmd -p $param" | ||
| done | ||
|
|
||
| if ! command -v papermill &> /dev/null; then | ||
| echo "Error: papermill is not installed. Please install papermill to proceed." | ||
| exit 1 | ||
| fi | ||
|
|
||
| echo "Running command: $papermill_cmd" | ||
| $papermill_cmd | ||
|
|
||
| if [ $? -ne 0 ]; then | ||
| echo "Error: papermill execution failed." >&2 | ||
| exit 1 | ||
| fi | ||
|
|
||
| echo "Notebook execution completed successfully" | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back 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.
Should we set the matrix with Kubernetes and Python versions as part of our
setup-e2e-testtemplate ?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 ?
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.
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?
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.
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 ?
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 so. It will be better if we could execute e2e tests with multiple Kubernetes and Python versions.
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 guess, maybe gang-scheduling could be limited to only
intergration-test.yamlsince it is a bit redundant to test them again in notebook tests.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.
@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.
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 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.
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 see, thanks for checking!