Lazily import onnx so a broken onnx does not block 'import monai' (#8455)#8937
Lazily import onnx so a broken onnx does not block 'import monai' (#8455)#8937Shizoqua wants to merge 1 commit into
Conversation
…rt monai` `monai/networks/utils.py` and `monai/bundle/scripts.py` imported `onnx`, `onnx.reference` and `onnxruntime` eagerly at module scope via `optional_import`, which calls `__import__` immediately. Because `import monai` auto-loads `monai.networks`, a broken or hanging onnx install (e.g. onnx 1.18 on Windows) takes down `import monai` with no error message. Defer these optional imports into the functions that use them (`convert_to_onnx`, `onnx_export`), matching the existing lazy pattern used for tensorrt. Importing MONAI no longer imports onnx, and the conversion paths are otherwise unchanged. Adds a regression test asserting `import monai` and `import monai.bundle` do not eagerly import onnx/onnxruntime. Fixes Project-MONAI#8455. Signed-off-by: Lanre Shittu <136805224+Shizoqua@users.noreply.github.com>
📝 WalkthroughWalkthroughModule-level Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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)
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
🧹 Nitpick comments (2)
monai/bundle/scripts.py (1)
1420-1423: ⚡ Quick winAdd a docstring to
save_onnx.
save_onnxis a new/modified definition without a Google-style docstring (Args/Returns/Raises). Please add one for guideline compliance.As per coding guidelines, "Docstrings should be present for all definition which describe each variable, return value, and raised exception in the appropriate section of the Google-style of docstrings."
🤖 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 `@monai/bundle/scripts.py` around lines 1420 - 1423, Add a Google-style docstring to the save_onnx function that is currently missing one. The docstring should include an Args section documenting the onnx_obj parameter (the ONNX object to save), the filename_prefix_or_stream parameter (the destination path or stream), and the **kwargs parameter for any additional arguments. Include a Returns section indicating None is returned, and a Raises section to document any exceptions that may be raised by the onnx.save call (such as exceptions from the optional_import or the underlying onnx library).Source: Coding guidelines
tests/networks/test_lazy_onnx_import.py (1)
50-59: ⚡ Quick winAdd Google-style docstrings to the new test methods.
_assert_not_eagerly_imported,test_import_monai_does_not_import_onnx, andtest_import_monai_bundle_does_not_import_onnxare definitions without docstrings.As per coding guidelines, "Docstrings should be present for all definition which describe each variable, return value, and raised exception in the appropriate section of the Google-style of docstrings."
🤖 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 `@tests/networks/test_lazy_onnx_import.py` around lines 50 - 59, Add Google-style docstrings to the three test methods that are currently missing them: _assert_not_eagerly_imported, test_import_monai_does_not_import_onnx, and test_import_monai_bundle_does_not_import_onnx. For _assert_not_eagerly_imported, document the import_stmt parameter and describe what the method asserts. For the two test methods, document their purpose in verifying that ONNX is not eagerly imported. Follow Google-style docstring format with Args, Returns, and Raises sections as appropriate for each method.Source: Coding guidelines
🤖 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 `@tests/networks/test_lazy_onnx_import.py`:
- Around line 50-53: Add a timeout parameter to the subprocess.run call in the
_assert_not_eagerly_imported method to prevent the test suite from hanging
indefinitely if the import operation stalls. Additionally, add PEP257-compliant
docstrings to the _assert_not_eagerly_imported helper method and to all test
methods in the test class (around lines 55-58) that currently lack them,
ensuring each docstring briefly describes what the method does.
---
Nitpick comments:
In `@monai/bundle/scripts.py`:
- Around line 1420-1423: Add a Google-style docstring to the save_onnx function
that is currently missing one. The docstring should include an Args section
documenting the onnx_obj parameter (the ONNX object to save), the
filename_prefix_or_stream parameter (the destination path or stream), and the
**kwargs parameter for any additional arguments. Include a Returns section
indicating None is returned, and a Raises section to document any exceptions
that may be raised by the onnx.save call (such as exceptions from the
optional_import or the underlying onnx library).
In `@tests/networks/test_lazy_onnx_import.py`:
- Around line 50-59: Add Google-style docstrings to the three test methods that
are currently missing them: _assert_not_eagerly_imported,
test_import_monai_does_not_import_onnx, and
test_import_monai_bundle_does_not_import_onnx. For _assert_not_eagerly_imported,
document the import_stmt parameter and describe what the method asserts. For the
two test methods, document their purpose in verifying that ONNX is not eagerly
imported. Follow Google-style docstring format with Args, Returns, and Raises
sections as appropriate for each method.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a001d9a2-1277-457f-8f82-d406bcf51f7c
📒 Files selected for processing (3)
monai/bundle/scripts.pymonai/networks/utils.pytests/networks/test_lazy_onnx_import.py
| def _assert_not_eagerly_imported(self, import_stmt: str) -> None: | ||
| code = _PROBE.format(import_stmt=import_stmt) | ||
| result = subprocess.run([sys.executable, "-c", code], capture_output=True, text=True) | ||
| self.assertEqual(result.returncode, 0, msg=(result.stdout + result.stderr).strip()) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
import ast, pathlib
p = pathlib.Path("tests/networks/test_lazy_onnx_import.py")
t = ast.parse(p.read_text())
for n in ast.walk(t):
if isinstance(n, ast.Call) and isinstance(n.func, ast.Attribute):
if isinstance(n.func.value, ast.Name) and n.func.value.id == "subprocess" and n.func.attr == "run":
has_timeout = any(isinstance(k, ast.keyword) and k.arg == "timeout" for k in n.keywords)
print(f"Line {n.lineno}: timeout_present={has_timeout}")
PYRepository: Project-MONAI/MONAI
Length of output: 94
🏁 Script executed:
cat -n tests/networks/test_lazy_onnx_import.pyRepository: Project-MONAI/MONAI
Length of output: 2879
🏁 Script executed:
#!/bin/bash
python - <<'PY'
import ast, pathlib
p = pathlib.Path("tests/networks/test_lazy_onnx_import.py")
content = p.read_text()
t = ast.parse(content)
print("=== ALL subprocess.run CALLS ===")
for n in ast.walk(t):
if isinstance(n, ast.Call) and isinstance(n.func, ast.Attribute):
if isinstance(n.func.value, ast.Name) and n.func.value.id == "subprocess" and n.func.attr == "run":
has_timeout = any(isinstance(k, ast.keyword) and k.arg == "timeout" for k in n.keywords)
print(f"Line {n.lineno}: has_timeout={has_timeout}")
print("\n=== DOCSTRINGS IN METHODS ===")
for n in ast.walk(t):
if isinstance(n, (ast.FunctionDef, ast.AsyncFunctionDef)):
has_docstring = (
n.body and isinstance(n.body[0], ast.Expr) and isinstance(n.body[0].value, ast.Constant)
)
print(f"Line {n.lineno}: {n.name} - has_docstring={has_docstring}")
PYRepository: Project-MONAI/MONAI
Length of output: 361
Add timeout to subprocess probe and docstrings to test methods.
Line 52 subprocess.run lacks a timeout parameter—if import monai hangs again, this test blocks the suite indefinitely. Additionally, the helper method _assert_not_eagerly_imported and test methods at lines 55–58 require docstrings per PEP257 guidelines.
Suggested patch
def _assert_not_eagerly_imported(self, import_stmt: str) -> None:
code = _PROBE.format(import_stmt=import_stmt)
- result = subprocess.run([sys.executable, "-c", code], capture_output=True, text=True)
+ result = subprocess.run([sys.executable, "-c", code], capture_output=True, text=True, timeout=30)
self.assertEqual(result.returncode, 0, msg=(result.stdout + result.stderr).strip())🧰 Tools
🪛 ast-grep (0.43.0)
[error] 51-51: Command coming from incoming request
Context: subprocess.run([sys.executable, "-c", code], capture_output=True, text=True)
Note: [CWE-20].
(subprocess-from-request)
🪛 Ruff (0.15.17)
[error] 52-52: subprocess call: check for execution of untrusted input
(S603)
🤖 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 `@tests/networks/test_lazy_onnx_import.py` around lines 50 - 53, Add a timeout
parameter to the subprocess.run call in the _assert_not_eagerly_imported method
to prevent the test suite from hanging indefinitely if the import operation
stalls. Additionally, add PEP257-compliant docstrings to the
_assert_not_eagerly_imported helper method and to all test methods in the test
class (around lines 55-58) that currently lack them, ensuring each docstring
briefly describes what the method does.
1 participant
Description
Fixes #8455. Installing a broken/hanging onnx (e.g.
onnx==1.18.0on Windows) makesimport monaifail with no error message.Root cause:
monai/networks/utils.pyandmonai/bundle/scripts.pycalloptional_import("onnx")(andonnx.reference/onnxruntime) at module scope.optional_importimports eagerly (__import__), andimport monaiauto-loadsmonai.networks, so importing MONAI unconditionally imports onnx — inheriting any onnx import failure/hang.This defers those optional imports into the functions that actually use them (
convert_to_onnxinnetworks/utils.py,onnx_export'ssave_onnxinbundle/scripts.py), matching the lazy pattern already used fortensorrtin the same file. After this change, importing MONAI no longer imports onnx; the ONNX conversion code paths are unchanged.Types of changes
Testing
Added
tests/networks/test_lazy_onnx_import.py, which (in a subprocess, with a meta-path recorder) asserts thatimport monaiandimport monai.bundledo not import onnx/onnxruntime — verified passing with onnx installed. The existingtests/networks/test_convert_to_onnx.pycontinues to exercise the conversion paths in CI.