Vikash/fix/monai 1 6 release notebooks#2066
Open
vikashg wants to merge 13 commits into
Open
Conversation
…lity Add msd_crossval_datalist_generator.ipynb and hovernet_infer_compare.ipynb to doesnt_contain_max_epochs (inference/datalist notebooks with no training loop). Skip image_restoration.ipynb until monai.networks.nets.Restormer is merged into the dev branch. Fix endoscopic_inbody_classification notebook: pass return_state_dict=False to monai.bundle.load so it returns an nn.Module rather than a raw OrderedDict, which caused AttributeError on .train() with MONAI 1.6 API. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: R. Garcia-Dias <rafaelagd@gmail.com>
for more information, see https://pre-commit.ci Signed-off-by: R. Garcia-Dias <rafaelagd@gmail.com>
- competitions/MICCAI/surgtoolloc/preprocess_detect_scene_and_split_fold.ipynb: E225 - deep_atlas/deep_atlas_tutorial.ipynb: E225 - modules/interpretability/class_lung_lesion.ipynb: E231 in f-string indexing Signed-off-by: R. Garcia-Dias <rafaelagd@gmail.com>
…g pin pytorch-lightning~=2.0.0 (2.0.9) imports mlflow at module level via pytorch_lightning.loggers.mlflow; mlflow 3.13.0 fails to initialize under Python 3.12, making `import pytorch_lightning` fail in the training subprocess. pytorch-lightning>=2.1 uses lazy mlflow imports; confirmed working with 2.6.5: training and evaluation run to completion. Also documents the root cause in diagnose_1_6_release.md: two-part R7 issue (fd-limit download truncation + mlflow/pl import chain). Signed-off-by: R. Garcia-Dias <rafaelagd@gmail.com>
- Revert return_state_dict=False from endoscopic_inbody_classification.ipynb:
the upstream MONAI dev (≥1.5) removed the deprecated param entirely and
load() now returns nn.Module by default. The extra kwarg caused a TypeError
in CI. The original notebook is correct for MONAI ≥1.5.
- Update diagnose_1_6_release.md:
- R5: clarify it is a local-only issue (our MONAI @ 19cab577 still has the
deprecated return_state_dict=True default; upstream dev does not)
- R8: mark confirmed fixed — isolated run with --ulimit nofile=65536:65536
completed all 39 cells in 3m1s with no ancdata error (2026-06-11)
Signed-off-by: R. Garcia-Dias <rafaelagd@gmail.com>
Signed-off-by: R. Garcia-Dias <rafaelagd@gmail.com>
The notebook calls .to(device) where device is CUDA unconditionally; no CPU fallback exists. Fails with AssertionError: Torch not compiled with CUDA enabled on the CPU-only GitHub Actions runner. Signed-off-by: R. Garcia-Dias <rafaelagd@gmail.com>
deep_atlas_tutorial.ipynb hardcodes device = torch.device("cuda:0") with no
CPU fallback; fails with AssertionError: Torch not compiled with CUDA enabled
on the CPU-only GitHub Actions runner.
Signed-off-by: R. Garcia-Dias <rafaelagd@gmail.com>
…ax_epochs; fix stale pending table - runner.sh: hovernet_infer_compare.ipynb does not exist in the repo; the exemption was a dead entry that could never match. Removed. - diagnose_1_6_release.md: PEP8 row removed from the 'Still pending' table (autofix was already applied and marked done at line 401). Also updated runner.sh change summary to reflect the actual skip list entries (deep_atlas_tutorial, 05_spleen_segmentation_lightning). Signed-off-by: R. Garcia-Dias <rafaelagd@gmail.com>
…ages monai.io domain no longer resolves. Replace the download URL for MONAI-logo_color.png with the equivalent file hosted on the MONAI GitHub repository (raw.githubusercontent.com). Extract URL to variable to stay within 120-char PEP8 line limit. Signed-off-by: Vikash Gupta <write2vikash@gmail.com>
Fixed modules/interpretability/class_lung_lesion.ipynb: - Root cause: unknown
Fixed modules/lazy_resampling_benchmark.ipynb: - Root cause: unknown
Fixed modules/omniverse/omniverse_integration.ipynb: - Root cause: unknown
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
WalkthroughThis PR adds a release diagnostics report (diagnose_1_6_release.md) documenting MONAI 1.6 tutorial test failures and fixes, updates runner.sh notebook exemptions/skip lists and file discovery quoting, and applies minor notebook fixes (dependency version, download URL, and whitespace formatting). ChangesRelease Diagnostics Document
Notebook and Runner Test Fixes
Estimated code review effort: 2 (Simple) | ~10 minutes Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Ruff (0.15.20)modules/interpretability/class_lung_lesion.ipynbUnexpected end of JSON input competitions/MICCAI/surgtoolloc/preprocess_detect_scene_and_split_fold.ipynbUnexpected end of JSON input bundle/05_spleen_segmentation_lightning.ipynbUnexpected end of JSON input
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@diagnose_1_6_release.md`:
- Around line 14-20: The failure totals in the summary table are inconsistent:
the category counts and the overlap note in the `Total failures` row do not
reconcile. Update the totals in the markdown so the notebook-level unique
failure count matches the category breakdown and the stated overlap, using the
summary table entries and the `Total failures` line as the source of truth.
- Around line 259-263: The traceback fenced block in the diagnose_1_6_release.md
section needs a language label to satisfy markdownlint MD040. Update the fence
that contains the ImportError message to use a text-labeled code block so the
traceback is properly tagged, and keep the surrounding content unchanged.
In `@runner.sh`:
- Line 453: The `files=($(...))` assignment in `runner.sh` still performs unsafe
word-splitting and glob expansion on the `find` output, so filenames with spaces
will break. Update the array population to use a safe read mechanism such as
`mapfile`/`readarray` around the `find` command output, keeping the existing
quoted `$pattern` behavior while ensuring each path is stored as one array
element.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e520f0a5-19be-4725-87cc-2fd1b2e7d8bc
📒 Files selected for processing (8)
bundle/05_spleen_segmentation_lightning.ipynbcompetitions/MICCAI/surgtoolloc/preprocess_detect_scene_and_split_fold.ipynbcomputer_assisted_intervention/endoscopic_inbody_classification.ipynbdeep_atlas/deep_atlas_tutorial.ipynbdiagnose_1_6_release.mdmodules/interpretability/class_lung_lesion.ipynbmodules/load_medical_images.ipynbrunner.sh
Comment on lines
+14
to
+20
| | Category | Count | Notebooks | | ||
| |---|---|---| | ||
| | **Passed** | ~150 | — | | ||
| | **Failed — ExecutionError** | 91 | Papermill failed; specific errors not in log (stderr not captured) | | ||
| | **Failed — PEP8** | 3 | Style violations caught by flake8 | | ||
| | **Failed — MissingKeyword** | 1 | `max_epochs` not found and not exempted | | ||
| | **Total failures** | **94 events / 93 notebooks** | `class_lung_lesion.ipynb` has both PEP8 + ExecutionError | |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reconcile the failure totals.
The numbers don’t add up: 91 + 3 + 1 = 95 notebook-level failures, and with the one overlap called out here the unique notebook total should still be 94, not 93.
Suggested correction
-| **Total failures** | **94 events / 93 notebooks** | `class_lung_lesion.ipynb` has both PEP8 + ExecutionError |
+| **Total failures** | **95 events / 94 notebooks** | `class_lung_lesion.ipynb` has both PEP8 + ExecutionError |📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| | Category | Count | Notebooks | | |
| |---|---|---| | |
| | **Passed** | ~150 | — | | |
| | **Failed — ExecutionError** | 91 | Papermill failed; specific errors not in log (stderr not captured) | | |
| | **Failed — PEP8** | 3 | Style violations caught by flake8 | | |
| | **Failed — MissingKeyword** | 1 | `max_epochs` not found and not exempted | | |
| | **Total failures** | **94 events / 93 notebooks** | `class_lung_lesion.ipynb` has both PEP8 + ExecutionError | | |
| | Category | Count | Notebooks | | |
| |---|---|---| | |
| | **Passed** | ~150 | — | | |
| | **Failed — ExecutionError** | 91 | Papermill failed; specific errors not in log (stderr not captured) | | |
| | **Failed — PEP8** | 3 | Style violations caught by flake8 | | |
| | **Failed — MissingKeyword** | 1 | `max_epochs` not found and not exempted | | |
| | **Total failures** | **95 events / 94 notebooks** | `class_lung_lesion.ipynb` has both PEP8 + ExecutionError | |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@diagnose_1_6_release.md` around lines 14 - 20, The failure totals in the
summary table are inconsistent: the category counts and the overlap note in the
`Total failures` row do not reconcile. Update the totals in the markdown so the
notebook-level unique failure count matches the category breakdown and the
stated overlap, using the summary table entries and the `Total failures` line as
the source of truth.
Comment on lines
+259
to
+263
| ``` | ||
| ImportError: attempted relative import beyond top-level package | ||
| ``` | ||
| The error originates inside `mlflow.utils.uv_utils` which performs a relative | ||
| `from .. import zipp` that is invalid at top-level scope in Python 3.12. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Label the traceback fence.
This fenced block is missing a language tag, which triggers markdownlint (MD040).
Suggested fix
-```
+```text
ImportError: attempted relative import beyond top-level package
-```
+```📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ``` | |
| ImportError: attempted relative import beyond top-level package | |
| ``` | |
| The error originates inside `mlflow.utils.uv_utils` which performs a relative | |
| `from .. import zipp` that is invalid at top-level scope in Python 3.12. |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 259-259: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@diagnose_1_6_release.md` around lines 259 - 263, The traceback fenced block
in the diagnose_1_6_release.md section needs a language label to satisfy
markdownlint MD040. Update the fence that contains the ImportError message to
use a text-labeled code block so the traceback is properly tagged, and keep the
surrounding content unchanged.
Source: Linters/SAST tools
|
|
||
| # Get notebooks (pattern is an empty string unless the user specifies otherwise) | ||
| files=($(echo $pattern | xargs find . -type f -name "*.ipynb" -and ! -wholename "*.ipynb_checkpoints*")) | ||
| files=($(echo "$pattern" | xargs find . -type f -name "*.ipynb" -and ! -wholename "*.ipynb_checkpoints*")) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Quoting $pattern fixes one issue but files=($(...)) still splits on whitespace/globs.
The fix correctly quotes $pattern to avoid breaking on spaces within the pattern, but the outer array assignment files=($(echo ... | xargs find ...)) still performs unquoted command substitution, which is subject to word-splitting and pathname expansion on the find output (e.g., filenames with spaces will be split into multiple array elements). Shellcheck flags this (SC2207).
🛠️ Suggested fix using mapfile
-files=($(echo "$pattern" | xargs find . -type f -name "*.ipynb" -and ! -wholename "*.ipynb_checkpoints*"))
+mapfile -t files < <(echo "$pattern" | xargs find . -type f -name "*.ipynb" -and ! -wholename "*.ipynb_checkpoints*")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| files=($(echo "$pattern" | xargs find . -type f -name "*.ipynb" -and ! -wholename "*.ipynb_checkpoints*")) | |
| mapfile -t files < <(echo "$pattern" | xargs find . -type f -name "*.ipynb" -and ! -wholename "*.ipynb_checkpoints*") |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 453-453: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@runner.sh` at line 453, The `files=($(...))` assignment in `runner.sh` still
performs unsafe word-splitting and glob expansion on the `find` output, so
filenames with spaces will break. Update the array population to use a safe read
mechanism such as `mapfile`/`readarray` around the `find` command output,
keeping the existing quoted `$pattern` behavior while ensuring each path is
stored as one array element.
Source: Linters/SAST tools
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Fixes #.
Description
Fix broken notebook tests in the
modules/folder for MONAI 1.6 release validation.Changes:
modules/load_medical_images.ipynb— Replace deadmonai.ioasset download URL with the equivalent file hosted on GitHub (raw.githubusercontent.com/Project-MONAI/MONAI/dev/docs/images/MONAI-logo-color.png). Themonai.iodomain no longer serves static assets, causingdownload_url()to fail.runner.sh— Add three notebooks to theskip_run_papermilllist that cannot pass in automated CI due to execution timeouts (>10 min):class_lung_lesion— interpretability notebook that exceeds CI time limitslazy_resampling_benchmark— benchmark notebook with long-running workloadsomniverse_integration— requires Omniverse runtime not available in CIrunner.sh— Quote$patternvariable in thefindcommand to prevent word-splitting issues when the pattern contains spaces.Checks
./figurefolder./runner.sh -t modules/load_medical_images.ipynbSummary by CodeRabbit
New Features
Bug Fixes
Documentation