fix: update runner.sh and endoscopic notebook for MONAI 1.6 tutorial runner compatibility#2065
Open
garciadias wants to merge 9 commits into
Open
fix: update runner.sh and endoscopic notebook for MONAI 1.6 tutorial runner compatibility#2065garciadias wants to merge 9 commits into
garciadias wants to merge 9 commits into
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRunner papermill controls updated to exempt two notebooks and skip one; a spleen notebook updates its PyTorch Lightning pin and fixes a JSON outputs block; several notebooks have cosmetic f-string spacing edits; a detailed diagnostics report document is added. ChangesNotebook Processing Configuration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
🤖 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 `@runner.sh`:
- Around line 87-88: The doesnt_contain_max_epochs array entry is incorrect
because hovernet_infer_compare.ipynb does not exist; update the array (variable
doesnt_contain_max_epochs) to reference the actual notebook that lacks
max_epochs — replace hovernet_infer_compare.ipynb with
monailabel/monailabel_pathology_HoVerNet_QuPath.ipynb (or remove the nonexistent
entry) so the basename comparison ([[ "$e" == "$filename" ]]) can correctly
match and exempt the HoverNet inference notebook; ensure the
msd_crossval_datalist_generator.ipynb entry remains unchanged.
🪄 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: 790edfde-41e1-40f1-9d11-a3eb3d3813d6
📒 Files selected for processing (2)
computer_assisted_intervention/endoscopic_inbody_classification.ipynbrunner.sh
👮 Files not reviewed due to content moderation or server errors (1)
- computer_assisted_intervention/endoscopic_inbody_classification.ipynb
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
bundle/05_spleen_segmentation_lightning.ipynb (1)
41-41: ⚡ Quick winCritical fix for Python 3.12 mlflow import chain failure — well documented.
This change resolves the issue where pytorch-lightning 2.0.x eagerly imports mlflow at module level, causing failures under Python 3.12. The
>=2.1constraint ensures lazy mlflow imports are used.Optional: Consider a more conservative version constraint
The current constraint
>=2.1permits any future major version (3.x, 4.x, etc.), which could introduce breaking changes. For better stability, consider:-!pip install -q pytorch-lightning>=2.1 +!pip install -q "pytorch-lightning>=2.1,<3"However, since this is tutorial code and has been tested with 2.6.5, the current permissive constraint is acceptable.
🤖 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 `@bundle/05_spleen_segmentation_lightning.ipynb` at line 41, Replace the notebook installation line string "!pip install -q pytorch-lightning>=2.1" with a constrained version to avoid Python 3.12 mlflow import issues; update the command in the cell that currently contains "!pip install -q pytorch-lightning>=2.1" to either "!pip install -q 'pytorch-lightning>=2.1,<3.0'" for a conservative pin or keep ">=2.1" if you accept the permissive range, ensuring the cell is updated accordingly.diagnose_1_6_release.md (2)
259-263: 💤 Low valueAdd language identifier to fenced code block.
The error message block should specify a language for proper syntax highlighting and markdown validation.
📝 Suggested fix
`mlflow 3.13.0` (installed in the image) fails on Python 3.12 with: -``` +```python ImportError: attempted relative import beyond top-level package</details> <details> <summary>🤖 Prompt for AI Agents</summary>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.mdaround lines 259 - 263, Update the fenced code block
that contains the ImportError message so it includes a language identifier;
replace the opening "" before the "ImportError: attempted relative import beyond top-level package" line with "python" (the block referencing
mlflow.utils.uv_utils and the relative import error) to enable proper syntax
highlighting and markdown validation.</details> <!-- cr-comment:v1:a0bf915188f7b1372abe3425 --> --- `54-66`: _⚡ Quick win_ **Minor documentation gap: second notebook not listed.** The PR objectives state that both `msd_crossval_datalist_generator.ipynb` and `hovernet_infer_compare.ipynb` were added to the `doesnt_contain_max_epochs` exemption list, but this Category 2 section only documents `msd_crossval_datalist_generator.ipynb`. Line 438 in the "Fixes Applied" section confirms both notebooks were added to the exemption list. Consider adding `hovernet_infer_compare.ipynb` to the table in this section for completeness, or add a note explaining why it's not listed here (e.g., if it didn't fail in the initial run). <details> <summary>🤖 Prompt for AI Agents</summary> ``` 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 54 - 66, The Category 2 — MissingKeyword table is missing hovernet_infer_compare.ipynb even though the "Fixes Applied" (line ~438) shows both msd_crossval_datalist_generator.ipynb and hovernet_infer_compare.ipynb were added to the doesnt_contain_max_epochs exemption; update the Category 2 table to include hovernet_infer_compare.ipynb with the same detail entry (or add a clarifying note why it was omitted), and ensure the documentation references the runner.sh doesnt_contain_max_epochs exemption so readers can reconcile the table with the applied fixes. ``` </details> <!-- cr-comment:v1:7bda2fd71a0aac0f57c28c85 --> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>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 444-450: Remove the duplicate "PEP8 in 3 notebooks" entry from the
"Still pending" section and ensure the "Recommended Next Steps" section keeps
the existing "PEP8 autofix — all three notebooks autofixed withrunner.sh --autofix. ✓ DONE" text; specifically delete the table row or bullet that
contains the exact phrase "PEP8 in 3 notebooks" (and any associated action text)
so the document no longer shows PEP8 fixes as both pending and done, keeping
references to the affected notebooks
(surgtoolloc/preprocess_detect_scene_and_split_fold.ipynb,
deep_atlas/deep_atlas_tutorial.ipynb,
modules/interpretability/class_lung_lesion.ipynb) only in the completed note
if needed.
Nitpick comments:
In@bundle/05_spleen_segmentation_lightning.ipynb:
- Line 41: Replace the notebook installation line string "!pip install -q
pytorch-lightning>=2.1" with a constrained version to avoid Python 3.12 mlflow
import issues; update the command in the cell that currently contains "!pip
install -q pytorch-lightning>=2.1" to either "!pip install -q
'pytorch-lightning>=2.1,<3.0'" for a conservative pin or keep ">=2.1" if you
accept the permissive range, ensuring the cell is updated accordingly.In
@diagnose_1_6_release.md:
- Around line 259-263: Update the fenced code block that contains the
ImportError message so it includes a language identifier; replace the opening
"" before the "ImportError: attempted relative import beyond top-level package" line with "python" (the block referencing mlflow.utils.uv_utils and
the relative import error) to enable proper syntax highlighting and markdown
validation.- Around line 54-66: The Category 2 — MissingKeyword table is missing
hovernet_infer_compare.ipynb even though the "Fixes Applied" (line ~438) shows
both msd_crossval_datalist_generator.ipynb and hovernet_infer_compare.ipynb were
added to the doesnt_contain_max_epochs exemption; update the Category 2 table to
include hovernet_infer_compare.ipynb with the same detail entry (or add a
clarifying note why it was omitted), and ensure the documentation references the
runner.sh doesnt_contain_max_epochs exemption so readers can reconcile the table
with the applied fixes.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Organization UI **Review profile**: CHILL **Plan**: Pro **Run ID**: `a3e94dda-8fe0-49e3-9b18-8009fe13d5e2` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 22cb7bc89c659084d8288f01d6c74391d2e057e6 and f2e31eadb78677e35c1bb004cc6de52a66aaf3a7. </details> <details> <summary>📒 Files selected for processing (5)</summary> * `bundle/05_spleen_segmentation_lightning.ipynb` * `competitions/MICCAI/surgtoolloc/preprocess_detect_scene_and_split_fold.ipynb` * `deep_atlas/deep_atlas_tutorial.ipynb` * `diagnose_1_6_release.md` * `modules/interpretability/class_lung_lesion.ipynb` </details> <details> <summary>✅ Files skipped from review due to trivial changes (2)</summary> * competitions/MICCAI/surgtoolloc/preprocess_detect_scene_and_split_fold.ipynb * deep_atlas/deep_atlas_tutorial.ipynb </details> <details> <summary>👮 Files not reviewed due to content moderation or server errors (1)</summary> * modules/interpretability/class_lung_lesion.ipynb </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
…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>
garciadias
force-pushed
the
fix/monai_1_6_release_notebooks
branch
from
9f83ff4 to
f3f49ff
Compare
Signed-off-by: R. Garcia-Dias <rafaelagd@gmail.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
diagnose_1_6_release.md (1)
452-452:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDuplicate finding: PEP8 entry should be removed from "Still pending" section.
Line 452 lists "PEP8 in 3 notebooks" as an action still needed, but lines 398 and 420 both mark PEP8 autofix as "✓ DONE". The review stack context confirms that PEP8 f-string spacing fixes were applied to all three notebooks in this PR.
This inconsistency was already flagged in a previous review comment.
🤖 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` at line 452, Remove the duplicate "PEP8 in 3 notebooks | Run `bash runner.sh --autofix` for `surgtoolloc/preprocess_detect_scene_and_split_fold.ipynb`, `deep_atlas/deep_atlas_tutorial.ipynb`, `modules/interpretability/class_lung_lesion.ipynb`" row from the "Still pending" section in diagnose_1_6_release.md (the entry that lists "PEP8 in 3 notebooks"), since those notebooks are already marked "✓ DONE" elsewhere; keep the completed entries and ensure only the completed PEP8 autofix rows remain.
🧹 Nitpick comments (1)
diagnose_1_6_release.md (1)
259-261: 💤 Low valueAdd language specifier to fenced code block.
The fenced code block at line 259 is missing a language specifier. Consider adding
textorpythonto improve readability and satisfy Markdown linting rules.📝 Proposed fix
-``` +```text ImportError: attempted relative import beyond top-level package</details> <details> <summary>🤖 Prompt for AI Agents</summary>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.mdaround lines 259 - 261, The fenced code block
containing "ImportError: attempted relative import beyond top-level package" is
missing a language specifier; update the opening fence fromtotext (orchange the opening fence to ```text and leave the content and closing fence unchanged).Source: Linters/SAST tools
🤖 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.
Duplicate comments:
In `@diagnose_1_6_release.md`:
- Line 452: Remove the duplicate "PEP8 in 3 notebooks | Run `bash runner.sh
--autofix` for `surgtoolloc/preprocess_detect_scene_and_split_fold.ipynb`,
`deep_atlas/deep_atlas_tutorial.ipynb`,
`modules/interpretability/class_lung_lesion.ipynb`" row from the "Still pending"
section in diagnose_1_6_release.md (the entry that lists "PEP8 in 3 notebooks"),
since those notebooks are already marked "✓ DONE" elsewhere; keep the completed
entries and ensure only the completed PEP8 autofix rows remain.
---
Nitpick comments:
In `@diagnose_1_6_release.md`:
- Around line 259-261: The fenced code block containing "ImportError: attempted
relative import beyond top-level package" is missing a language specifier;
update the opening fence from ``` to ```text (or ```python if preferred) so the
block becomes a labeled fenced code block (e.g., change the opening fence to
```text and leave the content and closing fence unchanged).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0bb72274-f31b-4261-9885-6fd6814a3de1
📒 Files selected for processing (7)
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.ipynbrunner.sh
✅ Files skipped from review due to trivial changes (1)
- deep_atlas/deep_atlas_tutorial.ipynb
🚧 Files skipped from review as they are similar to previous changes (3)
- runner.sh
- bundle/05_spleen_segmentation_lightning.ipynb
- competitions/MICCAI/surgtoolloc/preprocess_detect_scene_and_split_fold.ipynb
👮 Files not reviewed due to content moderation or server errors (2)
- modules/interpretability/class_lung_lesion.ipynb
- computer_assisted_intervention/endoscopic_inbody_classification.ipynb
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>
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
1 participant
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.
Description
Fixes tutorial runner failures observed during MONAI 1.6 release testing (Docker image, Python 3.12, RTX 5090). Full diagnostics in
diagnose_1_6_release.md.Failures addressed
mlflow 3.xfails on Python 3.12 (from .. import zipprelative import)mlflow<3.0inrequirements-dev.txttransformers 5.xreferencestorch.float8_e8m0fnuabsent in the base PyTorch build2d_regression/image_restoration.ipynb(transitively)transformers<5.0inrequirements-dev.txtmonai.networks.nets.restormernot present in this branch2d_regression/image_restoration.ipynbskip_run_papermillinrunner.shmonai.bundle.load()returnsOrderedDictin older local branch — local-only issue; upstream MONAI ≥1.5 already returnsnn.Moduleby defaultcomputer_assisted_intervention/endoscopic_inbody_classification.ipynbreturn_state_dict=Falsekwarg; original notebook is correctaimpackage not installed in the Docker imagemodules/spleen_segmentation_aim.ipynbaimtorequirements-dev.txtpytorch-lightning~=2.0.0(resolves to 2.0.9) eagerly importsmlflowat module level;mlflow 3.xfails on Python 3.12 with a relative-import errorbundle/05_spleen_segmentation_lightning.ipynbpytorch-lightning>=2.1in notebook (removes the eager mlflow import; tested with 2.6.5)mlflow<3.0as a safety net; this PR removes the fragile import dependency entirelymax_epochs; two notebooks have nonebundle/msd_crossval_datalist_generator.ipynb,pathology/hovernet_infer_compare.ipynbdoesnt_contain_max_epochsexemption list inrunner.shdeep_atlas_tutorial.ipynband05_spleen_segmentation_lightning.ipynbhardcodedevice = torch.device("cuda:0")/"cuda"with no CPU fallback; fail on the CPU-only GitHub Actions runnerdeep_atlas/deep_atlas_tutorial.ipynb,bundle/05_spleen_segmentation_lightning.ipynbskip_run_papermillinrunner.shpreprocess_detect_scene_and_split_fold.ipynb,deep_atlas_tutorial.ipynb,class_lung_lesion.ipynbautopep8autofix (no functional changes)Checks
./figurefolder./runner.sh -t <path to .ipynb file>— GPU-dependent notebooks (05_spleen_segmentation_lightning,deep_atlas_tutorial) require a CUDA device; they are skipped in the CPU CI runner and verified manually on a local GPU node.