Fix memory leak in optional_import traceback handling#8782
Fix memory leak in optional_import traceback handling#8782aymuos15 wants to merge 1 commit intoProject-MONAI:devfrom
Conversation
…I#7480, Project-MONAI#7727) When an import fails, `optional_import` captured the live traceback object and stored it in a `_LazyRaise` closure. The traceback held references to stack frames containing CUDA tensors, preventing garbage collection and causing GPU memory leaks. Replace the live traceback with a string-formatted copy via `traceback.format_exception()` and clear the original with `import_exception.__traceback__ = None`. The formatted traceback is appended to the error message so debugging information is preserved. Also move three hot-path `optional_import` calls in `monai/transforms/utils.py` (cucim.skimage, cucim morphology EDT) from function bodies to module level, eliminating repeated leaked tracebacks on every invocation. Fixes Project-MONAI#7480, fixes Project-MONAI#7727 Signed-off-by: Soumya Snigdha Kundu <soumya_snigdha.kundu@kcl.ac.uk>
📝 WalkthroughWalkthroughThe changes refactor optional CucIM/CuPy dependency handling to prevent GPU memory leaks. Private import flags and guards are introduced in Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/utils/test_optional_import.py (1)
80-97: Prefix unused variable with underscore.Line 91:
modis never used. Rename to_modto indicate intentional discard.🔧 Suggested fix
- mod, flag = optional_import("nonexistent_module_for_leak_test") + _mod, flag = optional_import("nonexistent_module_for_leak_test")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/utils/test_optional_import.py` around lines 80 - 97, In test_no_traceback_leak, the unused variable `mod` returned from optional_import should be renamed to `_mod` to signal intentional discard; update the tuple unpacking in the _do_import function from `mod, flag = optional_import("nonexistent_module_for_leak_test")` to `_mod, flag = optional_import("nonexistent_module_for_leak_test")` (keeping the rest of test_no_traceback_leak, _Marker, and gc check unchanged).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/utils/test_optional_import.py`:
- Around line 80-97: In test_no_traceback_leak, the unused variable `mod`
returned from optional_import should be renamed to `_mod` to signal intentional
discard; update the tuple unpacking in the _do_import function from `mod, flag =
optional_import("nonexistent_module_for_leak_test")` to `_mod, flag =
optional_import("nonexistent_module_for_leak_test")` (keeping the rest of
test_no_traceback_leak, _Marker, and gc check unchanged).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7b9386de-fac6-49d8-a44b-50a0d17f850d
📒 Files selected for processing (3)
monai/transforms/utils.pymonai/utils/module.pytests/utils/test_optional_import.py
| cp, has_cp = optional_import("cupy") | ||
| cp_ndarray, _ = optional_import("cupy", name="ndarray") | ||
| exposure, has_skimage = optional_import("skimage.exposure") | ||
| _cucim_skimage, _has_cucim_skimage = optional_import("cucim.skimage") |
There was a problem hiding this comment.
I believe we had strange import issues if we had cucim at the module level, such as very slow import or some other buggy behaviour. Our solution was to put importation into functions so that this happened only when the library was needed, which was very rarely. Please undo this change but replace it with a note that cucim is deliberately not imported at the module level, there should have been one already we neglected to add in the past.
Summary
optional_importwhere stored traceback objects retain references to entire call stacks, preventing garbage collectionimport_exception.__traceback__ = Noneoptional_importcalls for cucim to module level inmonai/transforms/utils.pyFixes #7480, #7727
Details
monai/utils/module.py:__traceback__object withtraceback.format_exception()stringimport_exception.__traceback__ = Noneto break reference chainmonai/transforms/utils.py:optional_importcalls to module level (consistent with existing skimage/scipy/cupy pattern)distance_transform_edtTest plan
test_no_traceback_leak— weakref regression test for the leaktest_failed_import_shows_traceback_string— verifies traceback in error messagetest_optional_import.pytests pass