fix(test): ensure papermill tests run successfully for all supported …

…notebooks

Our testing framework had a fundamental issue in that it would, for certain images, naively run the test suite of parent images for a child image.  In the case of `tensorflow` images, this caused problems because the (child) `tensorflow` image would **purposefully** _downgraded_ a package version given compatability issues. Furthermore, when attempting to verify the changes to address this core issue, numerous other bugs were encountered that required to be fixed.

Significant changes introduced in this commit:
- logic of the `test-%` Makefile target was moved into a shell script named `test_jupyter_with_papermill.sh`
- test resources required to be copied to pod are now retrieved via the locally checked out repo
    - previously there were pulled from a remote branch via `wget` (defaulting to `main` branch)
    - this ensure our PR checks are now always leveraging any updated files
- test_notebook.ipynb files now expect an `expected_versions.json` file to exist within the same directory.
    - `expected_versions.json` file is derived from the relevant `...-notebook-imagestream.yaml` manifest and leveraged when asserting on dependency versions
		- admittedly the duplication of various helper functions across all the notebook files is annoying - but helps to keep the `.ipynb` files self-contained
- `...-notebook-imagestream.yaml` manifest had annotations updated to include any package that had a unit test in `test_notebook.ipynb` asserting versions
- CUDA tensorflow unit test that converts to ONNX was updated to be actually functional

Minor changes introduced in this commit:
- use `ifdef OS` in Makefile to avoid warnings about undefined variable
- all `test_notebook.ipynb` files:
    - have an `id` attribute defined in metadata
		- specify the `nbformat` as `4.5`
- the more compute-intensive notebooks had `readinessProbe` and `livenessProbe` settings updated to be less aggressive
    - was observing liveness checks sporadically failing while the notebook was being tested - and this update seemed to help
- `trustyai` notebook now runs the minimal and datascience papermill tests (similar to `tensorflow` and `pytorch`) instead of including the test code within its own `test_noteook.ipynb` file
- various "quality of life" improvements where introduced into `test_jupyter_with_papermill.sh`
    - properly invoke tests for any valid/supported target
        - previously certain test targets required manual intervention in order to run end to end
    - improved logging (particularly when running with the `-x` flag)
		- more modular structure to hopefully improve readability
		- script now honors proper shell return code semantics (i.e. returns `0` on success)

It should also be noted that while most `intel` notebooks are now passing the papermill tests - there are issues with the `intel` `tensorflow` unit tests still.  More detail is captured in the JIRA ticket related to this commit.  However, we are imminently removing `intel` logic from the `notebooks` repo entirely... so I didn't wanna burn any more time trying to get that last notebook to pass as it will be removed shortly!

Further details on `expected_versions.json`:
- `yq` (which is installed via the `Makefile` is used to:
    - query the relevant imagestream manifest to parse out the `["opendatahub.io/notebook-software"]` and `["opendatahub.io/notebook-python-dependencies"]` annotations from the first element of the `spec.tags` attribute
		- inject name/version elements for `nbdime` and `nbgitpuller` (which are asserted against in `minimal` notebook tests)
		- convert this `yaml` list to a JSON object of the form: `{<package>: <version>}`
- this JSON object is then copied into the running notebook workload in the same directly that `test_notebook.ipynb` resides
- each `test_notebook.ipynb` has a couple helper functions defined to then interact with this file:
    - `def load_expected_versions() -> dict:`
		- `def get_expected_version(dependency_name: str) -> str:
- The argument provided to the `get_expected_version` function should match the `name` attribute of the JSON structure defined in the imagestream manifest

Related-to: https://issues.redhat.com/browse/RHOAIENG-16587

.gitignore

@@ -128,6 +128,7 @@ venv/
 ENV/
 env.bak/
 venv.bak/
+.DS_store
 
 # Spyder project settings
 .spyderproject

Makefile

@@ -28,13 +28,15 @@ BUILD_DEPENDENT_IMAGES ?= yes
 PUSH_IMAGES ?= yes
 
 # OS dependant: Generate date, select appropriate cmd to locate container engine
-ifeq ($(OS), Windows_NT)
-	DATE 		?= $(shell powershell -Command "Get-Date -Format 'yyyyMMdd'")
-	WHERE_WHICH ?= where
-else
-	DATE 		?= $(shell date +'%Y%m%d')
-	WHERE_WHICH ?= which
+ifdef OS
+	ifeq ($(OS), Windows_NT)
+		DATE 		?= $(shell powershell -Command "Get-Date -Format 'yyyyMMdd'")
+		WHERE_WHICH ?= where
+	endif
 endif
+DATE 		?= $(shell date +'%Y%m%d')
+WHERE_WHICH ?= which
+
 
 # linux/amd64 or darwin/arm64
 OS_ARCH=$(shell go env GOOS)/$(shell go env GOARCH)
@@ -340,64 +342,11 @@ undeploy-c9s-%: bin/kubectl
 	$(info # Undeploying notebook from $(NOTEBOOK_DIR) directory...)
 	$(KUBECTL_BIN) delete -k $(NOTEBOOK_DIR)
 
-# Function for testing a notebook with papermill
-#   ARG 1: Notebook name
-#   ARG 1: UBI flavor
-#   ARG 1: Python kernel
-define test_with_papermill
-	$(eval PREFIX_NAME := $(subst /,-,$(1)_$(2)))
-	$(KUBECTL_BIN) exec $(FULL_NOTEBOOK_NAME) -- /bin/sh -c "python3 -m pip install papermill"
-	if ! $(KUBECTL_BIN) exec $(FULL_NOTEBOOK_NAME) -- /bin/sh -c "wget ${NOTEBOOK_REPO_BRANCH_BASE}/jupyter/$(1)/$(2)-$(3)/test/test_notebook.ipynb -O test_notebook.ipynb && python3 -m papermill test_notebook.ipynb $(PREFIX_NAME)_output.ipynb --kernel python3 --stderr-file $(PREFIX_NAME)_error.txt" ; then
-		echo "ERROR: The $(1) $(2) notebook encountered a failure. To investigate the issue, you can review the logs located in the ocp-ci cluster on 'artifacts/notebooks-e2e-tests/jupyter-$(1)-$(2)-$(3)-test-e2e' directory or run 'cat $(PREFIX_NAME)_error.txt' within your container. The make process has been aborted."
-		exit 1
-	fi
-	if $(KUBECTL_BIN) exec $(FULL_NOTEBOOK_NAME) -- /bin/sh -c "cat $(PREFIX_NAME)_error.txt | grep --quiet FAILED" ; then
-		echo "ERROR: The $(1) $(2) notebook encountered a failure. The make process has been aborted."
-		$(KUBECTL_BIN) exec $(FULL_NOTEBOOK_NAME) -- /bin/sh -c "cat $(PREFIX_NAME)_error.txt"
-		exit 1
-	fi
-endef
-
 # Verify the notebook's readiness by pinging the /api endpoint and executing the corresponding test_notebook.ipynb file in accordance with the build chain logic.
 .PHONY: test
 test-%: bin/kubectl
-	# Verify the notebook's readiness by pinging the /api endpoint
-	$(eval NOTEBOOK_NAME := $(subst .,-,$(subst cuda-,,$*)))
-	$(eval PYTHON_VERSION := $(shell echo $* | sed 's/.*-python-//'))
-	$(info # Running tests for $(NOTEBOOK_NAME) notebook...)
-	$(KUBECTL_BIN) wait --for=condition=ready pod -l app=$(NOTEBOOK_NAME) --timeout=600s
-	$(KUBECTL_BIN) port-forward svc/$(NOTEBOOK_NAME)-notebook 8888:8888 & curl --retry 5 --retry-delay 5 --retry-connrefused http://localhost:8888/notebook/opendatahub/jovyan/api ; EXIT_CODE=$$?; echo && pkill --full "^$(KUBECTL_BIN).*port-forward.*"
-	$(eval FULL_NOTEBOOK_NAME = $(shell ($(KUBECTL_BIN) get pods -l app=$(NOTEBOOK_NAME) -o custom-columns=":metadata.name" | tr -d '\n')))
-
-	# Tests notebook's functionalities
-	if echo "$(FULL_NOTEBOOK_NAME)" | grep -q "minimal-ubi9"; then
-		$(call test_with_papermill,minimal,ubi9,python-$(PYTHON_VERSION))
-	elif echo "$(FULL_NOTEBOOK_NAME)" | grep -q "intel-tensorflow-ubi9"; then
-		$(call test_with_papermill,intel/tensorflow,ubi9,python-$(PYTHON_VERSION))
-	elif echo "$(FULL_NOTEBOOK_NAME)" | grep -q "intel-pytorch-ubi9"; then
-		$(call test_with_papermill,intel/pytorch,ubi9,python-$(PYTHON_VERSION))
-	elif echo "$(FULL_NOTEBOOK_NAME)" | grep -q "datascience-ubi9"; then
-		$(MAKE) validate-ubi9-datascience PYTHON_VERSION=$(PYTHON_VERSION) -e FULL_NOTEBOOK_NAME=$(FULL_NOTEBOOK_NAME)
-	elif echo "$(FULL_NOTEBOOK_NAME)" | grep -q "pytorch-ubi9"; then
-		$(MAKE) validate-ubi9-datascience PYTHON_VERSION=$(PYTHON_VERSION) -e FULL_NOTEBOOK_NAME=$(FULL_NOTEBOOK_NAME)
-		$(call test_with_papermill,pytorch,ubi9,python-$(PYTHON_VERSION))
-	elif echo "$(FULL_NOTEBOOK_NAME)" | grep -q "tensorflow-ubi9"; then
-		$(MAKE) validate-ubi9-datascience PYTHON_VERSION=$(PYTHON_VERSION) -e FULL_NOTEBOOK_NAME=$(FULL_NOTEBOOK_NAME)
-		$(call test_with_papermill,tensorflow,ubi9,python-$(PYTHON_VERSION))
-	elif echo "$(FULL_NOTEBOOK_NAME)" | grep -q "intel-ml-ubi9"; then
-		$(call test_with_papermill,intel/ml,ubi9,python-$(PYTHON_VERSION))
-	elif echo "$(FULL_NOTEBOOK_NAME)" | grep -q "trustyai-ubi9"; then
-		$(call test_with_papermill,trustyai,ubi9,python-$(PYTHON_VERSION))
-	elif echo "$(FULL_NOTEBOOK_NAME)" | grep -q "anaconda"; then
-		echo "There is no test notebook implemented yet for Anaconda Notebook...."
-	else
-		echo "No matching condition found for $(FULL_NOTEBOOK_NAME)."
-	fi
-
-.PHONY: validate-ubi9-datascience
-validate-ubi9-datascience:
-	$(call test_with_papermill,minimal,ubi9,python-$(PYTHON_VERSION))
-	$(call test_with_papermill,datascience,ubi9,python-$(PYTHON_VERSION))
+	$(info # Running tests for $* notebook...)
+	@./scripts/test_jupyter_with_papermill.sh $*
 
 # Validate that runtime image meets minimum criteria
 # This validation is created from subset of https://github.com/elyra-ai/elyra/blob/9c417d2adc9d9f972de5f98fd37f6945e0357ab9/Makefile#L325

jupyter/datascience/ubi9-python-3.11/test/test_notebook.ipynb

@@ -7,6 +7,9 @@
    "metadata": {},
    "outputs": [],
    "source": [
+    "from pathlib import Path\n",
+    "import json\n",
+    "import re\n",
     "import unittest\n",
     "from unittest import mock\n",
     "from platform import python_version\n",
@@ -24,22 +27,35 @@
     "import kafka\n",
     "from kafka import KafkaConsumer, KafkaProducer, TopicPartition\n",
     "from kafka.producer.buffer import SimpleBufferPool\n",
-    "from kafka import KafkaConsumer\n",
     "from kafka.errors import KafkaConfigurationError\n",
     "import boto3\n",
     "\n",
     "def get_major_minor(s):\n",
     "    return '.'.join(s.split('.')[:2])\n",
     "\n",
+    "def load_expected_versions() -> dict:\n",
+    "    lock_file = Path('./expected_versions.json')\n",
+    "    data = {}\n",
+    "\n",
+    "    with open(lock_file, 'r') as file:\n",
+    "        data = json.load(file)\n",
+    "\n",
+    "    return data    \n",
+    "\n",
+    "def get_expected_version(dependency_name: str) -> str:\n",
+    "    raw_value = expected_versions.get(dependency_name)\n",
+    "    raw_version = re.sub(r'^\\D+', '', raw_value)\n",
+    "    return get_major_minor(raw_version)\n",
+    "\n",
     "class TestPythonVersion(unittest.TestCase):\n",
     "    def test_version(self):\n",
-    "        expected_major_minor = '3.11'\n",
+    "        expected_major_minor = expected_major_minor = get_expected_version('Python')\n",
     "        actual_major_minor = get_major_minor(python_version())\n",
     "        self.assertEqual(actual_major_minor, expected_major_minor, \"incorrect version\")\n",
     "\n",
     "class TestPandas(unittest.TestCase):\n",
     "    def test_version(self):\n",
-    "        expected_major_minor = '2.2'\n",
+    "        expected_major_minor = get_expected_version('Pandas')\n",
     "        actual_major_minor = get_major_minor(pd.__version__)\n",
     "        self.assertEqual(actual_major_minor, expected_major_minor, \"incorrect version\")\n",
     "\n",
@@ -94,7 +110,7 @@
     "\n",
     "class TestNumpy(unittest.TestCase):\n",
     "    def test_version(self):\n",
-    "        expected_major_minor = '2.1'\n",
+    "        expected_major_minor = get_expected_version('Numpy')\n",
     "        actual_major_minor = get_major_minor(np.__version__)\n",
     "        self.assertEqual(actual_major_minor, expected_major_minor, \"incorrect version\")\n",
     "\n",
@@ -119,7 +135,7 @@
     "\n",
     "class TestScipy(unittest.TestCase):\n",
     "    def test_version(self):\n",
-    "        expected_major_minor = '1.14'\n",
+    "        expected_major_minor = get_expected_version('Scipy')\n",
     "        actual_major_minor = get_major_minor(scipy.__version__)\n",
     "        self.assertEqual(actual_major_minor, expected_major_minor, \"incorrect version\")\n",
     "\n",
@@ -136,7 +152,7 @@
     "\n",
     "class TestSKLearn(unittest.TestCase):\n",
     "    def test_version(self):\n",
-    "        expected_major_minor = '1.5'\n",
+    "        expected_major_minor = get_expected_version('Scikit-learn')\n",
     "        actual_major_minor = get_major_minor(sklearn.__version__)\n",
     "        self.assertEqual(actual_major_minor, expected_major_minor, \"incorrect version\")\n",
     "\n",
@@ -160,7 +176,7 @@
     "class TestMatplotlib(unittest.TestCase):\n",
     "\n",
     "    def test_version(self):\n",
-    "        expected_major_minor = '3.9'\n",
+    "        expected_major_minor = get_expected_version('Matplotlib')\n",
     "        actual_major_minor = get_major_minor(matplotlib.__version__)\n",
     "        self.assertEqual(actual_major_minor, expected_major_minor, \"incorrect version\")\n",
     "\n",
@@ -171,7 +187,7 @@
     "class TestKafkaPython(unittest.TestCase):\n",
     "\n",
     "    def test_version(self):\n",
-    "        expected_major_minor = '2.2'\n",
+    "        expected_major_minor = get_expected_version('Kafka-Python-ng')\n",
     "        actual_major_minor = get_major_minor(kafka.__version__)\n",
     "        self.assertEqual(actual_major_minor, expected_major_minor, \"incorrect version\")\n",
     "\n",
@@ -193,7 +209,7 @@
     "class TestBoto3(unittest.TestCase):\n",
     "\n",
     "    def test_version(self):\n",
-    "        expected_major_minor = '1.35'\n",
+    "        expected_major_minor = get_expected_version('Boto3')\n",
     "        actual_major_minor = get_major_minor(boto3.__version__)\n",
     "        self.assertEqual(actual_major_minor, expected_major_minor, \"incorrect version\")\n",
     "\n",
@@ -212,6 +228,7 @@
     "\n",
     "        self.assertEqual(boto3.DEFAULT_SESSION, session)\n",
     "\n",
+    "expected_versions = load_expected_versions()\n",
     "unittest.main(argv=[''], verbosity=2, exit=False)"
    ]
   }
@@ -223,5 +240,5 @@
   "orig_nbformat": 4
  },
  "nbformat": 4,
- "nbformat_minor": 2
+ "nbformat_minor": 5
 }

jupyter/intel/ml/ubi9-python-3.11/kustomize/base/statefulset.yaml

@@ -36,17 +36,19 @@ spec:
           livenessProbe:
             tcpSocket:
               port: notebook-port
-            initialDelaySeconds: 5
-            periodSeconds: 5
+            initialDelaySeconds: 15
+            periodSeconds: 10
+            timeoutSeconds: 5
             successThreshold: 1
             failureThreshold: 3
           readinessProbe:
             httpGet:
               path: /notebook/opendatahub/jovyan/api
               port: notebook-port
               scheme: HTTP
-            initialDelaySeconds: 10
-            periodSeconds: 5
+            initialDelaySeconds: 15
+            periodSeconds: 10
+            timeoutSeconds: 5
             successThreshold: 1
             failureThreshold: 3
           resources:

jupyter/intel/ml/ubi9-python-3.11/test/test_notebook.ipynb

@@ -3,11 +3,15 @@
   {
    "cell_type": "code",
    "execution_count": null,
+   "id": "bac7ee1b-65c4-4515-8006-7ba01e843906",
    "metadata": {
     "tags": []
    },
    "outputs": [],
    "source": [
+    "from pathlib import Path\n",
+    "import json\n",
+    "import re\n",
     "import unittest\n",
     "from unittest import mock\n",
     "from platform import python_version\n",
@@ -27,7 +31,6 @@
     "from kafka.producer.buffer import SimpleBufferPool\n",
     "from kafka import KafkaConsumer\n",
     "from kafka.errors import KafkaConfigurationError\n",
-    "import boto3\n",
     "import kfp_tekton\n",
     "import kfp\n",
     "from kfp import LocalClient, run_pipeline_func_locally\n",
@@ -37,15 +40,29 @@
     "def get_major_minor(s):\n",
     "    return '.'.join(s.split('.')[:2])\n",
     "\n",
+    "def load_expected_versions() -> dict:\n",
+    "    lock_file = Path('./expected_versions.json')\n",
+    "    data = {}\n",
+    "\n",
+    "    with open(lock_file, 'r') as file:\n",
+    "        data = json.load(file)\n",
+    "\n",
+    "    return data    \n",
+    "\n",
+    "def get_expected_version(dependency_name: str) -> str:\n",
+    "    raw_value = expected_versions.get(dependency_name)\n",
+    "    raw_version = re.sub(r'^\\D+', '', raw_value)\n",
+    "    return get_major_minor(raw_version)\n",
+    "    \n",
     "class TestPythonVersion(unittest.TestCase):\n",
     "    def test_version(self):\n",
-    "        expected_major_minor = '3.11'\n",
+    "        expected_major_minor = get_expected_version('Python')\n",
     "        actual_major_minor = get_major_minor(python_version())\n",
     "        self.assertEqual(actual_major_minor, expected_major_minor, \"incorrect version\")\n",
     "\n",
     "class TestModin(unittest.TestCase):\n",
     "    def test_version(self):\n",
-    "        expected_major_minor = '0.24'\n",
+    "        expected_major_minor = get_expected_version('Modin')\n",
     "        actual_major_minor = get_major_minor(pdm.__version__)\n",
     "        self.assertEqual(actual_major_minor, expected_major_minor, \"incorrect version\")\n",
     "\n",
@@ -61,7 +78,7 @@
     "\n",
     "class TestPandas(unittest.TestCase):\n",
     "    def test_version(self):\n",
-    "        expected_major_minor = '2.1'\n",
+    "        expected_major_minor = get_expected_version('Pandas')\n",
     "        actual_major_minor = get_major_minor(pd.__version__)\n",
     "        self.assertEqual(actual_major_minor, expected_major_minor, \"incorrect version\")\n",
     "\n",
@@ -116,7 +133,7 @@
     "\n",
     "class TestNumpy(unittest.TestCase):\n",
     "    def test_version(self):\n",
-    "        expected_major_minor = '1.24'\n",
+    "        expected_major_minor = get_expected_version('Numpy')\n",
     "        actual_major_minor = get_major_minor(np.__version__)\n",
     "        self.assertEqual(actual_major_minor, expected_major_minor, \"incorrect version\")\n",
     "\n",
@@ -141,7 +158,7 @@
     "\n",
     "class TestScipy(unittest.TestCase):\n",
     "    def test_version(self):\n",
-    "        expected_major_minor = '1.11'\n",
+    "        expected_major_minor = get_expected_version('Scipy')\n",
     "        actual_major_minor = get_major_minor(scipy.__version__)\n",
     "        self.assertEqual(actual_major_minor, expected_major_minor, \"incorrect version\")\n",
     "\n",
@@ -158,7 +175,7 @@
     "\n",
     "class TestSKLearn(unittest.TestCase):\n",
     "    def test_version(self):\n",
-    "        expected_major_minor = '1.3'\n",
+    "        expected_major_minor = get_expected_version('Scikit-learn')\n",
     "        actual_major_minor = get_major_minor(sklearn.__version__)\n",
     "        self.assertEqual(actual_major_minor, expected_major_minor, \"incorrect version\")\n",
     "\n",
@@ -182,7 +199,7 @@
     "class TestMatplotlib(unittest.TestCase):\n",
     "\n",
     "    def test_version(self):\n",
-    "        expected_major_minor = '3.6'\n",
+    "        expected_major_minor = get_expected_version('Matplotlib')\n",
     "        actual_major_minor = get_major_minor(matplotlib.__version__)\n",
     "        self.assertEqual(actual_major_minor, expected_major_minor, \"incorrect version\")\n",
     "\n",
@@ -193,7 +210,7 @@
     "class TestKafkaPython(unittest.TestCase):\n",
     "\n",
     "    def test_version(self):\n",
-    "        expected_major_minor = '2.0'\n",
+    "        expected_major_minor = get_expected_version('Kafka-Python')\n",
     "        actual_major_minor = get_major_minor(kafka.__version__)\n",
     "        self.assertEqual(actual_major_minor, expected_major_minor, \"incorrect version\")\n",
     "\n",
@@ -215,19 +232,16 @@
     "class TestKFPTekton(unittest.TestCase):\n",
     "\n",
     "    def test_version(self):\n",
-    "        expected_major_minor = '1.6.0'\n",
+    "        unsupported_version = '1.6'\n",
     "\n",
-    "        self.assertLess(kfp_tekton.__version__, expected_major_minor)\n",
+    "        expected_major_minor = get_expected_version('KFP-Tekton')\n",
     "\n",
+    "        self.assertLess(expected_major_minor, unsupported_version)\n",
+    "        self.assertLess(kfp_tekton.__version__, unsupported_version)\n",
+    "\n",
+    "expected_versions = load_expected_versions()\n",
     "unittest.main(argv=[''], verbosity=2, exit=False)"
    ]
-  },
-  {
-   "cell_type": "code",
-   "execution_count": null,
-   "metadata": {},
-   "outputs": [],
-   "source": []
   }
  ],
  "metadata": {
@@ -250,5 +264,5 @@
   }
  },
  "nbformat": 4,
- "nbformat_minor": 4
+ "nbformat_minor": 5
 }