[App] Bootstrap URDF/MJCF converters via launch_simulation - #6824
[App] Bootstrap URDF/MJCF converters via launch_simulation#6824ooctipus wants to merge 5 commits into
Conversation
github-actions
Bot
added
documentation
Greptile SummaryThe PR consolidates URDF and MJCF converter startup under the shared simulation launcher.
Confidence Score: 4/5The mixed-visualizer regression should be fixed before merging because the newly advertised Both converter preview helpers return as soon as Kit is among the parsed visualizers, preventing the shared simulation context from initializing any additional requested visualizer backend. Files Needing Attention: scripts/tools/convert_urdf.py and scripts/tools/convert_mjcf.py Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
CLI[Converter CLI arguments] --> Detect{Standalone importer wheel installed?}
Detect -->|No| Kit[launch_simulation require_kit=true]
Detect -->|Yes| Runtime[launch_simulation require_kit=false]
Kit --> Convert[URDF or MJCF conversion]
Runtime --> Convert
Convert --> Viz{Requested visualizers}
Viz -->|kit present| Viewport[Show USD in Kit viewport]
Viz -->|kitless only| Sim[Create SimulationContext]
Sim --> Backends[Initialize Newton / Rerun / Viser]
Reviews (1): Last reviewed commit: "Bootstrap URDF/MJCF converters via launc..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if "kit" in visualizers: | ||
| # a Kit app that resolved without a GUI has no viewport to display the asset in | ||
| if AppLauncher.has_gui(): | ||
| sim_utils.show_stage_in_viewport(usd_path) | ||
| return |
There was a problem hiding this comment.
When the converter receives a mixed request such as --viz kit,newton, this branch returns immediately after handling Kit, so the SimulationContext that initializes Newton is never created and the requested Newton preview is silently omitted. The same early return affects the MJCF converter.
| # a Kit app that resolved without a GUI has no viewport to display the asset in | ||
| if AppLauncher.has_gui(): | ||
| sim_utils.show_stage_in_viewport(usd_path) | ||
| return |
There was a problem hiding this comment.
When --viz kit resolves to an app without a GUI, this branch returns without displaying the asset or explaining why the explicitly requested preview was skipped. Logging an actionable warning would distinguish this configuration from a successful preview; the MJCF converter has the same silent path.
![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 consolidation around launch_simulation is coherent, but the PR introduces four actionable regressions: it removes released converter CLI spellings without deprecation, imports isaaclab.sim before the required Kit runtime starts, drops established preview fallback and error-handling behavior, and categorizes breaking changes incorrectly in the changelog.
- Design and architecture: Using an additive
require_kitlauncher signal is consistent with the existing launcher-argument channel. However, both converter scripts importisaaclab.simat module scope before enteringlaunch_simulation; this conflicts with the deleted provider’s documented requirement to deferisaaclab.sim.utilsinitialization until after Kit starts and can leave Kit-dependent bindings unset on the wheel-absent path. - API: The converters regain the standard launcher arguments and
require_kitis documented, but replacing the converter-local visualizer parser immediately removes the released bare--vizand--viz autoforms. Those accepted CLI inputs require a deprecation period before removal. - Implementation: The rewritten preview path no longer selects CPU automatically when CUDA is unavailable, warns when no requested visualizer is created, or isolates optional preview failures after conversion succeeds. These behaviors should be retained or replaced proportionately. The changelog must also move the wheel-preference and breaking CLI entries out of
Fixed, with the breaking entry recorded underChangedaccording to repository rules.
Significant concerns. Posted 4 actionable findings inline.
Automated review; human maintainers own approval decisions.
| choices=["position", "velocity", "none"], | ||
| help="The type of control to use for the joint drive.", | ||
| ) | ||
| add_launcher_args(parser) |
There was a problem hiding this comment.
🟡 Warning · Api — Accepted viz spellings removed without deprecation
Replacing the converter-local --viz (which accepted a bare flag and auto) with add_launcher_args makes both spellings fail argparse on convert_urdf.py and convert_mjcf.py. These shipped as accepted CLI inputs in a prior release, and the repository rule requires deprecating public surface before removing it. Keep them accepted for one release, mapped to the resolved backend, with a deprecation warning.
|
|
||
| import os # noqa: E402 | ||
|
|
||
| import isaaclab.sim as sim_utils # noqa: E402 |
There was a problem hiding this comment.
🟡 Warning · Implementation — Simulation package imported before Kit launch decision
isaaclab.sim is imported at module scope, before launch_simulation runs inside main(). The deleted _importer_api.py explicitly deferred this import because isaaclab.sim.utils binds its Kit dependencies at import time from has_kit(), so importing it before Kit starts leaves those bindings unset for the process. On the wheel-absent require_kit path this affects the later enable_extension call. convert_mjcf.py has the same ordering.
| # Kitless preview: the physics backend ingests the USD stage and every visualizer renders the | ||
| # shared scene data, so no backend-specific code is needed here. Physics is not stepped -- the | ||
| # asset is shown in its imported pose until the visualizer window is closed. | ||
| sim = sim_utils.SimulationContext(sim_utils.SimulationCfg(device=args_cli.device, physics=physics_cfg)) |
There was a problem hiding this comment.
🟡 Warning · Implementation — Preview loses device fallback and error isolation
The deleted _preview_kitless selected cpu when CUDA was unavailable (the launcher device default is cuda:0), warned when no visualizer backend was created, and wrapped the preview so an optional viewer failure did not fail an already-written conversion. None of these survive: a CPU-only host now fails by default, an unavailable visualizer exits silently, and preview exceptions propagate out of main(). Same in convert_mjcf.py.
| script declares required positional arguments and is invoked with ``--help``. The launcher | ||
| arguments now appear in the help output of ``scripts/tools/convert_mesh.py``, | ||
| ``convert_urdf.py``, and ``convert_mjcf.py`` instead of an "arguments are required" error. | ||
| * Changed the URDF and MJCF converters to prefer the standalone ``isaacsim-asset-isolated`` |
There was a problem hiding this comment.
🟡 Warning · Implementation — Breaking and changed entries filed under Fixed
The wheel-preference bullet and the **Breaking:** Removed the converter-local --viz flag bullet sit under the Fixed heading. Repository changelog rules require entries in the correct category, with breaking changes under Changed prefixed by **Breaking:** and removals under Removed. As written, the compiled changelog publishes a breaking CLI removal as a fix.
ErenAta16
left a comment
ErenAta16
left a comment
There was a problem hiding this comment.
A -848 line removal is hard to review by reading, so I checked the two things a deletion of this shape usually gets wrong. Both are clean.
Nothing is left pointing at the removed modules
_converter_cli 0 references
ConverterCli 0
_importer_api 0
ImporterProvider 0
across .py, .rst and .md on the branch. The removal is complete rather than partial.
The CLI surface, including a false alarm worth recording
I diffed the add_argument calls in both converter scripts against main, and my first pass looked alarming:
convert_mjcf.py
main : --fix-base, --import-sites, --make-instanceable
PR : --merge_mesh, --collision_from_visuals, --collision_type,
--self_collision, --import_physics_scene
Three flags gone, five new ones, and the naming convention flipped from hyphen to underscore. Two checks later, neither is a problem:
migrating_to_isaaclab_3-0.rst:1858already states those three flags "are no longer available", and that file is onmainand untouched here. So the code was lagging the documented 3.0 migration and this PR closes the gap rather than opening one.- Every new flag registers both spellings (
"--merge_mesh", "--merge-mesh"), so the migration guide's--merge-mesh/--self-collisionexamples work against the implementation.convert_urdf.pydoes the same for--fix_base/--fix-base.
Recording this because a reviewer diffing the argparse surface will hit the same three-flags-missing result, and the reassurance is two files away.
convert_urdf.py lost nothing and gained five flags that previously came from ConverterCli, which matches the description.
Two small things
The usage block in convert_urdf.py lists one spelling. Line 25 documents --fix_base while line 54 registers --fix_base, --fix-base. Same for the others. Harmless, but someone reading the header won't learn the hyphen form exists, and the migration guide uses hyphens throughout — so the two docs teach different spellings for the same flag.
require_kit is overwritten unconditionally.
try:
metadata.distribution("isaacsim-asset-isolated")
args_cli.require_kit = False
except metadata.PackageNotFoundError:
args_cli.require_kit = Truesim_launcher.py:493 documents it as "whether the caller needs Kit for a reason cfg cannot express", and it's read at 524 through _get_arg. Today nothing registers it as a CLI flag, so there's no user value to discard and the assignment is the intended channel. Worth a short comment saying that, though — the name reads like an override the caller might have set, and if add_launcher_args ever grows a --require_kit, these two lines would silently win over it with no indication.
The early ImportError naming the wheel is a good touch. Reporting "neither Isaac Sim nor the standalone importer wheel is installed" is a much better first failure than the launcher's generic "Isaac Sim is absent", which is exactly the confusion the description mentions.
AppLauncher.add_app_launcher_args probes the command line with parse_known_args to check the caller's arguments for name collisions with the SimulationApp config fields. That probe exits the process when a required argument is missing, which is the case for every tool script that takes required positionals and is invoked with --help: the launcher arguments never reach the help output, and argparse reports "the following arguments are required" instead. The collision check only needs the declared argument names and their defaults, so fall back to those when the probe exits, and discard the usage line argparse writes to stderr on the way out.
The converter scripts carried their own process bootstrap in ConverterCli: a Kit-vs-kitless decision, an AppLauncher launch, teardown with a forced exit code, and a --viz flag with its own grammar. All of that already exists in launch_simulation, which derives needs_kit from the scanned config, skips AppLauncher when Kit is already running, closes the app in a finally block, and inherits --viz from AppLauncher.add_app_launcher_args. The one thing launch_simulation could not express is a tool that needs Kit for a reason its config does not describe. Add a require_kit override for that and use it from the converters, which need Kit only when the standalone isaacsim-asset-isolated wheel is absent. That also makes the wheel the preferred backend when both are installed, rather than always launching Kit whenever Isaac Sim is present. ImporterProvider goes with it. Loading the importer is a has_kit() guard around the extension enable followed by an ordinary import: the wheel exposes the same isaacsim.asset.importer.* namespace and needs no code of its own. Breaking: --viz auto and a bare --viz are no longer accepted; name the backend, e.g. --viz kit or --viz newton. In exchange the launcher's comma-separated form (--viz kit,newton) and the remaining launcher arguments (--device, --livestream, --experience) work again.
launcher_args is already the channel for launch signals a script contributes: sim_launcher reads `physics` and `distributed` from it without add_launcher_args ever defining them, and writes `visualizer_intent` into it as a purely config-derived signal. A dedicated keyword argument made a second mechanism for the same kind of information and put a new symbol on the public launch_simulation signature. Unknown keys are safe to carry: AppLauncher builds its SimulationApp config by intersecting launcher_args with _SIM_APP_CFG_TYPES, so `require_kit` is filtered out the same way `visualizer_intent` already is.
The collision check validates the parser's configuration, not a particular invocation: it reports arguments that duplicate names add_app_launcher_args owns, and arguments whose type cannot be ingested by SimulationApp. Reading them off the parser replaces the probe, the SystemExit handler it needed, and the stderr redirect that hid the probe's usage line. parse_known_args returns each argument's default when it is absent from the command line, so this is the same mapping for everything the user did not pass.
With no Isaac Sim and no importer wheel, the converters fell through to the launcher's generic diagnostic, which attributes the requirement to the PhysX backend and the Kit visualizer and never mentions the wheel. That points a kitless user at the one install they were trying to avoid, and it surfaced only after the converter config had been printed. Check for both sources right after parsing, as the converter CLI did before, so the failure names the wheel and happens before any work.
4ef82e4 to
3f387ce
Compare
4 participants
1. Summary
Folds the URDF/MJCF converter bootstrap into
launch_simulation, the launcher every other tooland demo script already uses.
ConverterCliandImporterProvider(added in #6460) are removed:their launch decision, teardown, and visualizer handling already exist in
launch_simulation,and their
--vizflag already exists as the launcher's--visualizer/--viz.Net +379 / −850.
This is a follow-up to #6460, not a revert of it. The kitless importer support that PR added is
kept and extended — the standalone wheel is now the preferred backend rather than unreachable
whenever Isaac Sim happens to be installed.
2. What changed
app/sim_launcher.pylaunch_simulationreads arequire_kitkey fromlauncher_args. The one thing the launcher could not express is a tool that needs Kit for a reason its config does not describe. Additive: it can only turn a kitless launch into a Kit one, never the reverse.sim/converters/_converter_cli.pylaunch_simulationderivesneeds_kitfrom the scanned config, skipsAppLauncherwhen Kit is already running, closes the app in afinallywith exit-code propagation, validates backend combinations, and syncs the kitless visualizer selection to carb.sim/converters/_importer_api.pyhas_kit()guard around the extension enable plus an ordinary import — the wheel exposes the sameisaacsim.asset.importer.*namespace and needs no code of its own.sim/converters/{urdf,mjcf}_converter.py_convert_asset, 7 lines each.scripts/tools/convert_{urdf,mjcf}.pyadd_launcher_args+launch_simulationpreamble, matching their siblingconvert_mesh.py.args_cli.require_kitis set from the presence of theisaacsim-asset-isolatedwheel.app/app_launcher.py--helpon scripts with required positional arguments (see below).test_converter_cli.pyandtest_importer_api.pydeleted;test_launch_simulation_require_kit.pyadded (3 tests); one--helpregression test added totest_argparser_launch.py.3. Why the flag came back
--vizis not a new flag.AppLauncher.add_app_launcher_argshas defined--visualizer/--vizfor a while, taking a validated CSV of
kit,newton,rerun,viser,none, and 57 scripts get it fromone line. #6460 removed
add_app_launcher_argsfrom these two scripts and reintroduced--vizlocally with a different grammar, so
--viz kit,newtonworked everywhere except the converters.Because the local flag's backend choice was driven by
AppLauncher.is_available()— "is Isaac Siminstalled", not "is Kit needed" — the wheel path was unreachable whenever Isaac Sim was present,
and the kitless visualizers had to be rejected with
parser.errorto cover for it. Derivingneeds_kitfrom the wheel instead removes both the rejection and the flag divergence.4. Why
require_kitis a launcher argument, not a parameterlauncher_argsis already the channel for launch signals a script contributes:sim_launcherreads
physicsanddistributedoff it withoutadd_launcher_argsever defining them, and writesvisualizer_intentinto it as a purely config-derived signal. A dedicated keyword argument wouldhave been a second mechanism for the same class of information, plus a new symbol on the public
launch_simulationsignature.Carrying the extra key is safe:
AppLauncherbuilds_sim_app_configby intersectinglauncher_argswith_SIM_APP_CFG_TYPES, sorequire_kitis filtered out beforeSimulationAppsees it — the same path
visualizer_intentalready takes.5.
--helpfix (first commit)add_app_launcher_argsprobed the command line withparse_known_args()to check for namecollisions with the SimulationApp config fields. That probe exits when a required argument is
missing, which is the case for every tool script that takes required positionals and is invoked
with
--help. Ondeveloptoday:The check validates the parser's configuration rather than a particular invocation -- it reports
arguments duplicating names
add_app_launcher_argsowns, and arguments whose type SimulationAppcannot ingest -- so it now reads the declared arguments off the parser instead of parsing argv:
parse_known_argsreturns each argument's default when it is absent from the command line, so thisis the same mapping for everything the user did not pass. The function already reaches into
parser._actions/parser._option_string_actionsa few lines above, to pop and restore the helpaction for this same eager-exit reason; this finishes that workaround's other half.
This fixes
convert_mesh.pyas well as the two rewritten scripts. Happy to split it into its ownPR if preferred -- it is self-contained and touches shared code.
6. Behavior changes
--viz autoand a bare--vizare no longer accepted. Name the backend(
--viz kit,--viz newton). The spelling they replace shipped in 15.0.0.--viz kit,newton,--device,--livestream, and--experienceare accepted again on bothconverters ([App] Add support for kitless URDF/MJCF importer #6460 dropped
add_app_launcher_args, which the review bot flagged as removing CLIsurface without a deprecation period).
isaacsim-asset-isolatedwheel is preferred over Kit when both are installed; Kit islaunched only when the wheel is absent or a Kit preview is requested.
--viz newtonwith IsaacSim installed is now possible rather than a hard error.
cost a full interpreter start plus a heavy import per run to turn a crash into a nicer message,
and could not prevent the crash it described.
Changelog fragment is filed as
.minor.rston the reasoning that the--vizspelling it breaksshipped in the release dated the same day; say the word if
.major.rstis preferred.7. Validation
Unit tests and lint:
test_launch_simulation_require_kit.py(4 tests, dict and namespace launcher args) —verified failing without the
require_kitchange.--helpregression test intest_argparser_launch.py— verified failing without theapp_launcher.pychange.test_argparser_launch.py+test_kwarg_launch.py+ the new file: 46 passed. A pristineupstream/developworktree in the same environment gives 41 passed with the identical1 failure (
test_matrix_headless_with_viz_names_takes_precedence) and 2 errors (missingpytest-mock), so both are pre-existing and unrelated.pre-commitsuite on all changed files.Both converters run against real assets (
source/isaaclab/test/sim/urdfs/test_merge_joints.urdfand newton's bundled
nv_ant.xml) in a kitless environment — newton installed, no Isaac Sim, noimporter wheel:
Isaac Sim and the
isaacsim-asset-isolatedwheel, before any conversion work. An earlierrevision of this branch regressed here: it fell through to the launcher's generic diagnostic,
which blames the PhysX backend and Kit visualizer and never mentions the wheel. Fixed in
Report both importer sources when neither is installed..dist-infoonPYTHONPATHso the distribution proberesolves) —
require_kitisFalse,launch_simulationtakes the kitless branch and startsno Kit app,
UrdfConverteris constructed, the config is translated,_convert_assetisreached, and
has_kit()correctly skipsenable_extension. Execution stops only atfrom isaacsim.asset.importer.urdf import ..., i.e. the real wheel contents.--help,--viz kit,newton, and--deviceon both scripts, plus--helponconvert_mesh.py.Not covered here — needs a reviewer with Isaac Sim or the wheel installed:
--viz kitviewport preview and the kitlessnewton/rerun/viserpreviews.test_urdf_converter.py/test_mjcf_converter.py, which need one of the two installs.🤖 Generated with Claude Code