NOT READY!! fix(newton): gate deprecated shape-color replacement… - #7450
NOT READY!! fix(newton): gate deprecated shape-color replacement…#7450yts-nv wants to merge 3 commits into
Conversation
… to avoid heap corruption replace_newton_builder_shape_colors traverses USD material bindings during Newton env cloning. On assets with malformed / out-of-scope PhysicsMaterial bindings (e.g. ShadowHand), this corrupts the glibc heap and aborts (SIGABRT: "malloc(): unaligned tcache chunk detected" / "malloc_consolidate(): invalid chunk size" / "double free or corruption"). Reproduced on L40 (Ada) with Isaac-Shadow-Handover-Direct @2048, presets=newton_mjwarp (~22% of runs under glibc malloc-hardening; ~1-3% in CI). Add an ISAACLAB_NEWTON_REPLACE_SHAPE_COLORS=0 escape hatch that skips this deprecated workaround (default preserves current behavior). A/B on the repro: 2 crashes / 9 runs -> 0 crashes / 18 runs with the workaround disabled. Adds a unit test for the gate. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
github-actions
Bot
added
bug
Greptile SummaryThe PR adds an environment-controlled opt-out for Newton’s deprecated USD shape-color replacement, allowing crash-prone material traversal to be skipped while preserving existing behavior by default.
Confidence Score: 5/5The PR appears safe to merge with no actionable correctness, security, or test-integration issues identified. The opt-out is checked before any stage access, default behavior remains unchanged, and the tests exercise both relevant gate states. Important Files Changed
Reviews (1): Last reviewed commit: "[omniperf-agent] fix(newton): gate depre..." | Re-trigger Greptile |
![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 environment gate is a minimal opt-out that preserves default behavior and bypasses USD traversal when set to "0". Before merge, the user-visible control needs API documentation and a changelog fragment, and the new test file needs the required SPDX header.
- Design and architecture: Placing the gate at the start of the deprecated helper directly avoids stage access and color replacement without changing the cloning path or default behavior. The process-wide control is coherent with the stated workaround, but its release and documentation integration is incomplete.
- API: The helper’s signature and return type remain unchanged, but its documented behavior is now conditional. Document
ISAACLAB_NEWTON_REPLACE_SHAPE_COLORS, its default, the exact"0"opt-out semantics, and the resulting zero return so callers can discover and correctly interpret the new behavior. - Implementation: The early return correctly occurs before the warning and all USD access, and the tests cover disabled and default-enabled paths. Add the required
source/isaaclab/changelog.d/fragment for this user-visible change and prepend the repository’s standard copyright/SPDX header to the new test file.
Minor fixes needed. Posted 3 actionable findings inline.
Automated review; human maintainers own approval decisions.
| Returns: | ||
| Number of shapes that had their colors replaced. | ||
| """ | ||
| import os |
There was a problem hiding this comment.
🟡 Warning · Implementation — Missing changelog fragment for isaaclab package
Repository rules require one changelog fragment under source/<pkg>/changelog.d/ for every touched source package when the change is user-visible (or a .skip fragment otherwise). This diff adds a user-facing opt-out to the isaaclab package but contains no fragment, so the release notes will omit the new gate and its usage guidance. Add a past-tense fragment under source/isaaclab/changelog.d/.
| Number of shapes that had their colors replaced. | ||
| """ | ||
| import os | ||
| if os.environ.get("ISAACLAB_NEWTON_REPLACE_SHAPE_COLORS", "1") == "0": |
There was a problem hiding this comment.
🟡 Warning · Api — New environment gate undocumented on public helper
The docstring (lines 209-229) still states unconditionally that shape colors are overwritten and that the return value counts replacements. With ISAACLAB_NEWTON_REPLACE_SHAPE_COLORS=0 the function returns 0 without touching the stage, and this toggle appears nowhere in the docstring or public documentation, so callers cannot discover it. Document the variable, its "1" default, the exact "0" semantics, and the gated return behavior.
| @@ -0,0 +1,34 @@ | |||
| """Regression test for the Newton shape-color-replacement gate. | |||
There was a problem hiding this comment.
🟡 Warning · Implementation — New source file missing SPDX header
Repository rules require the current SPDX header template on new source files. This file begins directly with its module docstring and has no copyright/SPDX lines, unlike adjacent files under source/isaaclab_newton/test/. Prepend the standard header block before the docstring.
…ape-color gate; move test into isaaclab core - Bump isaaclab 20.0.1 -> 20.0.2 and add a Fixed changelog entry for the ISAACLAB_NEWTON_REPLACE_SHAPE_COLORS gate. - Move the gate unit test into isaaclab core (source/isaaclab/test/sim/), where the gated function is defined, keeping the change within a single extension. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…untime instead of an env flag Replace the ISAACLAB_NEWTON_REPLACE_SHAPE_COLORS env-var gate with an automatic runtime guard. Before calling UsdShade.MaterialBindingAPI.ComputeBoundMaterial for a shape, verify its authored material:binding* targets resolve to prims present on the stage (_material_binding_targets_are_resolvable). Shapes whose bindings point outside the reference scope (e.g. the ShadowHand payload) are skipped, avoiding the native heap corruption (SIGABRT) with no user configuration required. Validated on Isaac-Shadow-Handover-Direct @2048 (presets=newton_mjwarp, glibc malloc-hardened): 0 crashes / 18 runs (vs 2 / 9 originally). Updates the unit test and changelog accordingly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
yts-nv
changed the title
1 participant
Description
replace_newton_builder_shape_colors resolves each Newton shape's bound material via
UsdShade.MaterialBindingAPI(shape).ComputeBoundMaterial(). On assets whose USD authoring has
out-of-scope material:binding* targets — e.g. the ShadowHand payload (many
_ReportErrors ... outside the scope of the reference warnings) — that traversal corrupts the
heap and aborts (SIGABRT: malloc(): unaligned tcache chunk detected /
malloc_consolidate(): invalid chunk size / double free) during env cloning. Intermittent
(~1-3% in CI; ~22% under glibc malloc hardening).
Fix
Detect malformed bindings at runtime: before ComputeBoundMaterial, verify every authored
material:binding* target resolves to a prim on the stage; skip shapes whose bindings point
out-of-scope (they keep their existing color). No configuration required.
Validation
L40/Ada, develop, Isaac-Shadow-Handover-Direct @2048, presets=newton_mjwarp,
GLIBC_TUNABLES=glibc.malloc.check=3:
Tests
(https://nvidia.slack.com/archives/D0AML1N5FJ7/p1788176012096689?thread_ts=1788145887.140769&cid=D0AML1N5FJ7)
Adds source/isaaclab/test/sim/test_newton_shape_color_gate.py (resolvable / out-of-scope /
no-binding). pytest: 3 passed.
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