[omniperf-agent] test(sim): reproduce Newton/UsdPhysics rigid-body de… - #7452
[omniperf-agent] test(sim): reproduce Newton/UsdPhysics rigid-body de…#7452yts-nv wants to merge 1 commit into
Conversation
…scriptor crash Add a regression reproducer for the intermittent native crash in OpenUSD/usd-exchange UsdPhysics::moveDescsToDict<UsdPhysicsRigidBodyDesc>, reached via UsdPhysics.LoadUsdPhysicsFromRange (newton.ModelBuilder.add_usd) when building the Isaac-Shadow-Handover-Direct env on the Newton (newton_mjwarp) backend. - check_newton_usdphysics_shadowhand_crash.py: standalone reproducer; amplifies the offending load so the ~2-3%/call fault becomes ~100%/run. Prints COMPLETED_NO_CRASH (exit 0) only when the underlying bug is fixed. - test_newton_usdphysics_shadowhand_repro.py: pytest wrapper; runs the reproducer as a subprocess and asserts it completes. Skips on non-Ada GPUs and without Isaac Sim/Newton. The bug is in OpenUSD/usd-exchange UsdPhysics (or needs a Newton import_usd guard), NOT in Isaac Lab. Verified on develop 630317b on an L40 (Ada, sm_89): SIGSEGV in moveDescsToDict (exit 139) or a glibc heap-corruption abort. Expected to FAIL until the upstream fix lands; ship it in the same PR as that fix. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![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
Adds a standalone Newton/UsdPhysics native-crash reproducer and subprocess pytest wrapper, but the PR knowingly introduces a failing Ada test without the corresponding upstream fix. The test also violates the required launcher workflow and has unreliable completion and teardown handling. A changelog fragment is missing for the touched package.
- Design and architecture: Using a subprocess is appropriate for isolating a process-fatal native crash. However, adding the reproducer directly to the shared simulation suite before the upstream fix makes that suite intentionally red on targeted Ada systems. The wrapper also depends on the repository-root
isaaclab.shlauncher despite the repository requirement to run Python scripts and tests through the uv-managed environment. - API: No public Isaac Lab API, configuration, or export changes are introduced. The helper consumes existing AppLauncher, task-resolution, Gym, Newton, and UsdPhysics interfaces and monkeypatches
LoadUsdPhysicsFromRangeonly within the subprocess. - Implementation: The wrapper accepts the
COMPLETED_NO_CRASHmarker without requiring a zero subprocess exit code, although the marker is printed before teardown. Additionally, callingsimulation_app.close()beforeos._exit(0)cannot avoid the documented teardown deadlock, while thefinallycan convert teardown exceptions into success. The regression should land with the fix or be explicitly opt-in/xfail, use the prescribed uv-managed Python path, require coherent marker and exit-status signaling, correct teardown termination, and include the required changelog fragment or.skipfragment.
Significant concerns. Posted 5 actionable findings inline.
Automated review; human maintainers own approval decisions.
|
|
||
|
|
||
| @pytest.mark.skipif(not _HELPER.is_file(), reason="reproducer helper script not found") | ||
| def test_newton_usdphysics_shadowhand_no_native_crash(): |
There was a problem hiding this comment.
🟡 Warning · Implementation — Known-failing test added to shared suite
The module docstring and PR description state this test fails until an upstream OpenUSD/usd-exchange or Newton fix lands, and no such fix is included here. On Ada (sm_89) runners every non-skipped path ends in the assert or pytest.fail, making the shared isaaclab sim suite permanently red for a defect outside this repository. Land it together with the fix, or gate it behind an explicit opt-in marker/xfail.
|
|
||
| proc = subprocess.run( | ||
| [ | ||
| str(isaaclab_sh), |
There was a problem hiding this comment.
🟡 Warning · Design Architecture — Test shells out to isaaclab.sh launcher
Repository guidance reserves ./isaaclab.sh for installer workflows and requires uv run python for scripts and tests. Beyond the rule, _find_isaaclab_sh() returns None for installed/non-checkout environments, so the test silently skips and the regression is never exercised there, and the child may resolve a different interpreter than the parent. Launch the helper via sys.executable or the uv-managed Python entry point.
| ) | ||
| output = (proc.stdout or "") + (proc.stderr or "") | ||
|
|
||
| if "COMPLETED_NO_CRASH" in output: |
There was a problem hiding this comment.
🟡 Warning · Implementation — Success ignores subprocess exit code
The test returns as soon as COMPLETED_NO_CRASH appears in the captured output, without checking proc.returncode. The helper prints that marker before simulation_app.close(), so a SIGSEGV or heap-corruption abort during Isaac Sim teardown still leaves the marker in stdout and is reported as a clean run. Require both the marker and a zero exit code.
|
|
||
| print(f"COMPLETED_NO_CRASH task={args.task} num_envs={args.num_envs} loops={_loops}", flush=True) | ||
| try: | ||
| simulation_app.close() |
There was a problem hiding this comment.
🔵 Suggestion · Implementation — Hard exit cannot break teardown deadlock
simulation_app.close() runs before the finally: os._exit(0), so the stated purpose of the hard exit is defeated: if teardown deadlocks as the comment claims, the finally is never reached and the parent blocks until its 900s timeout. The finally also swallows teardown exceptions into exit 0, and makes the declared -> int return unreachable. Hard-exit directly after flushing the marker.
| @@ -0,0 +1,132 @@ | |||
| # Copyright (c) 2022-2026, The Isaac Lab Project Developers (https://github.com/isaac-sim/IsaacLab/blob/main/CONTRIBUTORS.md). | |||
There was a problem hiding this comment.
🔵 Suggestion · Implementation — Missing changelog fragment for isaaclab package
Repository rules require a fragment under source/<pkg>/changelog.d/ for every touched package, using .skip when no release note is warranted. This PR adds files under source/isaaclab without any fragment, leaving the release tooling with no record for the package.
Greptile SummaryAdds a subprocess-based regression reproducer for the intermittent Newton/UsdPhysics crash when constructing the Shadow Handover environment on Ada GPUs.
Confidence Score: 4/5The PR should not merge until the amplifier is made to repeat the LoadUsdPhysicsFromRange invocation that the target task actually uses. The target task reaches the wrapped binding with include_paths=["/"], so the new path condition skips all configured amplification and allows the intermittent native crash to produce false-green test runs. Files Needing Attention: source/isaaclab/test/sim/check_newton_usdphysics_shadowhand_crash.py Important Files Changed
Reviews (1): Last reviewed commit: "[omniperf-agent] test(sim): reproduce Ne..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if any("/World/envs/env" in r for r in roots): | ||
| for _ in range(_loops): | ||
| _orig_load(stage, include_paths, *rest, **kw) |
There was a problem hiding this comment.
/World/envs/env_{} layout, Newton takes the flat-load path and calls LoadUsdPhysicsFromRange with include_paths=["/"]. This condition therefore skips every repeated call, leaving only the original 2–3% crash opportunity and allowing false-green regression runs.
1 participant
…scriptor crash
Add a regression reproducer for the intermittent native crash in OpenUSD/usd-exchange UsdPhysics::moveDescsToDict, reached via UsdPhysics.LoadUsdPhysicsFromRange (newton.ModelBuilder.add_usd) when building the Isaac-Shadow-Handover-Direct env on the Newton (newton_mjwarp) backend.
The bug is in OpenUSD/usd-exchange UsdPhysics (or needs a Newton import_usd guard), NOT in Isaac Lab. Verified on develop 630317b on an L40 (Ada, sm_89): SIGSEGV in moveDescsToDict (exit 139) or a glibc heap-corruption abort. Expected to FAIL until the upstream fix lands; ship it in the same PR as that fix.
Description
Important
Confirm the pull request base before submitting. Target
developfor allcontributions. The
release/3.0.0-beta2branch is a frozen stable landingsnapshot and is not used for ongoing maintenance.
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context.
List any dependencies that are required for this change.
Fixes # (issue)
Type of change
Release backport
developScreenshots
Please attach before and after screenshots of the change if applicable.
Checklist
Docker and GPU tests run on demand. Push the commits you want tested, then
comment
run-cion the pull request.pre-commitchecks with./isaaclab.sh --formatsource/<pkg>/changelog.d/for every touched package (do not editCHANGELOG.rstor bumpextension.toml— CI handles that)CONTRIBUTORS.mdor my name already exists there