Fix converted assets spawning without physics, and add an importers extra - #6935
Fix converted assets spawning without physics, and add an importers extra#6935hujc7 wants to merge 11 commits into
Conversation
github-actions
Bot
added
documentation
hujc7
changed the title
hujc7
changed the title
usd-core and usd-exchange each install a complete pxr into the same directory, so co-installing them silently replaces one OpenUSD with the other and uninstalling either deletes files the survivor still needs. No resolver reports it: they are separate distributions and the collision only happens on disk. Isaac Lab required usd-core on x86_64 while the standalone URDF and MJCF importers require usd-exchange, so the two could not be combined. Use usd-exchange everywhere, which also aligns x86_64 with aarch64. Newton selects [sim] rather than [sim,importers], whose extra pulls usd-core back in; the mesh-processing packages that extra carried are declared directly. Based on isaac-sim#6866 by @kellyguo11.
The importers let conversion run without Isaac Sim, but the wheel contributes to the isaacsim namespace: beside a full Isaac Sim it displaces the Kit extension serving the same import and conversion fails with "No module named 'isaacsim.asset'". Packaging cannot express "install unless Isaac Sim is present", so ship them opt-in. The pair is declared in tool.uv.conflicts, which binds uv sync. The install CLI installs each feature in its own pip pass, so no resolver ever sees a combination there; it now reads the same table and rejects any declared conflict, covering the four pairs that were previously declared and unenforced as well. Also put MuJoCo's USD schemas on OpenUSD's plugin search path. They are a codeless plugin registered when mujoco-usd-converter is imported, but OpenUSD builds its schema registry once and ignores plugins registered afterwards, so MJCF conversion failed whenever anything queried a schema first.
The URDF and MJCF importers put the physics description behind a "Physics" variant set, and the Isaac Sim importer extensions leave it unselected. An unselected variant set contributes nothing, so those assets composed with no joints, articulation roots, or mass properties. Select a variant during conversion, configurable through AssetConverterBaseCfg.physics_variant and defaulting to the backend-portable one. Conversion raises when the asset does not offer the requested variant rather than substituting another backend's. At spawn time, select_usd_variants now raises for a "Physics" set that is absent or lacks the requested variant. USD accepts a selection naming a variant that does not exist and composes the prim as if nothing were selected, which loses the physics with no diagnostic. Other variant sets keep warning and continuing, which is what they were introduced for. class_to_dict serialized enum members by expanding their internals, so the new enum field wrote unusable entries into config.yaml; update_class_from_dict now rebuilds the member from its value.
The kit-less container is where the standalone importers actually serve conversion, so the URDF and MJCF converter tests belong in its validation. Mark them, and the suites already running there, with a kitless marker so the workflow selects by marker rather than by a list it has to be edited to extend. The image has no Isaac Sim, so pytest cannot be pointed at a directory: collecting one imports every module in it and most of the suite builds an AppLauncher at import time. Files are discovered by grepping for the marker, which needs no import.
c16c09c to
ed60409
Compare
…hange-provider # Conflicts: # uv.lock
One fragment per touched package: fold the usd-exchange entry into the single isaaclab fragment, and skip isaaclab_newton, where the change is pytest markers only.
Greptile SummaryThe PR standardizes kit-less OpenUSD packaging on
Confidence Score: 4/5The PR appears safe to merge, with non-blocking fixes recommended for the mismatched pytest marker registration and contradictory co-installation guidance. The implementation changes do not leave an established blocking failure, but test-marker validation and importer installation documentation are inconsistent with the newly introduced contracts. Files Needing Attention: pyproject.toml; docs/source/how-to/import_new_asset.rst Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart LR
Input[URDF or MJCF] --> Importer{Runtime}
Importer -->|Isaac Sim| Kit[Kit importer extension]
Importer -->|Kit-less| Isolated[isaacsim-asset-isolated]
Kit --> USD[Generated USD]
Isolated --> USD
USD --> Variant[Select Physics variant]
Variant --> Asset[Spawnable physics asset]
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| "benchmark: test covers the Isaac Lab benchmark framework and infrastructure", | ||
| "rendering: test exercises the rendering / camera / visualizer pipeline", | ||
| "smoke: tests for core installation, task, and RL functionality", | ||
| "importers: test must pass inside the Kit-less container, which has no Isaac Sim runtime", |
There was a problem hiding this comment.
The new converter tests and container selection use pytest.mark.kitless, but this configuration registers importers instead. This produces unknown-marker warnings and makes collection fail under --strict-markers.
| "importers: test must pass inside the Kit-less container, which has no Isaac Sim runtime", | |
| "kitless: test must pass inside the Kit-less container, which has no Isaac Sim runtime", |
There was a problem hiding this comment.
Fixed in 2f6bf12. The marker is now registered as kitless, matching the six test files and the -m kitless selection in validate-kitless-image. It had been renamed along with the extra by a bulk rename that should only have touched the extra name.
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
Isaac Lab Review Bot
The OpenUSD provider consolidation and converter variant-selection changes are coherent, but four integration gaps need correction: the kit-less marker is registered under the wrong name, kit-less URDF tests write into a read-only mount, the importers conflict omits the Isaac-Sim-bearing teleop extra, and installer conflict detection does not expand optional submodule tokens to their actual root extras.
- Design and architecture: The opt-in
importersextra and installer-side enforcement appropriately model its incompatibility with Isaac Sim. However,teleopdirectly installs Isaac Sim and therefore also needs an explicit conflict withimporters. The installer must additionally expand optional submodules throughOPTIONAL_SUBMODULE_ROOT_EXTRAS; otherwisemimiccan install theteleoproot extra without triggering its declared OV conflicts. - API: The nested
AssetConverterBaseCfg.PhysicsVariantand exportedREQUIRED_VARIANT_SETSare documented consistently, and the stricterselect_usd_variantsbehavior is recorded in the changelog. No separate public API defect was identified, but packaging conflict behavior exposed through the install CLI remains incomplete forteleopandmimicrequests. - Implementation: The conversion hash is correctly written only after successful variant selection, and enum serialization is symmetric. The CI integration is currently inconsistent: tests and the action use
kitless, while pytest registersimporters. Moreover, selecting the full URDF converter module for kit-less CI runs tests that create output beneathsource/, which the action mounts read-only; those outputs need a writable temporary directory.
Minor fixes needed. Posted 4 actionable findings inline.
The full PR diff was reviewed; some supplemental surrounding file context was omitted.
Automated review; human maintainers own approval decisions.
| "benchmark: test covers the Isaac Lab benchmark framework and infrastructure", | ||
| "rendering: test exercises the rendering / camera / visualizer pipeline", | ||
| "smoke: tests for core installation, task, and RL functionality", | ||
| "importers: test must pass inside the Kit-less container, which has no Isaac Sim runtime", |
There was a problem hiding this comment.
🟡 Warning · Implementation — Registered marker name does not match tests
The tests and .github/actions/validate-kitless-image/action.yml use pytest.mark.kitless (grep for pytest\.mark\.kitless, then pytest -m kitless), but the marker registered here is named importers. So kitless stays unregistered and raises PytestUnknownMarkWarning on every marked module (failing under --strict-markers), while the registered importers marker selects nothing. Rename the registration to kitless.
There was a problem hiding this comment.
Fixed in 2f6bf12 — registered as kitless. Caused by a bulk rename of the extra (kitless → importers) that also caught the marker string, leaving all six marked modules with an unregistered marker.
| [{ extra = "isaacsim" }, { extra = "ov" }], | ||
| [{ extra = "isaacsim" }, { extra = "ovphysx" }], | ||
| # both supply the ``isaacsim.asset`` importers; together the wheel shadows the Kit extension | ||
| [{ extra = "isaacsim" }, { extra = "importers" }], |
There was a problem hiding this comment.
🟡 Warning · Design Architecture — teleop does not inherit the importers conflict
The teleop extra installs isaacsim[all,extscache] directly, which is why the table already mirrors (isaacsim, ov) with (teleop, ov). Only (isaacsim, importers) was added, so uv sync --extra teleop --extra importers resolves and ./isaaclab.sh -i teleop,importers passes the new check, co-installing isaacsim-asset-isolated beside a real Isaac Sim — exactly the shadowing this extra exists to prevent. Add (teleop, importers).
There was a problem hiding this comment.
Fixed in 2f6bf12: added [{ extra = "teleop" }, { extra = "importers" }]. Correct — teleop lists isaacsim[all,extscache]==6.0.1.0 directly, so it inherits the incompatibility, and the table already mirrored every isaacsim pair with a teleop one.
Note that after merging develop the ovphysx pairs were removed upstream, so the table is now teleop×ov, isaacsim×ov, isaacsim×importers, teleop×importers.
| pytestmark = pytest.mark.integration | ||
| # conversion is served by the standalone importers when Isaac Sim is absent, so these run | ||
| # unchanged in the Kit-less container | ||
| pytestmark = [pytest.mark.integration, pytest.mark.kitless] |
There was a problem hiding this comment.
🟡 Warning · Implementation — Kit-less run writes into read-only source mount
Adding kitless makes the grep-based runner in validate-kitless-image execute this module, but that job mounts source/ as :ro. Several tests here, including the new test_physics_variant_*, call os.makedirs(<test_dir>/output/...) and write the converted USD there, which fails with Read-only file system. Direct conversion output for kit-less-selected tests to tmp_path instead.
There was a problem hiding this comment.
Fixed in 2f6bf12. All 29 write sites in the two converter test modules now go through a helper that falls back to a temporary directory when the directory beside the tests is not writable, so the read-only source/ mount is handled. Verified by simulating the read-only mount: output resolves to /tmp/isaaclab_converter_tests.
|
|
||
| Tokens are not extras: ``ov[ovrtx]`` installs ``ovrtx``, not ``ov``. | ||
| """ | ||
| extras = set(optional_submodules) |
There was a problem hiding this comment.
🔵 Suggestion · Implementation — Submodule tokens are not mapped to root extras
set(optional_submodules) stores the CLI token, but OPTIONAL_SUBMODULE_ROOT_EXTRAS maps mimic to the teleop and mimic extras. So -i mimic,ov[ovphysx] installs the Isaac-Sim-bearing teleop extra while the declared {teleop, ovphysx} conflict never matches, leaving one of the pairs this function claims to enforce unenforced. Expand submodule tokens through OPTIONAL_SUBMODULE_ROOT_EXTRAS first.
There was a problem hiding this comment.
Fixed in 2f6bf12: optional submodule tokens are expanded through OPTIONAL_SUBMODULE_ROOT_EXTRAS before the conflict check, so -i mimic contributes both mimic and teleop.
Verified at the time: mimic + ov[ovphysx] was rejected. After merging develop the ovphysx conflict pairs no longer exist upstream, so that specific pair is legal again — the check reads [tool.uv].conflicts rather than hardcoding, so it tracked the change.
- Register the pytest marker as `kitless`; a rename had left it as `importers` while every test uses `kitless`, so the marker was unregistered and the registered one selected nothing. - Declare `teleop` conflicting with `importers`. teleop installs isaacsim itself, so it inherits the same incompatibility. - Expand optional submodule tokens through OPTIONAL_SUBMODULE_ROOT_EXTRAS before checking conflicts, so `-i mimic,ov[ovphysx]` is rejected -- mimic pulls the teleop extra, which carries Isaac Sim. - Write converted USD to a temporary directory when the tests cannot write beside themselves. The kit-less image mounts source/ read-only, and the kitless marker now selects these tests there. - Drop the documentation paragraph describing a fallback order for a shared environment; the two cannot share one.
The conflict check sees one invocation, so installing the two sides in separate passes -- or into a checkout carrying a local _isaac_sim -- still reached the state the extra exists to prevent. Check the target environment for the other side before installing either. Also carry newton[importers]'s trimesh>=4.6.8 lower bound into the base dependency, which the hand-expanded replacement had dropped, and give both Changed entries the migration guidance AGENTS.md requires.
…hange-provider # Conflicts: # pyproject.toml # uv.lock
Develop isaac-sim#6938 re-imposed the platform split by overriding usd-exchange to aarch64, so x86_64 would keep usd-core. This branch removes usd-core entirely, and the merge kept that override -- so the kit-less image installed no pxr at all and every converter test failed with "No module named 'pxr'". The install CLI applies these overrides too, not just uv sync, so the effect reached both paths.
The CLI constant tests pin the exact feature sets, so registering `importers` has to be reflected there. Also drop the aarch64-only usd-exchange override from the two published override files, which must match the root pyproject and still carried the entry removed from it.
1 participant
Summary
Kit-less conversion could not work. The standalone URDF/MJCF importers require
usd-exchange, Isaac Lab requiredusd-coreon x86_64, and both install a completepxrinto the same directory — co-installing silently replaces one OpenUSD with the other, and uninstalling either deletes files the survivor needs. No resolver reports it; the collision only happens on disk.Unify on
usd-exchange, and ship the importers as an opt-inimportersextra. They cannot be a base dependency: the wheel contributes to theisaacsimnamespace, so beside a full Isaac Sim it displaces the Kit extension serving the same import (No module named 'isaacsim.asset'). Packaging cannot express "install unless Isaac Sim is present".Also fixes converted assets spawning with no physics — previously #6881, folded here.
Based on #6866 by @kellyguo11.
What changed
usd-exchangeon all platforms;usd-coreremoved. Newton selects[sim], not[sim,importers], whose extra pullsusd-coreback in; the mesh-processing packages it carried are declared directly.importersextra, declared conflicting withisaacsimin[tool.uv].conflicts../isaaclab.sh --installnow rejects any conflicting pair frompyproject.toml, covering four pairs that were declared but unenforced."Physics"variant on the generated USD;select_usd_variantsraises for a required set that is absent or lacks the requested variant instead of composing nothing.kitlessmarker rather than a list in the workflow.Testing
./isaaclab.sh --install 'newton,rl[rsl-rl],importers'then convert a URDFuv sync --extra importers/--extra isaacsim-m kitless)Follow-up
usd-exchange2.3.0 vendors OpenUSD 25.05, which predates theUsdPhysicsParsingUtilityfix (upstream issue 4002, public 26.05). Measured on the shape upstream added as its regression test: 15/15 crashed, 0/15 withPXR_WORK_THREAD_LIMIT=1. usd-exchange MR !257 moves the default flavor to 26.08 and resolves it.A standalone
pip install isaaclab[importers]from outside a checkout needs--index-strategy unsafe-best-match --prerelease allow; inside a checkout uv reads both from[tool.uv].