Fix MDL module dependency retrieval

[ooctipus]

Summary

• Collect relative MDL module imports when mirroring remote assets locally.
• Covers ordinary import and [export] using ... import ... forms, including current-package .:: imports and parent-package ..:: imports.
• Leaves global/built-in MDL modules to the renderer module path while preserving existing MDL texture dependency handling.

Validation

• python3 -m py_compile source/isaaclab/isaaclab/utils/assets.py source/isaaclab/test/utils/test_assets.py
• direct importlib check for _find_mdl_dependencies with Hospital-style relative imports, using-imports, and parent-relative imports
• python3 tools/changelog/cli.py check develop
• pre-commit run --files source/isaaclab/isaaclab/utils/assets.py source/isaaclab/test/utils/test_assets.py source/isaaclab/changelog.d/fix-mdl-module-dependencies.rst

Focused pytest was not runnable in this shell because the local Python environment is missing lazy_loader. Full Kit/Hospital viewport repro was not run locally.

[isaaclab-review-bot]

🤖 Isaac Lab Review Bot

PR #6034 — Fix MDL module dependency retrieval

Summary

This PR correctly extends the MDL dependency scanner to handle relative module imports (.::Module) in addition to the existing texture resource handling. The Hospital environment materials reference sibling MDL modules via import .::OmniUe4Function etc., which were previously not collected during local asset mirroring.

✅ Findings

Correctness: Good

• The approach of stripping comments/strings first (via _MDL_RESOURCE_RE.sub) before scanning for import statements correctly prevents false positives from commented-out or string-embedded imports.
• The \b word boundary in _MDL_IMPORT_RE correctly prevents matching reimport or similar tokens.
• Multi-import statements (comma-separated) are handled.
• export import .::Module; (valid MDL) is correctly captured.
• Global imports (::nvidia::...) are properly skipped since they only match .:: prefixes.

Design: Clean

• Extracting _find_mdl_dependencies() as a separate function improves testability and readability.
• The logic for mapping :: path separators to filesystem / via posixpath.join is correct for this use case.

Tests: Comprehensive

• Tests cover: simple relative imports, nested modules (Shared::OmniUe4Base), wildcard imports, global imports (correctly excluded), line comments, block comments, and quoted strings containing import-like text.

💭 Minor Observations (non-blocking)

1. Wildcard-only import edge case: import .::*; would result in module_name = "" after .removesuffix("::*"), and components would be empty, so nothing is added. This is correct behavior (package-level wildcard has no single file to fetch).

2. Multi-level relative imports: MDL supports ..:: (parent-level) imports. The current code only handles .:: (sibling). This is fine for the Hospital use case but could be extended if other environments use parent-relative imports.

Verdict

LGTM — Clean, focused fix with good test coverage. CI passing (pre-commit ✓, build ✓). No correctness issues found.

---

0 blocking findings | Reviewed at 465ee10

---

🔄 Update — reviewed be50acf

New commits refactor the import parsing logic:

• _MDL_IMPORT_RE and _MDL_RELATIVE_IMPORT_RE moved to module-level compiled constants (good for performance).
• Import resolution extracted into _find_mdl_import_dependencies() with improved ambiguity handling: import .::A::B; now correctly emits candidates for both interpretations (module A/B.mdl and symbol B from A.mdl), while wildcard imports (::*) unambiguously resolve to a single module path.
• Tests updated to reflect the new candidate-based output.

This is cleaner and more correct than the previous approach. No new issues found. Still LGTM.

Reviewed at be50acf

[greptile-apps]

Greptile Summary

This PR fixes MDL module dependency resolution by extending the existing MDL scanner to also collect relative module imports (e.g., import .::OmniUe4Function;) in addition to texture references, ensuring sibling MDL files are downloaded when mirroring remote assets locally.

• Extracts the MDL dependency logic into a standalone _find_mdl_dependencies function and adds a new _MDL_IMPORT_RE regex to match import statements, filtering to only relative (.::) references.
• Converts each relative module path to a sibling .mdl file path using :: as a directory separator, stripping wildcard suffixes (::*) before resolution.
• Adds a focused unit test covering the Hospital-style import patterns, confirming that absolute imports, commented-out imports, and string-embedded imports are all correctly excluded.

Confidence Score: 4/5

Safe to merge; the change is additive, has no impact on existing texture-dependency or USD-layer paths, and the new import-scanning logic is well-guarded against comments and string literals.

The implementation correctly strips comments and quoted strings before running the import regex, handles wildcard suffixes, and resolves nested module paths to sibling file paths that integrate cleanly with the existing URL-resolution logic. The two minor gaps — MDL using statements not being scanned, and comma-batched imports lacking a test — don't affect the stated Hospital-environment use case.

source/isaaclab/isaaclab/utils/assets.py — worth a second look for coverage of the MDL using keyword if other environments beyond Hospital are expected to work.

Important Files Changed

Filename	Overview
source/isaaclab/isaaclab/utils/assets.py	Adds _find_mdl_dependencies helper and _MDL_IMPORT_RE regex; correctly strips comments/strings before import scanning and resolves relative module paths to sibling .mdl files. MDL using statements are not captured (by design for this PR's scope).
source/isaaclab/test/utils/test_assets.py	Adds a well-structured test covering relative imports, wildcards, nested module paths, absolute imports, and comment/string exclusion edge cases.
source/isaaclab/changelog.d/fix-mdl-module-dependencies.rst	Changelog entry clearly describes the fix for sibling MDL module retrieval.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[_find_asset_dependencies\nlocal_asset_path] --> B{suffix == .mdl?}
    B -- Yes --> C[Open & read MDL source]
    C --> D[_find_mdl_dependencies\nsource]
    D --> E[Scan _MDL_RESOURCE_RE\nfor quoted strings]
    E --> F{Matches\n_MDL_TEXTURE_RE?}
    F -- Yes --> G[Add texture path to refs]
    F -- No --> H[Continue]
    D --> I[Strip strings + comments\n_MDL_RESOURCE_RE.sub]
    I --> J[Scan _MDL_IMPORT_RE\non cleaned source]
    J --> K{Starts with '.::' ?}
    K -- No --> L[Skip - absolute or non-relative import]
    K -- Yes --> M[Strip leading '.::'\nremove trailing '::*']
    M --> N[Split '::' into path components\nfilter empty and '*']
    N --> O[Add component/path.mdl to refs]
    D --> P[Return refs set]
    B -- No --> Q{suffix in _USD_EXTENSIONS?}
    Q -- No --> R[Return empty set]
    Q -- Yes --> S[Parse USD layer\nUsdUtils.ModifyAssetPaths]
    S --> T[Return USD asset refs]

Loading

_{Reviews (1): Last reviewed commit: "Fix MDL module dependency retrieval" | Re-trigger Greptile}

[greptile-apps]

MDL using statement not scanned for relative imports

MDL supports two interchangeable forms for module imports: import .::Foo; and using .::Foo::*;. The new scanner only matches the import keyword via _MDL_IMPORT_RE. Any MDL material that uses using .::Foo::*; for relative sibling imports would silently drop those dependencies, leaving the mirrored asset tree incomplete. If other environments (beyond Hospital) rely on using for relative imports, those files won't be downloaded.

[greptile-apps]

Multi-import single-line form not covered by tests

The splitting logic in _find_mdl_dependencies correctly handles comma-separated imports on one line (e.g., import .::Foo, .::Bar;), but this form has no test case. Adding a case like import .::A, .::B; would verify the comma-split path and guard against future regressions on that branch.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

[isaaclab-review-bot]

🔄 Incremental Review Update (be50acf)

Thanks for the update! The commit was amended with improvements:

✅ Refactored parsing logic - The import parsing was extracted to a dedicated _find_mdl_import_dependencies() function with a proper regex (_MDL_RELATIVE_IMPORT_RE) instead of string manipulation.

✅ Improved symbol/module ambiguity handling - The new code correctly handles cases like import .::Helpers::make_color; which could mean either module Helpers/make_color.mdl OR symbol make_color from Helpers.mdl. It now generates both candidates (Helpers.mdl and Helpers/make_color.mdl), ensuring dependencies aren't missed.

✅ Test expanded - Added test case for the ambiguous symbol case with import .::Helpers::make_color;.

---

Outstanding items from initial review:

• ⚠️ using statement support (inline comment) - Still not addressed. MDL's using .::Foo::*; syntax is an alternative to import but isn't scanned.
• ⚠️ Multi-import comma syntax test (inline comment) - Not addressed, though the regex-based approach should handle this naturally.

[isaaclab-review-bot]

🔄 Re-review after new commits (781b06b ← 465ee10)

The new changes address both previous review comments:

1. ✅ using statement scanning — Now handled via _MDL_USING_IMPORT_RE with dedicated parsing in _find_mdl_dependencies. The test includes both export using .::Local::Palette import * and using ..::Shared::Functions import make_color, make_normal forms.

2. ✅ Multi-import form coverage — The _MDL_RELATIVE_IMPORT_RE.finditer() approach naturally handles comma-separated module lists within a single import statement by finding all relative-path matches in the captured clause. Test coverage looks solid.

New changes summary:

• Added _MDL_IMPORT_RE, _MDL_USING_IMPORT_RE, and _MDL_RELATIVE_IMPORT_RE regex patterns
• Extracted MDL dependency logic into _find_mdl_dependencies() and _find_mdl_import_dependencies() helpers
• Handles relative imports with ..:: (parent) and .:: (sibling) prefixes
• Correctly strips comments and quoted strings before scanning imports
• Added comprehensive test covering relative imports, wildcard imports, parent-directory imports, commented/quoted imports (properly ignored)

Implementation looks clean and well-structured. No new concerns. 👍

[isaaclab-review-bot]

Incremental Review Update (781b06b)

✅ Previous concern addressed: The using statement scanning issue from my earlier review has been fully resolved. The new commit adds:

• _MDL_USING_IMPORT_RE regex to capture using ... import statements
• Processing of both using and import forms in _find_mdl_dependencies()
• Comprehensive test coverage including export using .::Local::Palette import * and using ..::Shared::Functions import ... patterns

📝 The multi-import test case suggestion (P2) remains as an optional enhancement, but the implementation is correct since finditer() naturally handles multiple relative imports in a single clause.

No new issues introduced in the incremental changes. LGTM 👍