Remove the use of deprecated pkg_resources in the importutil and tested compatibility with HSDK v4.0 CUDA 12#579
Remove the use of deprecated pkg_resources in the importutil and tested compatibility with HSDK v4.0 CUDA 12#579
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:
WalkthroughMigrates distribution discovery from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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.
Pull request overview
This PR removes the deprecated pkg_resources dependency from monai.deploy.utils.importutil by switching to importlib.metadata, and updates the segmentation tutorial notebook outputs to reflect running with Holoscan SDK v4.0.
Changes:
- Replaced
pkg_resources.working_setusage with animportlib.metadata-based distribution index and adapter. - Updated tutorial notebook install cell and re-ran the notebook, capturing new runtime/build logs (including Holoscan SDK v4.0 output).
Reviewed changes
Copilot reviewed 1 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
monai/deploy/utils/importutil.py |
Removes pkg_resources usage by introducing importlib.metadata distribution lookup and a small adapter layer. |
notebooks/tutorials/03_segmentation_app.ipynb |
Updates tutorial execution outputs and environment/build logs after re-running under a newer Holoscan SDK stack. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
![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 (1)
monai/deploy/utils/importutil.py (1)
58-63: Delegate to the stdlibrequiresproperty instead of reparsing metadata.
importlib.metadata.Distributionalready exposesrequiresas a public property. Re-readingRequires-Distmanually adds another behavior surface to keep in sync for no real gain. (docs.python.org)Proposed simplification
def requires(self) -> List[str]: - meta = self._dist.metadata - if hasattr(meta, "get_all"): - reqs = meta.get_all("Requires-Dist") - return list(reqs) if reqs else [] - return [] + return list(self._dist.requires or [])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monai/deploy/utils/importutil.py` around lines 58 - 63, The requires() method re-parses metadata instead of using importlib.metadata.Distribution's built-in requires property; change the implementation of requires (in the class that currently uses self._dist.metadata and meta.get_all("Requires-Dist")) to simply return self._dist.requires or an empty list if None, preserving the return type as List[str] — i.e., replace the manual metadata inspection with a direct call to the Distribution.requires attribute in the requires() method.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@monai/deploy/utils/importutil.py`:
- Around line 22-24: The _normalize_project_name function currently only
lowercases and replaces underscores; change it to implement PEP 503
normalization by lowercasing the input and collapsing every run of '.', '_' and
'-' into a single '-' (e.g., use a regex like re.sub(r"[-_.]+", "-",
name.lower())); update the implementation of _normalize_project_name accordingly
so calls from _get_working_set, is_dist_editable, dist_module_path,
is_module_installed, and dist_requires will get the corrected normalized names.
---
Nitpick comments:
In `@monai/deploy/utils/importutil.py`:
- Around line 58-63: The requires() method re-parses metadata instead of using
importlib.metadata.Distribution's built-in requires property; change the
implementation of requires (in the class that currently uses self._dist.metadata
and meta.get_all("Requires-Dist")) to simply return self._dist.requires or an
empty list if None, preserving the return type as List[str] — i.e., replace the
manual metadata inspection with a direct call to the Distribution.requires
attribute in the requires() 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 47ee1ca0-c265-464b-997b-0bf98aed3202
📒 Files selected for processing (2)
monai/deploy/utils/importutil.pynotebooks/tutorials/03_segmentation_app.ipynb
Signed-off-by: M Q <mingmelvinq@nvidia.com>
Signed-off-by: M Q <mingmelvinq@nvidia.com>
Signed-off-by: M Q <mingmelvinq@nvidia.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
monai/deploy/utils/importutil.py (1)
33-40: Cache working-set construction to avoid repeated full environment scans.
_get_working_set()is called by multiple helpers and currently re-enumerates distributions each time. Caching this map once per process would reduce repeated overhead.♻️ Proposed refactor
+from functools import lru_cache from importlib import import_module from importlib.metadata import distributions @@ +@lru_cache(maxsize=1) def _get_working_set() -> Dict[str, Any]: """Build a dict of distribution name -> dist-like object using importlib.metadata.""" result: Dict[str, Any] = {} for d in distributions(): key = _normalize_project_name(d.name) result[key] = _DistributionAdapter(d) return result🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monai/deploy/utils/importutil.py` around lines 33 - 40, _cache a single process-wide working-set dict so repeated calls to _get_working_set() do not re-scan distributions: add a module-level cache (e.g., _WORKING_SET_CACHE = None) and change _get_working_set() to return the cached dict if present; on first call build the map by iterating distributions(), using _normalize_project_name(d.name) as the key and wrapping values with _DistributionAdapter(d), store that result in the cache and return it. Ensure subsequent callers of _get_working_set() (and helpers that rely on it) receive the same cached dict.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@monai/deploy/utils/importutil.py`:
- Around line 33-40: _cache a single process-wide working-set dict so repeated
calls to _get_working_set() do not re-scan distributions: add a module-level
cache (e.g., _WORKING_SET_CACHE = None) and change _get_working_set() to return
the cached dict if present; on first call build the map by iterating
distributions(), using _normalize_project_name(d.name) as the key and wrapping
values with _DistributionAdapter(d), store that result in the cache and return
it. Ensure subsequent callers of _get_working_set() (and helpers that rely on
it) receive the same cached dict.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 20cd0582-4cf6-4e3e-a5ee-95b9e81e6569
📒 Files selected for processing (1)
monai/deploy/utils/importutil.py
Signed-off-by: M Q <mingmelvinq@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
monai/deploy/utils/importutil.py (1)
45-48: Switch to public APIs to avoid relying onimportlib.metadata.Distribution._path.Lines 45–48 and downstream usage (lines 55–62, 343–390) depend on the private
_pathattribute. This is undocumented and treated internally by pip as a hack; it may change or be absent in future Python versions or alternative distribution providers, causing editable detection to fail.Recommended approach:
- Replace direct
open()calls todirect_url.json(lines 361, 375) withDistribution.read_text("direct_url.json"), which returnsNoneif absent- Remove or refactor the
egg_infoproperty to use public metadata APIs- If filesystem paths are needed, use
importlib.metadata.files(name)+PackagePath.locate()instead of_pathThis ensures compatibility across Python 3.10–3.12 and maintains forward compatibility.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monai/deploy/utils/importutil.py` around lines 45 - 48, The code currently relies on the private Distribution._path (set in __init__ and later used by the egg_info property and direct file open calls), which is fragile; refactor to use public APIs: stop accessing dist._path in __init__ and remove/refactor the egg_info property to derive metadata via importlib.metadata APIs, replace direct open("direct_url.json") usages with dist.read_text("direct_url.json") (returns None if absent), and when a filesystem path is required use importlib.metadata.files(dist.metadata["Name"] or dist.metadata.name) combined with importlib.metadata.PackagePath.locate() (or PackagePath.locate on the returned files) to obtain real paths; update any code that assumed self._path (including methods using egg_info and the direct_url.json reads) to work with the read_text/files + PackagePath.locate approach instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@monai/deploy/utils/importutil.py`:
- Around line 33-39: _get_working_set currently rescans distributions on every
call and overwrites earlier matches; change it to cache the mapping and keep the
first-discovered distribution for each normalized name. Implement a module-level
cache (or decorate _get_working_set with functools.lru_cache) and when building
result use result.setdefault(key, _DistributionAdapter(d)) (or check "if key not
in result" before assignment) so earlier distributions are preserved; this will
ensure helpers like is_dist_editable, dist_module_path, and fix_holoscan_import
use the first matching distribution.
---
Nitpick comments:
In `@monai/deploy/utils/importutil.py`:
- Around line 45-48: The code currently relies on the private Distribution._path
(set in __init__ and later used by the egg_info property and direct file open
calls), which is fragile; refactor to use public APIs: stop accessing dist._path
in __init__ and remove/refactor the egg_info property to derive metadata via
importlib.metadata APIs, replace direct open("direct_url.json") usages with
dist.read_text("direct_url.json") (returns None if absent), and when a
filesystem path is required use importlib.metadata.files(dist.metadata["Name"]
or dist.metadata.name) combined with importlib.metadata.PackagePath.locate() (or
PackagePath.locate on the returned files) to obtain real paths; update any code
that assumed self._path (including methods using egg_info and the
direct_url.json reads) to work with the read_text/files + PackagePath.locate
approach instead.
🪄 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: 97052f2c-676b-4a8c-ab00-14d8095b9f5a
📒 Files selected for processing (1)
monai/deploy/utils/importutil.py
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 the current code and only fix it if needed.
Inline comments:
In `@monai/deploy/utils/importutil.py`:
- Around line 66-69: The module_path property currently returns the parent only
if self._path.is_dir(), which is unnecessary and too restrictive; update
module_path in importutil.py (the module_path method) to remove the is_dir()
check so it returns str(self._path.parent) when self._path is set (and still
returns "" when None/falsey), ensuring it handles both file and directory
metadata paths correctly.
🪄 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: 95d07f23-3cb4-41f1-8cc6-9f560ae1ebc0
📒 Files selected for processing (1)
monai/deploy/utils/importutil.py
Signed-off-by: M Q <mingmelvinq@nvidia.com>
There was a problem hiding this comment.
🧹 Nitpick comments (3)
monai/deploy/utils/importutil.py (3)
400-406: Simplify the return logic.The function can be simplified to a single return statement.
♻️ Proposed simplification
def is_module_installed(project_name: str) -> bool: working_set = _get_working_set() dist: Any = working_set.get(_normalize_project_name(project_name)) - if dist: - return True - else: - return False + return dist is not None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monai/deploy/utils/importutil.py` around lines 400 - 406, The is_module_installed function contains redundant conditional logic; simplify it to a single return by returning the boolean result of the lookup directly. Replace the if/else block in is_module_installed (which uses _get_working_set(), dist, and _normalize_project_name(project_name)) with a single return that evaluates whether working_set.get(_normalize_project_name(project_name)) is truthy.
360-371: Consider catchingTypeErrorfor robustness.If
data["dir_info"]exists but is not a dict (e.g.,null), accessing["editable"]will raiseTypeErrorinstead ofKeyError. This is unlikely but could happen with malformed metadata.♻️ Proposed fix
try: if data["dir_info"]["editable"]: return True - except KeyError: + except (KeyError, TypeError): pass🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monai/deploy/utils/importutil.py` around lines 360 - 371, The code that reads direct_url.json may raise TypeError if data["dir_info"] exists but is not a dict; update the editable-check in importutil.py so it handles that case—either validate that data.get("dir_info") is a dict before indexing or expand the exception handler to catch TypeError as well (e.g., change the except KeyError to except (KeyError, TypeError)) when checking data["dir_info"]["editable"] so malformed/null dir_info won't crash.
386-393: SameTypeErrorconsideration applies here.Similar to
is_dist_editable, ifdata["url"]unexpectedly returns a non-string type, thestartswithcall would raiseAttributeError.♻️ Proposed fix
try: file_url = data["url"] if file_url.startswith("file://"): return str(file_url[7:]) - except KeyError: + except (KeyError, TypeError, AttributeError): pass🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monai/deploy/utils/importutil.py` around lines 386 - 393, The direct_url.json parsing assumes data["url"] is a string and calls file_url.startswith(), which can raise AttributeError if the value is non-string; modify the block that reads direct_url.json (the file_url = data["url"] handling in importutil.py) to first verify file_url is a string (e.g., isinstance(file_url, str)) before calling startswith, and only then return the sliced path; if it's not a string, skip/ignore it the same way KeyError is handled so behavior remains 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 `@monai/deploy/utils/importutil.py`:
- Around line 400-406: The is_module_installed function contains redundant
conditional logic; simplify it to a single return by returning the boolean
result of the lookup directly. Replace the if/else block in is_module_installed
(which uses _get_working_set(), dist, and _normalize_project_name(project_name))
with a single return that evaluates whether
working_set.get(_normalize_project_name(project_name)) is truthy.
- Around line 360-371: The code that reads direct_url.json may raise TypeError
if data["dir_info"] exists but is not a dict; update the editable-check in
importutil.py so it handles that case—either validate that data.get("dir_info")
is a dict before indexing or expand the exception handler to catch TypeError as
well (e.g., change the except KeyError to except (KeyError, TypeError)) when
checking data["dir_info"]["editable"] so malformed/null dir_info won't crash.
- Around line 386-393: The direct_url.json parsing assumes data["url"] is a
string and calls file_url.startswith(), which can raise AttributeError if the
value is non-string; modify the block that reads direct_url.json (the file_url =
data["url"] handling in importutil.py) to first verify file_url is a string
(e.g., isinstance(file_url, str)) before calling startswith, and only then
return the sliced path; if it's not a string, skip/ignore it the same way
KeyError is handled so behavior remains unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 65395b41-04de-4f40-9214-fe4d5dc5debe
📒 Files selected for processing (1)
monai/deploy/utils/importutil.py
Signed-off-by: M Q <mingmelvinq@nvidia.com>
|
Summary by CodeRabbit
Bug Fixes
Refactor