Skip to content

Commit 3924fe7

Browse files
andyatmiamiandyatmiami
committed
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
1 parent 44d818a commit 3924fe7

29 files changed

+846
-380
lines changed

‎.gitignore

+1
Original file line numberDiff line numberDiff line change
@@ -128,6 +128,7 @@ venv/
128128
ENV/
129129
env.bak/
130130
venv.bak/
131+
.DS_store
131132

132133
# Spyder project settings
133134
.spyderproject

‎Makefile

+10-61
Original file line numberDiff line numberDiff line change
@@ -28,13 +28,15 @@ BUILD_DEPENDENT_IMAGES ?= yes
2828
PUSH_IMAGES ?= yes
2929

3030
# OS dependant: Generate date, select appropriate cmd to locate container engine
31-
ifeq ($(OS), Windows_NT)
32-
DATE ?= $(shell powershell -Command "Get-Date -Format 'yyyyMMdd'")
33-
WHERE_WHICH ?= where
34-
else
35-
DATE ?= $(shell date +'%Y%m%d')
36-
WHERE_WHICH ?= which
31+
ifdef OS
32+
ifeq ($(OS), Windows_NT)
33+
DATE ?= $(shell powershell -Command "Get-Date -Format 'yyyyMMdd'")
34+
WHERE_WHICH ?= where
35+
endif
3736
endif
37+
DATE ?= $(shell date +'%Y%m%d')
38+
WHERE_WHICH ?= which
39+
3840

3941
# linux/amd64 or darwin/arm64
4042
OS_ARCH=$(shell go env GOOS)/$(shell go env GOARCH)
@@ -340,64 +342,11 @@ undeploy-c9s-%: bin/kubectl
340342
$(info # Undeploying notebook from $(NOTEBOOK_DIR) directory...)
341343
$(KUBECTL_BIN) delete -k $(NOTEBOOK_DIR)
342344

343-
# Function for testing a notebook with papermill
344-
# ARG 1: Notebook name
345-
# ARG 1: UBI flavor
346-
# ARG 1: Python kernel
347-
define test_with_papermill
348-
$(eval PREFIX_NAME := $(subst /,-,$(1)_$(2)))
349-
$(KUBECTL_BIN) exec $(FULL_NOTEBOOK_NAME) -- /bin/sh -c "python3 -m pip install papermill"
350-
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
351-
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."
352-
exit 1
353-
fi
354-
if $(KUBECTL_BIN) exec $(FULL_NOTEBOOK_NAME) -- /bin/sh -c "cat $(PREFIX_NAME)_error.txt | grep --quiet FAILED" ; then
355-
echo "ERROR: The $(1) $(2) notebook encountered a failure. The make process has been aborted."
356-
$(KUBECTL_BIN) exec $(FULL_NOTEBOOK_NAME) -- /bin/sh -c "cat $(PREFIX_NAME)_error.txt"
357-
exit 1
358-
fi
359-
endef
360-
361345
# 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.
362346
.PHONY: test
363347
test-%: bin/kubectl
364-
# Verify the notebook's readiness by pinging the /api endpoint
365-
$(eval NOTEBOOK_NAME := $(subst .,-,$(subst cuda-,,$*)))
366-
$(eval PYTHON_VERSION := $(shell echo $* | sed 's/.*-python-//'))
367-
$(info # Running tests for $(NOTEBOOK_NAME) notebook...)
368-
$(KUBECTL_BIN) wait --for=condition=ready pod -l app=$(NOTEBOOK_NAME) --timeout=600s
369-
$(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.*"
370-
$(eval FULL_NOTEBOOK_NAME = $(shell ($(KUBECTL_BIN) get pods -l app=$(NOTEBOOK_NAME) -o custom-columns=":metadata.name" | tr -d '\n')))
371-
372-
# Tests notebook's functionalities
373-
if echo "$(FULL_NOTEBOOK_NAME)" | grep -q "minimal-ubi9"; then
374-
$(call test_with_papermill,minimal,ubi9,python-$(PYTHON_VERSION))
375-
elif echo "$(FULL_NOTEBOOK_NAME)" | grep -q "intel-tensorflow-ubi9"; then
376-
$(call test_with_papermill,intel/tensorflow,ubi9,python-$(PYTHON_VERSION))
377-
elif echo "$(FULL_NOTEBOOK_NAME)" | grep -q "intel-pytorch-ubi9"; then
378-
$(call test_with_papermill,intel/pytorch,ubi9,python-$(PYTHON_VERSION))
379-
elif echo "$(FULL_NOTEBOOK_NAME)" | grep -q "datascience-ubi9"; then
380-
$(MAKE) validate-ubi9-datascience PYTHON_VERSION=$(PYTHON_VERSION) -e FULL_NOTEBOOK_NAME=$(FULL_NOTEBOOK_NAME)
381-
elif echo "$(FULL_NOTEBOOK_NAME)" | grep -q "pytorch-ubi9"; then
382-
$(MAKE) validate-ubi9-datascience PYTHON_VERSION=$(PYTHON_VERSION) -e FULL_NOTEBOOK_NAME=$(FULL_NOTEBOOK_NAME)
383-
$(call test_with_papermill,pytorch,ubi9,python-$(PYTHON_VERSION))
384-
elif echo "$(FULL_NOTEBOOK_NAME)" | grep -q "tensorflow-ubi9"; then
385-
$(MAKE) validate-ubi9-datascience PYTHON_VERSION=$(PYTHON_VERSION) -e FULL_NOTEBOOK_NAME=$(FULL_NOTEBOOK_NAME)
386-
$(call test_with_papermill,tensorflow,ubi9,python-$(PYTHON_VERSION))
387-
elif echo "$(FULL_NOTEBOOK_NAME)" | grep -q "intel-ml-ubi9"; then
388-
$(call test_with_papermill,intel/ml,ubi9,python-$(PYTHON_VERSION))
389-
elif echo "$(FULL_NOTEBOOK_NAME)" | grep -q "trustyai-ubi9"; then
390-
$(call test_with_papermill,trustyai,ubi9,python-$(PYTHON_VERSION))
391-
elif echo "$(FULL_NOTEBOOK_NAME)" | grep -q "anaconda"; then
392-
echo "There is no test notebook implemented yet for Anaconda Notebook...."
393-
else
394-
echo "No matching condition found for $(FULL_NOTEBOOK_NAME)."
395-
fi
396-
397-
.PHONY: validate-ubi9-datascience
398-
validate-ubi9-datascience:
399-
$(call test_with_papermill,minimal,ubi9,python-$(PYTHON_VERSION))
400-
$(call test_with_papermill,datascience,ubi9,python-$(PYTHON_VERSION))
348+
$(info # Running tests for $* notebook...)
349+
@./scripts/test_jupyter_with_papermill.sh $*
401350

402351
# Validate that runtime image meets minimum criteria
403352
# 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

+27-10
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,9 @@
77
"metadata": {},
88
"outputs": [],
99
"source": [
10+
"from pathlib import Path\n",
11+
"import json\n",
12+
"import re\n",
1013
"import unittest\n",
1114
"from unittest import mock\n",
1215
"from platform import python_version\n",
@@ -24,22 +27,35 @@
2427
"import kafka\n",
2528
"from kafka import KafkaConsumer, KafkaProducer, TopicPartition\n",
2629
"from kafka.producer.buffer import SimpleBufferPool\n",
27-
"from kafka import KafkaConsumer\n",
2830
"from kafka.errors import KafkaConfigurationError\n",
2931
"import boto3\n",
3032
"\n",
3133
"def get_major_minor(s):\n",
3234
" return '.'.join(s.split('.')[:2])\n",
3335
"\n",
36+
"def load_expected_versions() -> dict:\n",
37+
" lock_file = Path('./expected_versions.json')\n",
38+
" data = {}\n",
39+
"\n",
40+
" with open(lock_file, 'r') as file:\n",
41+
" data = json.load(file)\n",
42+
"\n",
43+
" return data \n",
44+
"\n",
45+
"def get_expected_version(dependency_name: str) -> str:\n",
46+
" raw_value = expected_versions.get(dependency_name)\n",
47+
" raw_version = re.sub(r'^\\D+', '', raw_value)\n",
48+
" return get_major_minor(raw_version)\n",
49+
"\n",
3450
"class TestPythonVersion(unittest.TestCase):\n",
3551
" def test_version(self):\n",
36-
" expected_major_minor = '3.11'\n",
52+
" expected_major_minor = expected_major_minor = get_expected_version('Python')\n",
3753
" actual_major_minor = get_major_minor(python_version())\n",
3854
" self.assertEqual(actual_major_minor, expected_major_minor, \"incorrect version\")\n",
3955
"\n",
4056
"class TestPandas(unittest.TestCase):\n",
4157
" def test_version(self):\n",
42-
" expected_major_minor = '2.2'\n",
58+
" expected_major_minor = get_expected_version('Pandas')\n",
4359
" actual_major_minor = get_major_minor(pd.__version__)\n",
4460
" self.assertEqual(actual_major_minor, expected_major_minor, \"incorrect version\")\n",
4561
"\n",
@@ -94,7 +110,7 @@
94110
"\n",
95111
"class TestNumpy(unittest.TestCase):\n",
96112
" def test_version(self):\n",
97-
" expected_major_minor = '2.1'\n",
113+
" expected_major_minor = get_expected_version('Numpy')\n",
98114
" actual_major_minor = get_major_minor(np.__version__)\n",
99115
" self.assertEqual(actual_major_minor, expected_major_minor, \"incorrect version\")\n",
100116
"\n",
@@ -119,7 +135,7 @@
119135
"\n",
120136
"class TestScipy(unittest.TestCase):\n",
121137
" def test_version(self):\n",
122-
" expected_major_minor = '1.14'\n",
138+
" expected_major_minor = get_expected_version('Scipy')\n",
123139
" actual_major_minor = get_major_minor(scipy.__version__)\n",
124140
" self.assertEqual(actual_major_minor, expected_major_minor, \"incorrect version\")\n",
125141
"\n",
@@ -136,7 +152,7 @@
136152
"\n",
137153
"class TestSKLearn(unittest.TestCase):\n",
138154
" def test_version(self):\n",
139-
" expected_major_minor = '1.5'\n",
155+
" expected_major_minor = get_expected_version('Scikit-learn')\n",
140156
" actual_major_minor = get_major_minor(sklearn.__version__)\n",
141157
" self.assertEqual(actual_major_minor, expected_major_minor, \"incorrect version\")\n",
142158
"\n",
@@ -160,7 +176,7 @@
160176
"class TestMatplotlib(unittest.TestCase):\n",
161177
"\n",
162178
" def test_version(self):\n",
163-
" expected_major_minor = '3.9'\n",
179+
" expected_major_minor = get_expected_version('Matplotlib')\n",
164180
" actual_major_minor = get_major_minor(matplotlib.__version__)\n",
165181
" self.assertEqual(actual_major_minor, expected_major_minor, \"incorrect version\")\n",
166182
"\n",
@@ -171,7 +187,7 @@
171187
"class TestKafkaPython(unittest.TestCase):\n",
172188
"\n",
173189
" def test_version(self):\n",
174-
" expected_major_minor = '2.2'\n",
190+
" expected_major_minor = get_expected_version('Kafka-Python-ng')\n",
175191
" actual_major_minor = get_major_minor(kafka.__version__)\n",
176192
" self.assertEqual(actual_major_minor, expected_major_minor, \"incorrect version\")\n",
177193
"\n",
@@ -193,7 +209,7 @@
193209
"class TestBoto3(unittest.TestCase):\n",
194210
"\n",
195211
" def test_version(self):\n",
196-
" expected_major_minor = '1.35'\n",
212+
" expected_major_minor = get_expected_version('Boto3')\n",
197213
" actual_major_minor = get_major_minor(boto3.__version__)\n",
198214
" self.assertEqual(actual_major_minor, expected_major_minor, \"incorrect version\")\n",
199215
"\n",
@@ -212,6 +228,7 @@
212228
"\n",
213229
" self.assertEqual(boto3.DEFAULT_SESSION, session)\n",
214230
"\n",
231+
"expected_versions = load_expected_versions()\n",
215232
"unittest.main(argv=[''], verbosity=2, exit=False)"
216233
]
217234
}
@@ -223,5 +240,5 @@
223240
"orig_nbformat": 4
224241
},
225242
"nbformat": 4,
226-
"nbformat_minor": 2
243+
"nbformat_minor": 5
227244
}

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

+6-4
Original file line numberDiff line numberDiff line change
@@ -36,17 +36,19 @@ spec:
3636
livenessProbe:
3737
tcpSocket:
3838
port: notebook-port
39-
initialDelaySeconds: 5
40-
periodSeconds: 5
39+
initialDelaySeconds: 15
40+
periodSeconds: 10
41+
timeoutSeconds: 5
4142
successThreshold: 1
4243
failureThreshold: 3
4344
readinessProbe:
4445
httpGet:
4546
path: /notebook/opendatahub/jovyan/api
4647
port: notebook-port
4748
scheme: HTTP
48-
initialDelaySeconds: 10
49-
periodSeconds: 5
49+
initialDelaySeconds: 15
50+
periodSeconds: 10
51+
timeoutSeconds: 5
5052
successThreshold: 1
5153
failureThreshold: 3
5254
resources:

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

+33-19
Original file line numberDiff line numberDiff line change
@@ -3,11 +3,15 @@
33
{
44
"cell_type": "code",
55
"execution_count": null,
6+
"id": "bac7ee1b-65c4-4515-8006-7ba01e843906",
67
"metadata": {
78
"tags": []
89
},
910
"outputs": [],
1011
"source": [
12+
"from pathlib import Path\n",
13+
"import json\n",
14+
"import re\n",
1115
"import unittest\n",
1216
"from unittest import mock\n",
1317
"from platform import python_version\n",
@@ -27,7 +31,6 @@
2731
"from kafka.producer.buffer import SimpleBufferPool\n",
2832
"from kafka import KafkaConsumer\n",
2933
"from kafka.errors import KafkaConfigurationError\n",
30-
"import boto3\n",
3134
"import kfp_tekton\n",
3235
"import kfp\n",
3336
"from kfp import LocalClient, run_pipeline_func_locally\n",
@@ -37,15 +40,29 @@
3740
"def get_major_minor(s):\n",
3841
" return '.'.join(s.split('.')[:2])\n",
3942
"\n",
43+
"def load_expected_versions() -> dict:\n",
44+
" lock_file = Path('./expected_versions.json')\n",
45+
" data = {}\n",
46+
"\n",
47+
" with open(lock_file, 'r') as file:\n",
48+
" data = json.load(file)\n",
49+
"\n",
50+
" return data \n",
51+
"\n",
52+
"def get_expected_version(dependency_name: str) -> str:\n",
53+
" raw_value = expected_versions.get(dependency_name)\n",
54+
" raw_version = re.sub(r'^\\D+', '', raw_value)\n",
55+
" return get_major_minor(raw_version)\n",
56+
" \n",
4057
"class TestPythonVersion(unittest.TestCase):\n",
4158
" def test_version(self):\n",
42-
" expected_major_minor = '3.11'\n",
59+
" expected_major_minor = get_expected_version('Python')\n",
4360
" actual_major_minor = get_major_minor(python_version())\n",
4461
" self.assertEqual(actual_major_minor, expected_major_minor, \"incorrect version\")\n",
4562
"\n",
4663
"class TestModin(unittest.TestCase):\n",
4764
" def test_version(self):\n",
48-
" expected_major_minor = '0.24'\n",
65+
" expected_major_minor = get_expected_version('Modin')\n",
4966
" actual_major_minor = get_major_minor(pdm.__version__)\n",
5067
" self.assertEqual(actual_major_minor, expected_major_minor, \"incorrect version\")\n",
5168
"\n",
@@ -61,7 +78,7 @@
6178
"\n",
6279
"class TestPandas(unittest.TestCase):\n",
6380
" def test_version(self):\n",
64-
" expected_major_minor = '2.1'\n",
81+
" expected_major_minor = get_expected_version('Pandas')\n",
6582
" actual_major_minor = get_major_minor(pd.__version__)\n",
6683
" self.assertEqual(actual_major_minor, expected_major_minor, \"incorrect version\")\n",
6784
"\n",
@@ -116,7 +133,7 @@
116133
"\n",
117134
"class TestNumpy(unittest.TestCase):\n",
118135
" def test_version(self):\n",
119-
" expected_major_minor = '1.24'\n",
136+
" expected_major_minor = get_expected_version('Numpy')\n",
120137
" actual_major_minor = get_major_minor(np.__version__)\n",
121138
" self.assertEqual(actual_major_minor, expected_major_minor, \"incorrect version\")\n",
122139
"\n",
@@ -141,7 +158,7 @@
141158
"\n",
142159
"class TestScipy(unittest.TestCase):\n",
143160
" def test_version(self):\n",
144-
" expected_major_minor = '1.11'\n",
161+
" expected_major_minor = get_expected_version('Scipy')\n",
145162
" actual_major_minor = get_major_minor(scipy.__version__)\n",
146163
" self.assertEqual(actual_major_minor, expected_major_minor, \"incorrect version\")\n",
147164
"\n",
@@ -158,7 +175,7 @@
158175
"\n",
159176
"class TestSKLearn(unittest.TestCase):\n",
160177
" def test_version(self):\n",
161-
" expected_major_minor = '1.3'\n",
178+
" expected_major_minor = get_expected_version('Scikit-learn')\n",
162179
" actual_major_minor = get_major_minor(sklearn.__version__)\n",
163180
" self.assertEqual(actual_major_minor, expected_major_minor, \"incorrect version\")\n",
164181
"\n",
@@ -182,7 +199,7 @@
182199
"class TestMatplotlib(unittest.TestCase):\n",
183200
"\n",
184201
" def test_version(self):\n",
185-
" expected_major_minor = '3.6'\n",
202+
" expected_major_minor = get_expected_version('Matplotlib')\n",
186203
" actual_major_minor = get_major_minor(matplotlib.__version__)\n",
187204
" self.assertEqual(actual_major_minor, expected_major_minor, \"incorrect version\")\n",
188205
"\n",
@@ -193,7 +210,7 @@
193210
"class TestKafkaPython(unittest.TestCase):\n",
194211
"\n",
195212
" def test_version(self):\n",
196-
" expected_major_minor = '2.0'\n",
213+
" expected_major_minor = get_expected_version('Kafka-Python')\n",
197214
" actual_major_minor = get_major_minor(kafka.__version__)\n",
198215
" self.assertEqual(actual_major_minor, expected_major_minor, \"incorrect version\")\n",
199216
"\n",
@@ -215,19 +232,16 @@
215232
"class TestKFPTekton(unittest.TestCase):\n",
216233
"\n",
217234
" def test_version(self):\n",
218-
" expected_major_minor = '1.6.0'\n",
235+
" unsupported_version = '1.6'\n",
219236
"\n",
220-
" self.assertLess(kfp_tekton.__version__, expected_major_minor)\n",
237+
" expected_major_minor = get_expected_version('KFP-Tekton')\n",
221238
"\n",
239+
" self.assertLess(expected_major_minor, unsupported_version)\n",
240+
" self.assertLess(kfp_tekton.__version__, unsupported_version)\n",
241+
"\n",
242+
"expected_versions = load_expected_versions()\n",
222243
"unittest.main(argv=[''], verbosity=2, exit=False)"
223244
]
224-
},
225-
{
226-
"cell_type": "code",
227-
"execution_count": null,
228-
"metadata": {},
229-
"outputs": [],
230-
"source": []
231245
}
232246
],
233247
"metadata": {
@@ -250,5 +264,5 @@
250264
}
251265
},
252266
"nbformat": 4,
253-
"nbformat_minor": 4
267+
"nbformat_minor": 5
254268
}

0 commit comments

Comments
 (0)