Fix converted assets spawning without physics, and install the standalone importers by default - #6935
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.
Merging develop kept this branch's [tool.uv].conflicts block, which "[Docs Overhaul] --extra usage explained, and fixes to commands" (isaac-sim#6979) had deleted. The merge was textually clean but reverted that decision: ``uv sync --extra isaacsim --extra ov`` failed on this branch while it succeeds on develop, and the merged comment above ``all`` claimed no extra was forked while four were. Keep only the two pairs this branch needs. Isaac Sim and the OV backends resolve together again, and the contract test that guarded the empty table now asserts the single remaining fork instead.
The extra appeared in neither the ``-i`` selector table nor the optional extras table, so it was reachable only from the asset-conversion how-to. Add it to both, list it among the selectors ``-i`` does not install, and correct the claim that no extras conflict.
The section led with the legacy installer and offered a wheel install that cannot run: the published isaaclab wheel carries no importers extra, so ``uv pip install 'isaaclab[importers]'`` fails to resolve. Lead with ``uv sync --inexact --extra importers``, matching how the other opt-in extras are documented, and drop the wheel command along with the pre-release rationale that only existed to justify its flags. Attach ``--extra`` to the conversion commands so they work without a separate install step, and name the extras the rerun and viser previews need. State the Isaac Sim incompatibility as the error users actually hit rather than as resolver behaviour.
The scheduled version bump on develop raised the source package versions, but the lockfile still named the previous ones, so ``uv lock --check`` reported the lockfile needed updating.
Keep the constraint that explains the grep and drop the narrative around it.
The extra was declared conflicting with ``isaacsim`` and ``teleop`` on the premise that the standalone importer wheel displaces the Kit extension serving ``isaacsim.asset``. Testing the combination does not support it: uv sync --extra isaacsim --extra importers installs both, rc=0 RECORD overlap between the two distributions 0 of 71 paths pip isaacsim ships no isaacsim/asset tree in site-packages isaacsim.asset resolves to Kit's extension roots with the wheel present URDF conversion in the combined environment rc=0 Isaac Sim keeps serving ``isaacsim.asset`` from its extension roots, so the wheel neither overwrites files nor wins the namespace. Drop the conflicts table and the install-time check that hardcoded the same assumption, which would otherwise keep rejecting a working combination. The generic check reading ``[tool.uv].conflicts`` stays, so a future declaration is honored. Name ``tinyobjloader`` in the extra as well. The converters pull it transitively, and a pre-release only resolves when the specifier naming it is a direct requirement, so inheriting it made the extra uninstallable without ``--prerelease allow`` -- a switch that relaxes the policy for every package in the resolution rather than this one.
The extra installs beside Isaac Sim, so the warnings that it cannot share an environment no longer describe the behavior.
USD accepts a selection naming a variant that does not exist and then composes the prim as if nothing were selected, so a spawn configuration asking for a variant the asset lacks silently produced an asset without what that variant carries -- the same failure the converter now raises on. A variant set the prim does not have stays a warning: one configuration legitimately spawns assets that expose different options.
class_to_dict serializes enum members as their values, and the flat-iterable branch of update_class_from_dict replaces the container wholesale before the scalar enum branch runs, so a field like list[Flavor] came back as raw strings.
The version recorder queried usd-core, which this branch no longer installs, so every capture silently lost its OpenUSD version. Record usd-exchange too and keep usd-core so captures from environments predating the switch stay comparable.
The extras install in their own pip passes, and pip never sees ``[tool.uv].override-dependencies``. The ``importers`` extra carries isaacsim-asset-isolated, which requires ``mujoco==3.8.0`` and ``newton-usd-schemas==0.2.0`` exactly, so running it after the Newton pin left an environment with mujoco below what mujoco-warp requires. Reinstalling the pinned ``newton[sim]`` last restores the stack -- measured in an environment with no overrides in scope, mujoco moves 3.8.0 -> 3.11.0 and mujoco-warp follows. This is not specific to ``importers: any extra whose pip pass drags a Newton dependency backwards is now corrected.
The physics-backend comment in both converter scripts explained PhysX schema composition at a length the one-line assignment did not warrant; state only why the backend has to be named. The smoke harness declared six module-level constants for two entries, a shape no other entry uses. Inline the literals and keep one constant for the newton-shipped MJCF, mirroring _PORTABLE_MJCF in test_mjcf_converter.py. Its required_modules gate went with the guard: newton is a base dependency and cannot be absent. The converter tests had every output directory migrated to tmp_path, which rewrote 28 call sites unrelated to the physics-variant work. Restore the test_dir/output convention the files already used.
Asset conversion without Isaac Sim required opting into an importers extra, so the converters failed out of the box on a default install. The extra existed to keep the standalone wheel away from Isaac Sim, but the two share no files and Kit serves isaacsim.asset from its extension roots when the runtime is present, so there is nothing to keep apart. Make isaacsim-asset-isolated a base dependency and drop the extra along with its --install token, which was never released. tinyobjloader needs no explicit pin: prerelease = "allow" already resolves it to the version the converters require through their own requirement, measured identical at 431 packages.
The Kit-less CI job mounts the checkout read-only, and this branch marks the converter tests kitless, so writing usd_dir beside the test file fails with OSError: [Errno 30] on 28 tests. Record the reason at the marker that causes it. The standalone importers now ship in the base dependencies and contribute an isaacsim.asset namespace portion, so "import isaacsim" succeeds without the runtime. Ask the distribution instead, or the wheel test asserts nothing.
The converter scripts and their tests treated an installed isaacsim-asset-isolated as proof that the standalone importers could be used, and skipped Kit on that basis. That held only while the wheel was opt-in, so its presence implied a kit-less environment. As a base dependency the wheel is present next to the Isaac Sim runtime too, and there it cannot be imported: it contributes isaacsim.asset as a PEP 420 namespace portion, and the runtime ships isaacsim as a regular package, which discards every portion for that name. Conversion then took the kit-less path and died with ModuleNotFoundError: No module named 'isaacsim.asset'. Resolve the isaacsim spec instead, which distinguishes the two without importing anything, so the kit-less path is still preferred wherever it works.
The launch-decision comment restated the docstring at each of the four call sites. Keep the explanation in standalone_importers_available and leave one line where the choice is made.
| RUN uv venv --python /usr/bin/python3.12 --seed --no-managed-python "${VIRTUAL_ENV}" \ | ||
| && chmod +x "${ISAACLAB_PATH}/isaaclab.sh" \ | ||
| && "${ISAACLAB_PATH}/isaaclab.sh" --install newton,rl[all],ov[all],visualizer[all] \ | ||
| && "${ISAACLAB_PATH}/isaaclab.sh" --install newton,rl[all],ov[all],visualizer[all],importers \ |
There was a problem hiding this comment.
I don't see importers anywhere in the PR for isaaclab.sh -i, is this correct?
hujc7
changed the title
4 participants
1. Summary
usd-exchangeis the sole OpenUSD provider on all platforms;usd-coreis gone. Both vendor a completepxrinto the same directory, so co-installing silently replaces one with the other and uninstalling either deletes files the survivor needs. No resolver reports it."Physics"variant. Previously Add physics variant selection to asset converters #6881, folded here.Based on #6866 by @kellyguo11.
2. What changed
usd-exchangeon all platforms. Newton selects[sim], not[sim,importers], whose extra pullsusd-coreback in; the mesh-processing packages it carried are declared directly.isaacsim-asset-isolatedmoved to[project.dependencies]. Noimportersextra, no--install importerstoken — neither was ever released."Physics"variant on the generated USD, selected by the newAssetConverterBaseCfg.physics_variant(defaultphysics). The asset hash is recorded only after conversion succeeds, so a run that raised is not cached.select_usd_variantsraises when a variant set exists but does not offer the requested variant — USD accepts such a selection and composes the prim as if nothing were selected. An absent variant set still warns.kitlessmarker.3. Design notes
3.1 Installation is not availability
The wheel contributes
isaacsim.assetas a PEP 420 namespace portion. The Isaac Sim runtime shipsisaacsimas a regular package, and a regular package discards every namespace portion for that name — so an installed wheel is unimportable whenever the runtime is onsys.path.While the importers were opt-in, "installed" implied "kit-less environment", so
metadata.distribution(...)was a sound proxy for "usable". As a base dependency it no longer is.standalone_importers_available()resolves the spec instead, which answers availability without importingisaacsim— keeping it usable before a launch decision, where importing a simulation module is not allowed.The kit-less path stays preferred wherever it works; Kit is launched only when the runtime owns the namespace.
3.2 No
tinyobjloaderpintinyobjloaderpublishes no usable stable release (0.1, then2.0.0rc*). With the repo's existingprerelease = "allow", the converters' own requirement already resolves it to2.0.0rc13; an explicit pin changed nothing (measured: identical lock, 431 packages).[tool.uv]is untouched by this PR.4. Testing
4.1 Documented install variants
Each ran the steps from
docs/source/setup/installation/index.rstverbatim in a fresh Python 3.12 environment, then probed howisaacsimresolves, whether the importers import, the USD provider, and — the step install-CI never takes — whether a conversion actually produces a USD.isaacsimuv run --extra ovphysx./isaaclab.sh -i 'ov[ovrtx]'./isaaclab.sh -i isaacsimuv pip install "isaacsim[all,extscache]"+./isaaclab.sh -iuv init+uv add <isaaclab wheel>_isaac_simThe provider invariant holds in both full Isaac Sim environments — the ones most likely to drag
usd-coreback in.4.2 Other
install_cinon-GPU suite (fresh env per test)5. 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.warp-lang[benchmark]and[examples]still declareusd-coreon x86_64/Windows. Neither extra is selected here, so the collision is not reachable through this repo../isaaclab.sh -iwithout an active 3.12 environment fails withModuleNotFoundError: No module named 'tomllib'on hosts whose system Python is older than 3.11. Pre-existing, not touched here.