Updated code due to dependencies' API changes#583
Conversation
Signed-off-by: M Q <mingmelvinq@nvidia.com>
|
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:
WalkthroughAdds runtime holoscan init generation and graphs import fallback for Holoscan layout changes, pins Holoscan deps, clears notebook execution outputs, and updates notebook packaging to compute holoscan_cli manifest URLs and include --cuda 12. ChangesHoloscan 4.1+ compatibility and packaging updates
Sequence DiagramsequenceDiagram
participant NotebookClient
participant holoscan_cli
participant monai_deploy
participant HoloscanManifest as HoloscanCLIManifest
NotebookClient->>holoscan_cli: import __version__
holoscan_cli-->>NotebookClient: version string
NotebookClient->>NotebookClient: build manifest_url from version
NotebookClient->>monai_deploy: call package --cuda 12 --source {manifest_url}
monai_deploy->>HoloscanManifest: fetch manifest_url
HoloscanManifest-->>monai_deploy: return manifest/config
monai_deploy-->>NotebookClient: produce packaged image
🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers:
🚥 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 |
There was a problem hiding this comment.
Pull request overview
This PR updates dependency constraints and adjusts SDK integration points and tutorial notebooks to accommodate upstream API/layout changes in Holoscan (notably v4.1+) and related tooling.
Changes:
- Pin
holoscan-cu12andholoscan-clito a compatible version range (>=4.0.0,<4.3.0) and update example requirements for nvimgcodec extras. - Update tutorial notebooks to use a holoscan-cli release-tag manifest URL and pass
--cuda/--sourcewhen packaging. - Add Holoscan v4.1 compatibility shims by importing from
holoscan.flow_graphswith fallback toholoscan.graphsand aliasingFlowGraph.
Reviewed changes
Copilot reviewed 7 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| requirements.txt | Pins Holoscan dependencies to an explicit compatible range. |
| requirements-examples.txt | Adjusts nvimgcodec dependency to include extras. |
| notebooks/tutorials/05_multi_model_app.ipynb | Updates packaging command to use a holoscan-cli release manifest URL; clears cell outputs. |
| notebooks/tutorials/02_mednist_app.ipynb | Updates packaging command to use a holoscan-cli release manifest URL; clears cell outputs. |
| notebooks/tutorials/02_mednist_app-prebuilt.ipynb | Updates packaging command to use a holoscan-cli release manifest URL; clears cell outputs. |
| monai/deploy/utils/importutil.py | Updates Holoscan lazy-module patch content to include flow_graphs/graphs. |
| monai/deploy/graphs/init.py | Adds Holoscan v4.1+ import fallback (flow_graphs → graphs) and FlowGraph aliasing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
monai/deploy/utils/importutil.py (1)
437-465: ⚡ Quick winLazy module loading may fail for version-specific modules.
The
_EXTRA_MODULESlist now includes both"flow_graphs"(holoscan >= 4.1.0) and"graphs"(holoscan < 4.1.0), but only one of these modules exists in any given holoscan version. The__getattr__function (lines 455-465) will attempt to import whichever module is accessed, but doesn't catchModuleNotFoundErrorwhen the module doesn't exist.For example, if code tries to access
holoscan.graphswith holoscan 4.1.0+ installed, line 461'simportlib.import_module(module_name)will raiseModuleNotFoundError, which is not caught.This may be intentional (letting import errors propagate naturally), but consider whether the lazy loader should:
- Catch
ModuleNotFoundErrorand provide a more informative error message indicating version compatibility- Dynamically filter
_EXTRA_MODULESbased on available modules to prevent failed imports from appearing in__dir__autocomplete💡 Optional: Add version-aware error handling
def __getattr__(name): import importlib import sys if name in _EXTRA_MODULES: module_name = f"{__name__}.{name}" - module = importlib.import_module(module_name) # import - sys.modules[module_name] = module # cache - return module + try: + module = importlib.import_module(module_name) # import + sys.modules[module_name] = module # cache + return module + except ModuleNotFoundError: + # Provide version-specific guidance + if name == "flow_graphs": + raise AttributeError(f"module {__name__} has no attribute {name}. " + f"flow_graphs requires holoscan >= 4.1.0") from None + elif name == "graphs": + raise AttributeError(f"module {__name__} has no attribute {name}. " + f"graphs is only available in holoscan < 4.1.0") from None + raise else: raise AttributeError(f"module {__name__} has no attribute {name}")🤖 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/deploy/utils/importutil.py` around lines 437 - 465, The lazy loader can raise ModuleNotFoundError for version-specific names in _EXTRA_MODULES (e.g., "flow_graphs" vs "graphs"); update __getattr__ to catch ModuleNotFoundError around importlib.import_module(module_name) and re-raise a clearer AttributeError that mentions the missing module and the holoscan version compatibility (or suggest the alternative module name), and also update __dir__ (or compute a runtime-visible list) to filter _EXTRA_MODULES to only include modules that can be imported so autocomplete does not advertise unavailable names; reference __getattr__, __dir__, _EXTRA_MODULES and the importlib.import_module call when making the changes.
🤖 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.
Nitpick comments:
In `@monai/deploy/utils/importutil.py`:
- Around line 437-465: The lazy loader can raise ModuleNotFoundError for
version-specific names in _EXTRA_MODULES (e.g., "flow_graphs" vs "graphs");
update __getattr__ to catch ModuleNotFoundError around
importlib.import_module(module_name) and re-raise a clearer AttributeError that
mentions the missing module and the holoscan version compatibility (or suggest
the alternative module name), and also update __dir__ (or compute a
runtime-visible list) to filter _EXTRA_MODULES to only include modules that can
be imported so autocomplete does not advertise unavailable names; reference
__getattr__, __dir__, _EXTRA_MODULES and the importlib.import_module call when
making the changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2ff2d8c8-566d-44bd-ba3c-a34d24c37ad2
📒 Files selected for processing (10)
monai/deploy/graphs/__init__.pymonai/deploy/utils/importutil.pynotebooks/tutorials/01_simple_app.ipynbnotebooks/tutorials/02_mednist_app-prebuilt.ipynbnotebooks/tutorials/02_mednist_app.ipynbnotebooks/tutorials/03_segmentation_app.ipynbnotebooks/tutorials/04_monai_bundle_app.ipynbnotebooks/tutorials/05_multi_model_app.ipynbrequirements-examples.txtrequirements.txt
Signed-off-by: M Q <mingmelvinq@nvidia.com>
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 `@monai/deploy/utils/importutil.py`:
- Around line 414-418: The _holoscan_package_path function currently imports
holoscan which forces package init to run; change it to locate the installed
package directory without importing by using importlib.util.find_spec (or
importlib.machinery.ModuleSpec): call importlib.util.find_spec("holoscan"), then
if spec.submodule_search_locations exists use the first entry as the package
directory, otherwise use Path(spec.origin).resolve().parent; return that Path
and raise a clear error if find_spec returns None. Update callers such as
fix_holoscan_import to use the new behavior.
🪄 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: 73d2d66d-0463-442c-b904-1252b85d7604
📒 Files selected for processing (2)
monai/deploy/graphs/__init__.pymonai/deploy/utils/importutil.py
Signed-off-by: M Q <mingmelvinq@nvidia.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
monai/deploy/utils/importutil.py (1)
496-521:⚠️ Potential issue | 🟠 Major | ⚡ Quick winError handling bug: exceptions are treated as success.
fix_holoscan_import()returns the exception object on failure (line 508), but the CLI code at line 517 checksif file_path:. Since exception objects are truthy, errors are incorrectly treated as success—the exception gets printed to stdout and the script exits with code 0.This silently masks failures in the
runscript which captures stdout and assumes success.🐛 Proposed fix
def fix_holoscan_import(): """Fix holoscan __init__ to enable lazy load for avoiding failure on loading low level libs.""" try: holoscan_pkg_path = _holoscan_package_path() holoscan_init_path = holoscan_pkg_path / "__init__.py" extra_modules = _build_holoscan_extra_modules(holoscan_pkg_path) with open(str(holoscan_init_path), "w") as f_w: f_w.write(_build_holoscan_init_content(extra_modules)) return str(holoscan_init_path) except Exception as ex: - return ex + return None # Or: raise to propagate the error if __name__ == "__main__": """Utility functions that can be used in the command line.""" argv = sys.argv if len(argv) == 2 and argv[1] == "fix_holoscan_import": file_path = fix_holoscan_import() - if file_path: + if file_path and isinstance(file_path, str): print(file_path) sys.exit(0) else: sys.exit(1)Alternatively, consider re-raising the exception so the caller can see the actual error message:
except Exception as ex: - return ex + print(f"Error: {ex}", file=sys.stderr) + return None🤖 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/deploy/utils/importutil.py` around lines 496 - 521, The CLI treats any truthy return from fix_holoscan_import() as success, and fix_holoscan_import currently returns an Exception object on error; change fix_holoscan_import() so it does not return exceptions (either re-raise the caught exception with raise or return None), and update the __main__ CLI block to call fix_holoscan_import() inside a try/except that prints the real error to stderr and exits with non-zero status on failure; specifically modify fix_holoscan_import() to remove "return ex" and use "raise" (or "return None"), and update the main logic that calls fix_holoscan_import() to catch Exception as e, print the error (to stderr), and sys.exit(1) so failures are not treated as success.
🤖 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.
Outside diff comments:
In `@monai/deploy/utils/importutil.py`:
- Around line 496-521: The CLI treats any truthy return from
fix_holoscan_import() as success, and fix_holoscan_import currently returns an
Exception object on error; change fix_holoscan_import() so it does not return
exceptions (either re-raise the caught exception with raise or return None), and
update the __main__ CLI block to call fix_holoscan_import() inside a try/except
that prints the real error to stderr and exits with non-zero status on failure;
specifically modify fix_holoscan_import() to remove "return ex" and use "raise"
(or "return None"), and update the main logic that calls fix_holoscan_import()
to catch Exception as e, print the error (to stderr), and sys.exit(1) so
failures are not treated as success.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 56faac18-9dcf-46c4-b29d-8d72fd3dd7c0
📒 Files selected for processing (3)
monai/deploy/graphs/__init__.pymonai/deploy/operators/monet_bundle_inference_operator.pymonai/deploy/utils/importutil.py
✅ Files skipped from review due to trivial changes (1)
- monai/deploy/operators/monet_bundle_inference_operator.py
🚧 Files skipped from review as they are similar to previous changes (1)
- monai/deploy/graphs/init.py
Signed-off-by: M Q <mingmelvinq@nvidia.com>
Signed-off-by: M Q <mingmelvinq@nvidia.com>
Signed-off-by: M Q <mingmelvinq@nvidia.com>
chezhia
left a comment
chezhia
left a comment
There was a problem hiding this comment.
The changes look consistent and should be a stable solution until Holoscan cli 4.3.0 is released.
… resource path was restored Signed-off-by: M Q <mingmelvinq@nvidia.com>
@chezhia Thanks for the review! It has become clear that the holoscan-cli will cease to support packaging MAP/HAP starting in v4.3, and correspondingly only holoscan SDK versions up to v4.2 will be supported. Given that holoscan SDK and holoscan-cli have long term support by NVIDIA, MONAI Deploy App SDK should still work for a long time. Also, I raised an issue on the missing holoscan-cli manifest file at the original path in its repo, and it has been addressed, so there is no need to explicitly specify |
There was a problem hiding this comment.
Looks good to me as well. Thanks for the notification regarding holoscan-cli deprecation of HAP/MAP support.
Any concerns that seemingly only artifacts for holoscan==4.2.0 will be supported in the holoscan-cli repo? Legacy artifacts were removed in this commit. Pinning to holoscan-cli<=4.2.0 makes sense, but would MAP builds with holoscan-cli<4.2.0 fail because artifacts are not available to be pulled?
Tried building a MAP with holoscan and holoscan-cli == 3.10.0, getting the below error when trying to fetch from GitHub and grab artifact after initiating a MAP build. Worked-around by downloading holoscan-cli==3.10.0 artifacts-cu12.json locally and editing artifact_sources.py to fetch locally instead of from GitHub. Not sure if we could fix on App SDK side, if we want to address we probably need holoscan-cli alignment.
[2026-06-08 13:42:47,920] [INFO] (common) - Downloading CLI manifest file from https://raw.githubusercontent.com/nvidia-holoscan/holoscan-cli/refs/heads/main/releases/3.10.0/artifacts-cu12.json...
[2026-06-08 13:42:48,164] [DEBUG] (packager) - Error downloading manifest file from https://raw.githubusercontent.com/nvidia-holoscan/holoscan-cli/refs/heads/main/releases/3.10.0/artifacts-cu12.json: Not Found
Traceback (most recent call last):
File "/home/bluna301/miniconda3/envs/ped-abd-ct-seg/lib/python3.10/site-packages/holoscan_cli/common/artifact_sources.py", line 134, in _download_manifest_internal
manifest.raise_for_status()
File "/home/bluna301/miniconda3/envs/ped-abd-ct-seg/lib/python3.10/site-packages/requests/models.py", line 1026, in raise_for_status
raise HTTPError(http_error_msg, response=self)
requests.exceptions.HTTPError: 404 Client Error: Not Found for url: https://raw.githubusercontent.com/nvidia-holoscan/holoscan-cli/refs/heads/main/releases/3.10.0/artifacts-cu12.json
The above exception was the direct cause of the following exception:
Traceback (most recent call last):
File "/home/bluna301/miniconda3/envs/ped-abd-ct-seg/lib/python3.10/site-packages/holoscan_cli/packager/packager.py", line 120, in execute_package_command
_package_application(args)
File "/home/bluna301/miniconda3/envs/ped-abd-ct-seg/lib/python3.10/site-packages/holoscan_cli/packager/packager.py", line 91, in _package_application
packaging_args = PackagingArguments(args, temp_dir)
File "/home/bluna301/miniconda3/envs/ped-abd-ct-seg/lib/python3.10/site-packages/holoscan_cli/packager/arguments.py", line 62, in __init__
self._artifact_sources.download_manifest()
File "/home/bluna301/miniconda3/envs/ped-abd-ct-seg/lib/python3.10/site-packages/holoscan_cli/common/artifact_sources.py", line 125, in download_manifest
self._download_manifest_internal(
File "/home/bluna301/miniconda3/envs/ped-abd-ct-seg/lib/python3.10/site-packages/holoscan_cli/common/artifact_sources.py", line 136, in _download_manifest_internal
raise ManifestDownloadError(
holoscan_cli.common.exceptions.ManifestDownloadError: Error downloading manifest file from https://raw.githubusercontent.com/nvidia-holoscan/holoscan-cli/refs/heads/main/releases/3.10.0/artifacts-cu12.json: Not Found
[2026-06-08 13:42:48,182] [ERROR] (packager) - Error packaging application:
Error downloading manifest file from https://raw.githubusercontent.com/nvidia-holoscan/holoscan-cli/refs/heads/main/releases/3.10.0/artifacts-cu12.json: Not Found
Thanks Bryan @bluna301 ! Yes, on the same page with regard to the hotfix only works for holoscan-cli v4.2.0. I'd requested a v4.2.0 patch release to properly update the holoscan-cli code to work with the newly changed resource locations for all CLI releases, and it was agreed and noted as follow-up in the hotfix PR. In the meantime, MONAI Deploy users need to pin holoscan-cli and holoscan at v4.2.0. We'll also need to release a new version of the MONAI Deploy App SDK containing the update in this and your open PRs ASAP. |
|
Looks good to me as well.
Perfect, thanks @MMelQin! On the same page regarding short and long term fixes here. |
and simplified the requirements for nvidia-nvimgcodec Signed-off-by: M Q <mingmelvinq@nvidia.com>
|
|
I've also updated the README to clarify the highest compatible holoscan-cli and holoscan versions. |
Update the code due to changes in the following dependencies:
Summary by CodeRabbit
Chores
Documentation
Bug Fixes