Adds support for URDF/MJCF importers from pip wheel#5660
Conversation
github-actions
Bot
added
documentation
Greptile SummaryThis PR adds support for loading URDF and MJCF importers from the standalone
Confidence Score: 3/5The standalone-vs-extension dispatch works for single conversions, but batch conversion and the test suite both have current defects in the changed code. The new
Important Files Changed
Reviews (1): Last reviewed commit: "clean up" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| def _import_standalone_importer_module(module_name: str, distribution_path: str): | ||
| """Import an importer API module from the standalone distribution. | ||
|
|
||
| Launching Kit can prepend Isaac Sim extension paths that expose the same | ||
| ``isaacsim.asset.importer.*`` modules. When the standalone wheel is | ||
| installed, keep its site-packages root at the front of ``sys.path`` so the | ||
| wheel's API and converter runtime dependencies win over extension copies. | ||
| """ | ||
| distribution_root = Path(distribution_path).resolve() | ||
| sys.path[:] = [path for path in sys.path if Path(path or ".").resolve() != distribution_root] | ||
| sys.path.insert(0, str(distribution_root)) | ||
| importlib.invalidate_caches() | ||
|
|
||
| for loaded_module_name in list(sys.modules): | ||
| if loaded_module_name == module_name or loaded_module_name.startswith(f"{module_name}."): | ||
| del sys.modules[loaded_module_name] | ||
|
|
||
| module = importlib.import_module(module_name) | ||
| module_file = getattr(module, "__file__", None) | ||
| if module_file is not None and not Path(module_file).resolve().is_relative_to(distribution_root): | ||
| raise ImportError( | ||
| f"Expected {module_name} from standalone {_STANDALONE_IMPORTER_DISTRIBUTION!r} at " | ||
| f"{distribution_root}, but resolved {module_file}." | ||
| ) | ||
| return module |
There was a problem hiding this comment.
_import_standalone_importer_module evicts the entire isaacsim.asset.importer.* subtree from sys.modules and reimports it on every call to load_importer_api. Since load_importer_api is invoked inside _convert_asset (which is called for each new conversion), batch-converting a set of assets causes repeated module teardown and reimport. More critically, any code that holds a reference to URDFImporter or MJCFImporter obtained before a conversion will end up with a stale class object after the next conversion, silently breaking isinstance checks against those classes.
The sys.path manipulation is also permanent and global — it alters the path for the lifetime of the process. Caching the result of a successful module load (or doing this setup once at package init time) would avoid the repeated eviction-and-reimport cycle.
| def test_load_importer_api_prefers_standalone_package(monkeypatch): | ||
| """The standalone package path should not touch the Isaac Sim extension manager.""" | ||
| calls = [] | ||
|
|
||
| monkeypatch.setattr(importer_api, "_get_standalone_importer_distribution_path", lambda: "/standalone/site-packages") | ||
| monkeypatch.setattr( | ||
| importer_api.importlib, | ||
| "import_module", | ||
| lambda module_name: calls.append(("import", module_name)) or _make_importer_module("urdf"), | ||
| ) | ||
| monkeypatch.setattr( | ||
| importer_api, | ||
| "_enable_isaacsim_extension", | ||
| lambda extension_name: pytest.fail(f"Unexpected extension enable: {extension_name}"), | ||
| ) | ||
|
|
||
| importer_cls, config_cls = importer_api.load_importer_api("urdf") | ||
|
|
||
| assert importer_cls is _Importer | ||
| assert config_cls is _ImporterConfig | ||
| assert calls == [("import", "isaacsim.asset.importer.urdf")] |
There was a problem hiding this comment.
sys.path side-effect leaks between tests
monkeypatch restores _get_standalone_importer_distribution_path and importlib.import_module, but _import_standalone_importer_module itself is not mocked. As a result, load_importer_api("urdf") still executes the real function, which permanently prepends /standalone/site-packages to sys.path and calls importlib.invalidate_caches(). These mutations outlast the test. The same issue affects test_load_importer_api_falls_back_when_standalone_import_fails. Either patch _import_standalone_importer_module directly with a thin stub, or use monkeypatch.syspath_prepend so pytest cleans it up automatically.
| details = (result.stderr or result.stdout).strip() | ||
| raise RuntimeError( | ||
| f"The standalone {importer_kind.upper()} importer runtime is installed but cannot load in this environment. " | ||
| "This usually means the OpenUSD Python bindings from 'usd-core' and 'usd-exchange' are mismatched. " | ||
| "Reinstall 'usd-exchange' after installing IsaacLab and 'isaacsim-asset-isolated', or use a kitless " | ||
| "environment without 'usd-core'." | ||
| f"\nRuntime check exited with code {result.returncode}:\n{details}" |
There was a problem hiding this comment.
usd-core reference in error message
The error message advises "use a kitless environment without usd-core", but setup.py in this PR removes usd-core as a dependency entirely and replaces it with usd-exchange alone. Users on the new dependency model are unlikely to have usd-core installed, making the diagnostic hint misleading.
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review: PR #5660 — URDF/MJCF Importers from pip wheel
This PR adds support for standalone URDF/MJCF importers from isaacsim-asset-isolated, enabling kitless asset conversion. Overall, this is a well-structured refactor with good separation of concerns. Below are findings organized by priority.
🔬 Architecture & Design
Design Assessment: The approach is sound — abstracting importer loading into _importer_api.py allows flexible fallback between standalone packages and Isaac Sim extensions. The CLI refactoring into converter_cli_utils.py cleanly separates argument parsing logic from conversion logic.
Cross-Module Impact: The changes affect:
UrdfConverterandMjcfConverter— now useload_importer_api()instead of direct imports- CLI scripts — restructured to support kitless execution
setup.py— dependency changes affect all users
Implementation Findings
🟡 Warning: _importer_api.py:88-91 — sys.path manipulation has side effects
sys.path[:] = [path for path in sys.path if Path(path or ".").resolve() != distribution_root]
sys.path.insert(0, str(distribution_root))This globally modifies sys.path and purges cached modules. While necessary for the use case, it could affect other imports in the same process. Consider documenting this side effect in the function docstring more explicitly, noting that this function should only be called during initial importer loading.
🟡 Warning: _importer_api.py:100-105 — Module cache invalidation scope
The _clear_importer_provider_modules() function clears modules matching isaacsim.asset.importer and isaacsim.asset.transformer prefixes. If parent namespace packages (isaacsim.asset or isaacsim) are already loaded from a different location, they won't be cleared, potentially causing import resolution issues. Consider also clearing parent namespace packages if they exist.
🟡 Warning: converter_cli_utils.py:84-92 — Subprocess timeout handling
The 30-second timeout for runtime validation is hardcoded. On slower systems or when packages need to be compiled/cached on first import, this could fail spuriously. Consider making this configurable or increasing the default.
🔵 Improvement: urdf_converter.py — Logging infrastructure change
The switch from carb.log_warn to Python's logging module is good for kitless support. However, the module-level logger doesn't have a handler configured by default. Users running kitless may not see warnings unless they configure logging. Consider adding a brief note in the docstring about this.
🧪 Test Coverage Analysis
Coverage Summary: The new test_importer_api.py provides good unit tests for the core load_importer_api() function with four test cases covering:
- Isaac Sim extension preference when both providers available
- Extension-only path when standalone unavailable
- Standalone fallback when Isaac Sim unavailable
- No fallback to standalone when Isaac Sim extension fails (correct behavior)
Gaps:
🟡 Missing: converter_cli_utils.py tests — Criticality: 6/10
- The
parse_converter_cli_args()andensure_standalone_importer_runtime()functions have no direct unit tests - These functions contain significant logic (subprocess spawning, conditional Kit launching)
- Recommendation: Add tests for
parse_visualizer_csv()edge cases and_should_launch_kit()logic
🔍 Error Handling Review
Good patterns observed:
_importer_api.pyproperly chains exceptions with context (raise ImportError(...) from exc)ensure_standalone_importer_runtime()provides clear error messages with subprocess output
🟢 GOOD: The error messages in load_importer_api() tell users exactly what to do:
"Install the standalone 'isaacsim-asset-isolated' package or run with the Isaac Sim extension available."
Minor Observations
-
Documentation: The
--viz kitexamples in docs are helpful. The deprecation note for--headlessis clear. -
Changelog: Entry is clear and covers both the addition and the dependency change.
-
Dependency change: Removing
usd-core==25.11.0and relying solely onusd-exchange>=2.2.2plus the converter runtimes simplifies the dependency tree.
Verdict
Ship it with minor fixes. The architecture is sound, error handling is good, and the feature enables an important use case. The warnings above are improvements rather than blockers.
Recommended before merge:
- Add a docstring note about
sys.pathside effects in_import_standalone_importer_module() - Consider adding basic unit tests for
converter_cli_utils.pyhelper functions
Update (46543c2): Reviewed after merge from develop branch.
✅ Merge commit only — This push incorporates changes from the develop branch (35+ commits including camera.py migration, SceneDataProvider, RigidObjectCollection, Newton deformable support, etc.).
No changes to the core importer feature files (_importer_api.py, converter_cli_utils.py, urdf_converter.py, mjcf_converter.py, convert_urdf.py, convert_mjcf.py, test_importer_api.py) in this merge.
The only importer-related change is an unrelated addition of pytetwild==0.2.3 to setup.py for deformable body support (from the Newton deformable PR).
Previous observations remain applicable — the module cache eviction scope and sys.path manipulation documentation suggestions from earlier reviews are still open for consideration.
… into pip-importers
Signed-off-by: Kelly Guo <kellyg@nvidia.com>
1 participant
Description
Adds support for importing URDF and MJCF from kitless standalone path using the standalone wheel installable through pip.
Type of change
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there