Skip to content

NO-JIRA: tests(fix): remove unused tensorboard import (fixes #2077) #2111

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Sep 16, 2025
Merged

NO-JIRA: tests(fix): remove unused tensorboard import (fixes #2077) #2111

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d810b1c
opendatahub-io/notebooks32077: fix: remove unused tensorboard import …
pramodtoraskar Aug 25, 2025
c1434bb
❯ uv run ruff check jupyter/rocm/tensorflow/ubi9-python-3.12/test/tes…
jiridanek Sep 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 32 additions & 24 deletions jupyter/rocm/tensorflow/ubi9-python-3.12/test/test_notebook.ipynb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,49 +7,53 @@
"metadata": {},
"outputs": [],
"source": [
"from pathlib import Path\n",
"import json\n",
"import re\n",
"import unittest\n",
"from pathlib import Path\n",
"from platform import python_version\n",
"\n",
"import tensorflow as tf\n",
Copy link
Member

@jiridanek jiridanek Aug 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"import tensorflow as tf\n",
"import tensorflow as tf\n",
"import tensorboard as _ # for testing purposes\n",

When reporting the github issue, I was not sure if the import is genuinely unused or whether simply importing it is valuable as a sort of test. It checks that tensorboard is installed and is importable.

Maybe we should have a dedicated test that imports tensorboard to make it explicit.

Copy link
Contributor Author

@pramodtoraskar pramodtoraskar Aug 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @jiridanek for the thoughtful suggestion! I appreciate you taking the time to review this thoroughly.

I have a few thoughts I'd like to discuss, as a first-time contributor learning the best practices:

Question about the approach:

  • The original issue was a simple F401 linting warning (unused import)
  • Your suggestion adds both an import AND a dedicated test method
  • I'm wondering if this might be over-engineering for the problem we're solving

Alternative approaches to consider:

  1. Remove import entirely - simplest, cleanest solution
  2. Use import in existing test - import where it's actually needed
  3. Your suggested approach - explicit import + dedicated test

Questions for the team:

  • Is tensorboard actually a required dependency, or was this import leftover code?
  • Do we need both the import validation AND the existing test_tensorboard() method?
  • What's the project's philosophy on handling linting warnings vs. adding explicit tests?

As a newcomer, I'd love to understand the reasoning behind the preferred approach. I'm happy to implement whatever the team thinks is best - I'm just trying to understand the trade-offs and learn the project's standards.

Copy link
Member

@jiridanek jiridanek Aug 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another thing. I did not check, but I imagine the import tensorboard line may be duplicated many times in the repo, and many times it will be an unused import. So the fix should address all the places.

@coderabbitai can you please help find the tensorboard imports in this repo and determine if they are used or unused?

Copy link
Contributor

@coderabbitai coderabbitai bot Aug 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 Analysis chain

@jiridanek I'll help you find all tensorboard imports in the repository and analyze their usage patterns.

🏁 Script executed:

#!/bin/bash
# Find all files containing tensorboard imports
echo "=== Finding all tensorboard import statements ==="
rg -n "import tensorboard" --type=py --type=ipynb

echo -e "\n=== Finding alternative tensorboard import patterns ==="
rg -n "from tensorboard" --type=py --type=ipynb

echo -e "\n=== Finding files that reference tensorboard after import ==="
rg -n "tensorboard\." --type=py --type=ipynb

echo -e "\n=== Checking for tf.keras.callbacks.TensorBoard usage ==="
rg -n "tf\.keras\.callbacks\.TensorBoard" --type=py --type=ipynb

echo -e "\n=== Checking for TensorBoard callback usage ==="
rg -n "TensorBoard" --type=py --type=ipynb

Length of output: 1025

🏁 Script executed:

#!/bin/bash
# Find all files containing tensorboard imports - corrected approach
echo "=== Finding all tensorboard import statements in Python files ==="
rg -n "import tensorboard" --type=py

echo -e "\n=== Finding all tensorboard import statements in Jupyter notebooks ==="
rg -n "import tensorboard" --glob "*.ipynb"

echo -e "\n=== Finding alternative tensorboard import patterns in Python files ==="
rg -n "from tensorboard" --type=py

echo -e "\n=== Finding alternative tensorboard import patterns in Jupyter notebooks ==="
rg -n "from tensorboard" --glob "*.ipynb"

echo -e "\n=== Finding files that reference tensorboard after import in Python files ==="
rg -n "tensorboard\." --type=py

echo -e "\n=== Finding files that reference tensorboard after import in Jupyter notebooks ==="
rg -n "tensorboard\." --glob "*.ipynb"

echo -e "\n=== Checking for tf.keras.callbacks.TensorBoard usage ==="
rg -n "tf\.keras\.callbacks\.TensorBoard" --type=py --glob "*.ipynb"

echo -e "\n=== Checking for TensorBoard callback usage ==="
rg -n "TensorBoard" --type=py --glob "*.ipynb"

Length of output: 5077

@jiridanek Excellent intuition! Your suspicion was correct. I found exactly 3 files with unused import tensorboard statements:

Files with unused tensorboard imports:

  1. jupyter/rocm/tensorflow/ubi9-python-3.11/test/test_notebook.ipynb:15
  2. jupyter/tensorflow/ubi9-python-3.12/test/test_notebook.ipynb:15
  3. jupyter/tensorflow/ubi9-python-3.11/test/test_notebook.ipynb:15

Analysis:

  • All imports are genuinely unused: No files use tensorboard. anywhere
  • TensorBoard functionality is present: All files use tf.keras.callbacks.TensorBoard() instead
  • Same pattern everywhere: All 3 files have identical unused imports on line 15

Current situation:

Recommendation:

Rather than having separate PRs for each file, I recommend expanding this PR to address all 3 instances comprehensively. This would:

  1. Fix the root cause completely - no partial solutions
  2. Follow DRY principle - one fix for one recurring issue
  3. Reduce review overhead - single comprehensive review vs multiple small ones
  4. Ensure consistency - all TensorFlow test notebooks follow the same pattern

Would you like me to help create a GitHub issue to track the comprehensive fix for all remaining files? Based on the established patterns in your repository, this seems like the systematic approach that would best serve the project.

🧠 Learnings used 
Learnt from: jiridanek
PR: opendatahub-io/notebooks#0
File: :0-0
Timestamp: 2025-08-06T15:51:55.727Z
Learning: jiridanek requested GitHub issue creation for unused pathlib.Path import in jupyter/minimal/ubi9-python-3.12/test/test_notebook.ipynb during PR #1686 review. Issue #1687 was created with comprehensive problem description covering unused import at line 14, clear solution with diff example, detailed acceptance criteria including verification steps and broader audit suggestion, and proper context linking with PR and comment references, assigned to dibryant, continuing the established pattern of systematic code quality improvements through detailed issue tracking.

Learnt from: jiridanek
PR: opendatahub-io/notebooks#0
File: :0-0
Timestamp: 2025-08-06T15:51:58.704Z
Learning: jiridanek requested GitHub issue creation for unused pathlib.Path import removal in jupyter/minimal/ubi9-python-3.12/test/test_notebook.ipynb during PR #1686 review. Issue #1688 was successfully created with comprehensive problem description covering static analysis findings, specific file location and line number, clear before/after solution, detailed acceptance criteria, and proper context linking, continuing the established pattern of systematic code quality improvements through detailed issue tracking.

Learnt from: jiridanek
PR: opendatahub-io/notebooks#1333
File: runtimes/tensorflow/ubi9-python-3.12/Pipfile:13-15
Timestamp: 2025-07-08T19:29:32.006Z
Learning: jiridanek requested GitHub issue creation for investigating TensorFlow "and-cuda" extras usage patterns during PR #1333 review. Issue #1340 was created with comprehensive investigation framework covering platform-specific analysis, deployment scenarios, TensorFlow version compatibility, clear acceptance criteria, testing approach, and implementation timeline, continuing the established pattern of systematic code quality improvements through detailed issue tracking.

Learnt from: jiridanek
PR: opendatahub-io/notebooks#1333
File: runtimes/tensorflow/ubi9-python-3.12/Pipfile:13-15
Timestamp: 2025-07-08T19:29:32.006Z
Learning: jiridanek requested GitHub issue creation for investigating TensorFlow "and-cuda" extras usage patterns during PR #1333 review. Issue #1345 was created with comprehensive investigation framework covering platform-specific analysis, deployment scenarios, TensorFlow version compatibility, clear acceptance criteria, and testing approach, continuing the established pattern of systematic code quality improvements through detailed issue tracking.

Learnt from: jiridanek
PR: opendatahub-io/notebooks#1591
File: codeserver/ubi9-python-3.11/Pipfile.lock:1-1
Timestamp: 2025-08-01T16:03:18.732Z
Learning: jiridanek requested GitHub issue creation for Keras 3 / TensorFlow API compatibility problem during PR #1591 review. Issue #1594 was successfully created with comprehensive problem description covering API confusion between standalone keras==3.11.1 and tf.keras bundled with tensorflow==2.18.1, multiple solution options (remove standalone Keras, set KERAS_BACKEND, or documentation), clear acceptance criteria, affected files analysis, and proper context linking, continuing the established pattern of systematic code quality improvements through detailed issue tracking.

Learnt from: jiridanek
PR: opendatahub-io/notebooks#1259
File: jupyter/rocm/tensorflow/ubi9-python-3.12/kustomize/base/service.yaml:5-15
Timestamp: 2025-07-02T18:59:15.788Z
Learning: jiridanek creates targeted GitHub issues for specific test quality improvements identified during PR reviews in opendatahub-io/notebooks. Issue #1268 demonstrates this by converting a review comment about insufficient tf2onnx conversion test validation into a comprehensive improvement plan with clear acceptance criteria, code examples, and ROCm-specific context.

Learnt from: jiridanek
PR: opendatahub-io/notebooks#1306
File: jupyter/trustyai/ubi9-python-3.12/test/test_notebook.ipynb:71-76
Timestamp: 2025-07-04T06:04:43.085Z
Learning: jiridanek requested GitHub issue creation for duplicate CSV loading and validation problem in jupyter/trustyai/ubi9-python-3.12/test/test_notebook.ipynb during PR #1306 review. Issue #1322 was created with comprehensive problem description covering code redundancy, runtime failure risks, network inefficiency, and test reliability concerns, along with detailed solution including duplicate line removal, data validation implementation, repository-wide audit, acceptance criteria, implementation guidance, and proper context linking, continuing the established pattern of systematic code quality improvements through detailed issue tracking.

Learnt from: jiridanek
PR: opendatahub-io/notebooks#1333
File: runtimes/rocm-tensorflow/ubi9-python-3.12/Pipfile:13-15
Timestamp: 2025-07-09T08:07:24.937Z
Learning: jiridanek requested GitHub issue creation for tensorflow_rocm Python 3.12 compatibility problem during PR #1333 review. Issue #1354 was successfully created with comprehensive problem description covering missing cp312 wheels causing build failures, three solution options (upstream TensorFlow, Python 3.11 only, custom build), clear acceptance criteria, and proper context linking, continuing the established pattern of systematic code quality improvements through detailed issue tracking.

Learnt from: jiridanek
PR: opendatahub-io/notebooks#1333
File: runtimes/rocm-pytorch/ubi9-python-3.12/utils/bootstrapper.py:1-769
Timestamp: 2025-07-08T19:35:49.482Z
Learning: jiridanek requested GitHub issue creation for bootstrapper code duplication problem in runtimes/rocm-pytorch/ubi9-python-3.12/utils/bootstrapper.py during PR #1333 review. After an initial failed attempt, issue #1349 was successfully created with comprehensive problem description covering maintenance overhead and consistency risks from duplicate implementations across 5 Python 3.12 runtime environments, four solution options (symlinks, import-based, template-based, direct shared import) with pros/cons analysis, clear acceptance criteria for consolidation and maintainability, step-by-step implementation guidance, and proper context linking, continuing the established pattern of systematic code quality improvements through detailed issue tracking.

Learnt from: jiridanek
PR: opendatahub-io/notebooks#1306
File: jupyter/trustyai/ubi9-python-3.12/kustomize/base/kustomization.yaml:8-12
Timestamp: 2025-07-08T19:09:48.746Z
Learning: jiridanek requested GitHub issue creation for misleading CUDA prefix in TrustyAI image tags during PR #1306 review. Issue was created with comprehensive problem description covering both Python 3.11 and 3.12 versions, repository pattern analysis showing correct vs incorrect naming, clear solution with code examples, detailed acceptance criteria, and proper context linking, continuing the established pattern of systematic code quality improvements through detailed issue tracking.

Learnt from: jiridanek
PR: opendatahub-io/notebooks#2000
File: runtimes/rocm-pytorch/ubi9-python-3.11/requirements.txt:807-809
Timestamp: 2025-08-20T12:39:54.827Z
Learning: Issue #2055 tracks the remaining runtimes/rocm-tensorflow/ubi9-python-3.12/requirements.txt file that stayed at jupyter-core==5.7.2 during PR #2000, which will be addressed separately due to specific challenges with that ROCm TensorFlow image.

Learnt from: jiridanek
PR: opendatahub-io/notebooks#2000
File: runtimes/tensorflow/ubi9-python-3.11/Pipfile:41-41
Timestamp: 2025-08-20T12:19:34.523Z
Learning: jiridanek created GitHub issue #2055 to track the remaining runtimes/rocm-tensorflow/ubi9-python-3.12/Pipfile.lock file that stayed at jupyter-core==5.7.2 during PR #2000, ensuring systematic follow-up for complete CVE-2025-30167 remediation across all runtime images in the opendatahub-io/notebooks repository.

Learnt from: jiridanek
PR: opendatahub-io/notebooks#0
File: :0-0
Timestamp: 2025-06-16T11:32:09.203Z
Learning: In the opendatahub-io/notebooks repository, there is a known issue with missing `runtimes/rocm/pytorch/ubi9-python-3.11/kustomize/base/kustomization.yaml` file that causes rocm runtime tests to fail with "no such file or directory" error. This is tracked in JIRA RHOAIENG-22044 and was intended to be fixed in PR #1015.

Copy link
Member

@jiridanek jiridanek Aug 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TensorBoard functionality is present: All files use tf.keras.callbacks.TensorBoard() instead

Important to know, so the import by itself is not the only thing in the test that ensures TensorBoard is present in the image.

We will be removing the 3.11 code soon-ish, so we don't need to worry about those files

Copy link
Contributor

@coderabbitai coderabbitai bot Aug 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

Copy link
Member

@jiridanek jiridanek Aug 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would've been nice if the test actually asserted something about the tensorboard.

Copy link
Contributor Author

@pramodtoraskar pramodtoraskar Aug 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jiridanek Thank you for the detailed analysis! you're absolutely right - I think I should fix all 3 files in one PR rather than just one.

Quick Questions:

  1. Should I expand this PR to fix all 3 unused tensorboard imports?
  2. For the test improvements you mentioned - should I add actual assertions that validate TensorBoard functionality works?
  3. Since 3.11 code is being removed soon - should I still fix those files or focus only on the 3.12 ones?

I'm happy to make this PR comprehensive and address all instances. Just want to make sure I'm following the right approach.

Copy link
Contributor Author

@pramodtoraskar pramodtoraskar Sep 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @jiridanek @atheo89,
Looking for you comment to complete this PR. Any thoughts?

Copy link
Contributor Author

@pramodtoraskar pramodtoraskar Sep 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello Team
Looking for you comment to complete & merge this PR.
cc: @jiridanek @atheo89

"import tensorboard\n",
"import tf2onnx\n",
"from platform import python_version\n",
"\n",
"\n",
"def get_major_minor(s):\n",
" return '.'.join(s.split('.')[:2])\n",
" return \".\".join(s.split(\".\")[:2])\n",
"\n",
"\n",
"def load_expected_versions() -> dict:\n",
" lock_file = Path('./expected_versions.json')\n",
" lock_file = Path(\"./expected_versions.json\")\n",
" data = {}\n",
"\n",
" with open(lock_file, 'r') as file:\n",
" with open(lock_file, \"r\") as file:\n",
" data = json.load(file)\n",
"\n",
" return data \n",
" return data\n",
"\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",
" raw_version = re.sub(r\"^\\D+\", \"\", raw_value)\n",
" return get_major_minor(raw_version)\n",
"\n",
"\n",
"class TestTensorflowNotebook(unittest.TestCase):\n",
" \n",
"\n",
" def test_python_version(self):\n",
" expected_major_minor = get_expected_version('Python')\n",
" actual_major_minor = get_major_minor(python_version()) \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",
"\n",
" def test_tensorflow_version(self):\n",
" expected_major_minor = get_expected_version('ROCm-TensorFlow')\n",
" actual_major_minor = get_major_minor(tf.__version__) \n",
" expected_major_minor = get_expected_version(\"ROCm-TensorFlow\")\n",
" actual_major_minor = get_major_minor(tf.__version__)\n",
" self.assertEqual(actual_major_minor, expected_major_minor, \"incorrect version\")\n",
" \n",
"\n",
" def test_tf2onnx_conversion(self):\n",
" # Replace this with an actual TensorFlow model conversion using tf2onnx\n",
" model = tf.keras.Sequential([tf.keras.layers.Dense(1, input_shape=(10,))])\n",
" onnx_model = tf2onnx.convert.from_keras(model)\n",
" \n",
"\n",
" self.assertTrue(onnx_model is not None)\n",
"\n",
" def test_mnist_model(self):\n",
Expand All @@ -59,41 +63,45 @@
" x_train, x_test = x_train / 255.0, x_test / 255.0\n",
" model = tf.keras.models.Sequential([\n",
" tf.keras.layers.Flatten(input_shape=(28, 28)),\n",
" tf.keras.layers.Dense(128, activation='relu'),\n",
" tf.keras.layers.Dense(128, activation=\"relu\"),\n",
" tf.keras.layers.Dropout(0.2),\n",
" tf.keras.layers.Dense(10)\n",
" ])\n",
" predictions = model(x_train[:1]).numpy()\n",
" predictions\n",
" assert predictions\n",
" tf.nn.softmax(predictions).numpy()\n",
" loss_fn = tf.keras.losses.SparseCategoricalCrossentropy(from_logits=True)\n",
" loss_fn(y_train[:1], predictions).numpy()\n",
" model.compile(optimizer='adam', loss=loss_fn, metrics=['accuracy'])\n",
" model.compile(optimizer=\"adam\", loss=loss_fn, metrics=[\"accuracy\"])\n",
" model.fit(x_train, y_train, epochs=5)\n",
" model.evaluate(x_test, y_test, verbose=2)\n",
" model.evaluate(x_test, y_test, verbose=2)\n",
" probability_model = tf.keras.Sequential([\n",
" model,\n",
" tf.keras.layers.Softmax()\n",
" ])\n",
" probability_model(x_test[:5])\n",
"\n",
" def test_tensorboard(self):\n",
" # Check tensorboard is installed\n",
" import tensorboard as _ # noqa: PLC0415, F401\n",
"\n",
" # Create a simple model\n",
" model = tf.keras.Sequential([\n",
" tf.keras.layers.Dense(10, input_shape=(5,), activation='relu'),\n",
" tf.keras.layers.Dense(10, input_shape=(5,), activation=\"relu\"),\n",
" tf.keras.layers.Dense(1)\n",
" ])\n",
" # Compile the model\n",
" model.compile(optimizer='adam', loss='mse')\n",
" model.compile(optimizer=\"adam\", loss=\"mse\")\n",
" # Generate some example data\n",
" x_train = tf.random.normal((100, 5))\n",
" y_train = tf.random.normal((100, 1))\n",
" # Create a TensorBoard callback\n",
" log_dir = './logs'\n",
" log_dir = \"./logs\"\n",
" tensorboard_callback = tf.keras.callbacks.TensorBoard(log_dir=log_dir)\n",
" # Train the model\n",
" model.fit(x_train, y_train, epochs=2, callbacks=[tensorboard_callback])\n",
"\n",
"\n",
"expected_versions = load_expected_versions()\n",
"\n",
"suite = unittest.TestLoader().loadTestsFromTestCase(TestTensorflowNotebook)\n",
Expand Down
Loading