fix(simulation/isaac): Isaac Sim backend - #3343
Open
rmncardoso wants to merge 176 commits into
Open
rmncardoso wants to merge 176 commits into
rmncardoso wants to merge 176 commits into
Conversation
…ilable `is_available()` required torch and answered `(False, "PyTorch not installed. Isaac Sim requires torch with CUDA support.")` where it was missing. Isaac Sim's runtime is PhysX + Warp and never imports torch, and `[sim-isaac]` does not declare it, so the requirement was unsatisfiable by the extra that enables the backend. Measured on `nvcr.io/nvidia/isaac-sim:6.0.1`, which ships no torch: Isaac steps physics, drives a joint to its commanded target and returns RTX pixels there, while this method reported it unavailable. Absence of torch now yields no verdict. A torch that IS importable and reports no CUDA device still yields False, because that is real evidence. The `torch.cuda.is_available()` call also moves out of the `try` that classifies the import, so an ImportError from CUDA init is no longer misreported as an absent torch (AGENTS.md: a `try` covers only the operation whose exception it classifies). The pre-existing `test_is_available_returns_tuple` passed on the bug because it only asserts `"Isaac Sim" in reason`, which the wrong reason contains.
… covers
PhysX builds its tensor simulation view at world.reset(), and adding or
deleting a physics-body prim afterwards invalidates it. Stepping that stale
view advanced the clock and simulated nothing while every envelope reported
success.
Measured on nvcr.io/nvidia/isaac-sim:6.0.1 (A10G), all three consequences were
silent:
* a cube spawned at z=0.600 was still at z=0.600 after step(90), which
reported "Stepped 90x ... 33 steps/sec"; the same step after a reset()
took it to z=0.025.
* an already-working robot's get_observation() went empty - 2 keys to 0 on a
2-joint URDF arm, back to 2 after the reset.
* after remove_object, reading that robot RAISED a bare Exception ("Failed
to get DOF positions from backend") out of a method the SimEngine ABC
documents as returning a dict.
add_object and remove_object now mark the scene, step refuses while the mark
stands and names reset() as the remedy, and get_observation answers empty -
its documented degraded mode - with a WARNING rather than raising.
Which mutations invalidate the view was measured, not assumed: add_camera,
remove_camera, move_object, add_robot and remove_robot each left that arm
reporting both its keys, so none of them marks the scene.
load_scene is the one path that repairs the view without a reset - it rebuilds
through SimulationManager.initialize_physics() and world.play(), deliberately
not world.reset() (strands-labs#1802) - so it clears the mark where that rebuild lands and
a scene load still steps. A reload that removes prior objects and realizes
none skips the rebuild and stays marked, which is the honest verdict.
The gate is Isaac-only and changes no cross-backend contract: it is inert
until a body mutation marks the scene, and sits behind the existing "No world
created" and "World not initialized" refusals. The two cross-backend step
stubs seed the field because they pass a types.SimpleNamespace as self, so no
attribute of the class is in scope for them.
Verified: 23/25 checks on live Isaac Sim (the 2 failures are one premise - the
procedural builders leave articulation as None and report 0 observation keys
at every point in the lifecycle, a separate defect); isaac suite 1961 passed;
shared simulation suite and MuJoCo suite both unchanged against baseline.
… reporting one
add_robot(name) with no asset path took a "procedural" branch whose comment
read "Build procedurally via USD API" and which made no USD call at all. It
read joint names off a hardcoded dataclass, registered a prim path for a prim
it never created, and returned success.
Measured on nvcr.io/nvidia/isaac-sim:6.0.1 (A10G): 0 prims under
/World/Robots/so100, _RobotState.articulation None before AND after
world.reset(), and get_observation() == {} at every point in the lifecycle.
That was the documented headline example.
It was also wrong as metadata, which is the half a caller could check without
a GPU. For one robot name the MuJoCo backend reports a different vocabulary,
and for panda a different count:
so100 table: shoulder_pan shoulder_lift elbow_flex wrist_flex wrist_roll gripper
MuJoCo: Rotation Pitch Elbow Wrist_Pitch Wrist_Roll Jaw
panda table: 7 joints (panda_joint1..7)
MuJoCo: 9 joints (joint1..7, finger_joint1, finger_joint2)
So the joint-name parity these docs promise was false before any physics was
involved.
Both halves are answered by loading the description MuJoCo loads. add_robot
now resolves the name (or data_config=) through
simulation.model_registry.resolve_model - the same resolver the MuJoCo
backend uses - so one name means one file on both backends.
Also adds MJCF support. mjcf_path was refused outright on the stated grounds
that this backend "has no MJCF robot importer" - an assertion this repository
made in three places and measured in none. Isaac Sim 6.0.1 registers
isaacsim.asset.importer.mjcf (MJCFImporter / MJCFImporterConfig, plus the
MJCFCreateAsset Kit command). The new isaac/mjcf_assets.py converts through
it, cached content-addressed over the description AND every file its directory
holds (an MJCF <include>s bodies and references a meshdir, so a key over the
named file alone would serve a stale conversion), and the result is loaded by
the existing native USD path - so a name, an MJCF, a URDF and a USD converge
on one proven loader.
Two vendor behaviours are handled rather than assumed, both measured:
usd_path is an output DIRECTORY root (it writes <root>/<stem>/<stem>.usda and
returns that path), and with no destination it writes beside the source, which
fails with "Read-only file system" for every description this package
resolves. fix_base defaults to None, the vendor default, which honours the
description - the only choice that keeps a floating-base robot floating.
Deletes the three hardcoded builders and the get_procedural_robot /
list_procedural_robots lookup. Beyond describing no real robot, that data could
not have been authored into a working articulation: JointDef carries no joint
anchor frame, BodyDef.position meant parent-relative from the loaders but
cumulative world-frame in those tables, JointDef.axis is a free vector where
UsdPhysics takes an X/Y/Z token (panda_joint4's (0,-1,0) is unrepresentable),
stiffness was 0.0 everywhere, and six unitree_g1 bodies declared mass=0.0.
ProceduralRobot / BodyDef / JointDef / _validate_kinematic_tree stay - they are
the return type and shared guard of load_urdf / load_mjcf / load_usd.
Two further fixes fall out. Naming two asset paths was newly ambiguous with
mjcf_path live, and the elif chain would have dropped the loser silently, so
the combination is refused with every path quoted. And
MuJoCoSimEngine._unknown_model_msg's three-way diagnosis moved to
simulation.base.unknown_model_msg now that Isaac resolves names too; the MuJoCo
method delegates and its text is byte-identical (verified over the typo,
hardware-only and asset-missing branches).
Verified: 24/24 checks on live Isaac Sim, including exact joint-name equality
with MuJoCo (9/9 panda, 6/6 so100), a wired articulation, a non-empty
observation, and the conversion cache not re-converting. Isaac suite 1983
passed; tests/simulation and the MuJoCo suite unchanged against baseline;
whole-tree graders unchanged against baseline (15 pre-existing env failures
both before and after).
replicate() was a stub that reported success for doing nothing. Its body was a
comment between two clock reads, so the "Build time" it quoted was the duration
of two assignments:
t0 = time.perf_counter()
# In full implementation: use omni.isaac.cloner.Cloner
# to replicate the scene N times
self._replicated = True
self._num_envs_active = n
elapsed = time.perf_counter() - t0
Measured on nvcr.io/nvidia/isaac-sim:6.0.1 (A10G), replicate(64) reported
"Replicated to 64 environments. Build time: 0ms. Device: cuda:0." with the
stage's prim count unchanged at 69, get_state() reporting num_envs: 64, and
add_robot refused from then on - so the no-op also locked the caller out of the
scene it had not replicated.
The module the comment named does not exist on 6.x: omni.isaac.cloner raises
ModuleNotFoundError. The cloner is isaacsim.core.cloner.
replicate() now clones through isaacsim.core.cloner.GridCloner. Every registered
robot and object is cloned into {stage_path}/envs/env_i on a square grid, with
replicate_physics=True and inter-environment collision filtering. The scene
already on the stage is env_0, so num_envs counts it: replicate(64) is the
source plus 63 clones.
Three things the vendor API required, each found by measurement rather than from
the signature:
* clone() raises "root_path needs to be specified!" whenever
replicate_physics is set, despite root_path defaulting to None. base_env_path
and root_path are different strings and both are needed.
* a clone lands only where the target's PARENT scope already exists, and
otherwise does nothing WITHOUT RAISING. Across six flag combinations (with
and without replicate_physics, copy_from_source, base_env_path, root_path)
every call returned cleanly and created zero env prims until each scope was
defined first.
* the UsdPhysics.Scene is at /physicsScene, at the stage root.
/World/physicsScene - the intuitive spelling under the configured
stage_path, and the one this first shipped with - is invalid, and
filter_collisions' failure there is non-fatal, so a wrong constant silently
degrades the fleet into one whose environments collide. It is now discovered
from the stage.
Every failure leaves the simulation un-replicated, so a retry is possible and
add_robot is not refused over a clone that did not happen: an absent cloner
extension, a raising cloner, and - the important one - a cloner that returns
cleanly having created nothing. That last case is refused, not reported, because
it is exactly the shape the stub had. The guard reads the expected clone PATHS
rather than a prim count, because counting is not evidence: define_base_env adds
one prim and this method defines one scope per clone, so an interim version
reported "7 clones" over an empty env root and passed its own count-based check.
num_envs and spacing are validated on the shared positive_whole_number_error /
positive_finite_number_error domains before any stage work. IsaacConfig
validated its own num_envs at construction but this argument bypassed that: a
negative count reported success having built nothing, and a bool passed as one.
replicate(1) is an accepted no-op and deliberately does NOT mark the simulation
replicated, so add_robot keeps working - marking it would refuse every later
add_robot over a call that cloned nothing, which is what the stub did for every
count.
The payload reports what was built rather than what was asked for:
clones_created, prims_created, build_time_ms, spacing, physics_replicated,
collisions_filtered. There is no per-environment observation or action API, and
the success text says so on every call, so num_envs: 64 is not mistaken for 64
drivable robots.
Also fixes the GPU test that should have caught this. Its docstring said
"replicate() must create the requested parallel environments" while asserting
only status == "success" and "16" in text - both of which a no-op produces, and
did. It now reads the stage: env prims must exist, one root per requested
environment, prims_created must equal the measured delta, build_time_ms > 0.
And three false fleet claims in docs/simulation/isaac.md: "fleet RL on PhysX GPU
with 1024+ parallel environments", num_envs described as "Set to 1024+ for fleet
RL" (setting it alone creates nothing), and a "Fleet preview" whose code never
called replicate() at all.
Verified: 21/21 checks on live Isaac Sim - 53 -> 231 prims, env_1..env_7 on the
stage, prims_created matching the measured delta exactly, collisions_filtered
true, a real 38ms build time, get_state reporting 8, and the fleet stepping.
Isaac suite 2010 passed; tests/simulation 3 failed, all pre-existing MuJoCo
rendering failures identical on a clean tree.
…y-detects-a-kit-app
…s-an-unreset-scene
…efuses-instead-of-fabricating
…resolves-a-real-asset
…d depth is shape-guarded Two asymmetries in the render path, both of which handed a numeric consumer a buffer it was promised could not arrive. 1. A substituted zero-depth buffer reached get_frame. When a camera carries no depth annotator, _render_frame logs a WARNING and returns np.zeros(rgb.shape[:2]). That is a fair degradation for the ENVELOPE path it also serves - render() only needs pixels - but get_frame passed it straight through, while its own docstring promised the opposite: "this method raises on every degraded path [...] so a compositing consumer can never silently receive black pixels with zero depth". The SimEngine.get_frame contract on the ABC says the same, and stronger: backends "must never substitute silently wrong pixels -- failures raise", reserving None for backends with no depth path at all (Newton). The consequence is not a subtly wrong image. HybridCompositor's per-pixel rule is valid_fg = isfinite(fg_depth) & (fg_depth > depth_epsilon) & ..., and its documented convention treats 0 as sky - so an all-zero foreground depth loses EVERY pixel and composites a frame containing the backdrop alone, with the simulated robot entirely absent. A plausible-looking photoreal image of an empty scene. A WARNING in a log the compositor does not read is not a refusal. _render_frame now marks whether the depth came from the annotator (meta["json"]["depth_is_real"]), the envelope keeps its degradation unchanged, and get_frame refuses a substituted buffer naming the camera, why zeros are not an acceptable answer, and both remedies. Raising rather than returning None follows this backend's own docstring and lets the message name the misconfiguration; the ABC reserves None for a backend with no depth path, which Isaac is not. 2. Depth had no shape guard while RGB had one. _render_frame refuses a malformed RGB buffer by shape, naming the shape. Depth was np.asarray(depth_raw) and nothing else, so three shapes got through: a 0-D or 1-D buffer reached a consumer promised (H, W); a buffer whose size differed from RGB's was returned as though the two described one frame, the two never having been compared; and a ragged buffer raised NumPy's "setting an array element with a sequence. The requested array has an inhomogeneous shape ...", which the handler reported as a generic "Failed to render camera", naming neither the depth buffer nor what shape was expected. Depth now has RGB's guard plus the RGB comparison. The ragged case is wrapped where it actually raises - inside np.asarray, which runs BEFORE any shape guard can see it, so the guard alone would not have covered it. An interim version of this change claimed otherwise in a test docstring; the test now reads the words the wrapping adds rather than the camera name the generic message already had. get_frame's Raises: block names the new refusal, per the rule that it is the only place a caller learns which handler to write. Verified: 10/10 on live Isaac Sim - the real annotator returns (480, 640) float32 with 71.67% of pixels carrying geometry (1.00m to 48.34m), get_frame accepts it, depth_is_real is True, the envelope still degrades to zeros, and get_frame refuses only the substituted buffer. 14 new unit tests, 11 of which fail on pre-fix code (the 3 that pass are controls). Isaac suite 1983 passed; tests/rendering + tests/simulation/isaac 2846 passed.
…s not exist
ISAAC_SIM_DOCKER_IMAGE was nvcr.io/nvidia/isaac-sim:6.0, and NVIDIA publishes no
major.minor tag for that image. Measured with docker manifest inspect:
isaac-sim:6.0 -> no such manifest
isaac-sim:latest -> no such manifest
isaac-sim:6.0.0 -> exists
isaac-sim:6.0.1 -> exists
isaac-sim:5.0.0 -> exists
isaac-sim:4.5.0 -> exists
What makes this worse than a stale docs line is where the constant is read: it is
the single source the RECOVERY instructions are composed from. is_available()
returns it in the hint it gives when the runtime is absent, and create_world()
names it in the structured error it returns for the same reason. So the one
message a user sees when they have no Isaac Sim told them to pull an image that
cannot be pulled. Pinned to 6.0.1, the tag this backend is verified against on an
A10G.
The new test grades the property a registry lookup would confirm - a resolvable
tag for this image carries all three version components - so it needs no network
call and fails on the exact value that shipped. The two pre-existing tests over
this constant assert only that it appears IN those messages, which a wrong tag
satisfies perfectly.
Docs, three claims the backend does not keep:
- create_simulation("isaac") does NOT raise when Isaac Sim is missing. The page
claimed a ValueError carrying the install hint, then said in the next sentence
that discovery is lazy - which is what is implemented and what
tests/simulation/test_factory.py pins. 54 direct constructions and 18 factory
calls in the suite depend on it, so the behaviour is deliberate and the docs
were wrong; I nearly changed working code. The page now describes the real path
and the deliberate contrast with Newton, which does raise ImportError because it
imports its runtime to construct.
- Replicator synthetic data is not implemented; there is no Replicator code in the
package. The bullet now names the RTX metric depth get_frame does return.
- enable_rtx_sensors was a documented IsaacConfig field nothing read, so
enable_rtx_sensors=False silently left RTX sensors on. Removed from the config
and the table: IsaacConfig refuses unknown keywords with a TypeError naming the
argument, so a caller who passes it is now told rather than ignored.
And the finding that costs the most to rediscover: the pip install route runs
physics but produces no RTX pixels. Measured on a g5.2xlarge (A10G) - a pip-wheel
install boots SimulationApp, steps physics, reports success, and every RTX camera
read comes back empty, while the same script under the 6.0.1 container on the same
instance returns real frames. It reproduces in pure Isaac Sim with no
strands-robots code in the process, so it is the install route and not the GPU,
the driver, or this backend. Nothing raises: render() degrades to a blank frame by
contract, so a rollout recording video writes an all-black MP4 and reports
success. Documented where the pip route is offered, with Docker listed first.
Deliberately NOT changed: the three robots-sim links, which all return 200 and are
already labelled historical; the fleet/num_envs claims, owned by the replicate
branch; and the observation-parity claim, owned by the add_robot branch.
Verified: Isaac suite 1976 passed; docs-grader selection unchanged against
baseline (10 = 10, delta 0); ruff and mypy clean.
…it makes meaningful Two halves of one defect, because neither is usable alone. Nothing on this backend could have a floating base. fix_base = True was hardcoded at BOTH URDF import paths - the modern URDFImporterConfig and the legacy _urdf.ImportConfig - with no way for a caller to change it. Every URDF robot was welded to the world, so a humanoid could not fall, a quadruped could not walk, and nothing said so. And the four base_* observation entries were absent. The SimEngine.get_observation schema requires that a robot whose root is a 6-DoF free joint surface base_pos, base_quat, base_lin_vel and base_ang_vel rather than reporting the free joint as a scalar. MuJoCo emits all four (mujoco/rendering.py) and Newton emits all four (newton/simulation.py); across all 11 files of the Isaac package there were ZERO occurrences. A locomotion policy reading base_lin_vel - the base twist every walking controller is conditioned on - got nothing here and a value on the other two, which is what made the documented "policies and observation mappings transfer unchanged between backends" false for a legged robot. The two are one change because the absence was consistent: with every base welded, those four keys would have reported four constants. fix_base is a parameter rather than something read out of the file because URDF cannot answer it. The format has a floating joint type, but the universal convention for a mobile robot is a root link with no parent joint - byte-identical to how a bolted-down arm declares its base - so the consumer chooses. That is why Isaac's own importer takes the flag, and why MuJoCo and Newton, which read MJCF's <freejoint>, have no equivalent. True remains the default: existing behaviour, and the shipped LIBERO Franka depends on it. It applies to urdf_path only. A USD asset carries its own articulation root and a procedural build authors its own prims, so fix_base=False there is refused naming urdf_path as the remedy rather than accepted and ignored - which would leave the caller believing they had a floating base and the observation keys believing the opposite. The flag is checked on the shared boolean_flag_error domain, so fix_base="false" is refused rather than read as truthy and silently welding the base of a caller who asked for the opposite. Known limitation, measured rather than worked around: a reset() does not preserve a floating base's spawn height (z=1.2 reads 0.0402 after a reset), because world.reset() re-applies each registered prim's default state on post_reset - the mechanism load_scene avoids a reset for (strands-labs#1802). Recording the spawn pose via set_default_state was tried and does not change that reading, so the docstring states it instead. Two speculative fixes were made and then REVERTED, because the measurement did not support them. An earlier probe reported the floating base reaching base_pos 5.6e9 at base_lin_vel 7.1e9, and I gated the strong PD-gain override (kp 1e5 / kd 1e4) on fix_base and recorded the spawn pose as the default state. A lifecycle diagnostic then showed the articulation still reports kp 1e5 for a floating base - so the gate changed nothing observable - and reset() still lands at 0.0402 with set_default_state in place. The blow-up was an artifact of that probe adding two robots and resetting, not of this feature: with one robot the fall is clean. Both changes carried comments claiming effects they did not have, which is the defect class this branch series exists to remove, so they are out. Verified on nvcr.io/nvidia/isaac-sim:6.0.1 (A10G): z 1.2 -> 1.1898 -> 0.9786 -> 0.2245 -> 0.05, settling on the ground at the base half-height, against fix_base=True holding z 0.0 -> 0.0. 28 new unit tests, 25 of which fail on pre-fix code (the 3 that pass are controls). Isaac suite 1997 passed; tests/simulation/isaac + tests/rendering 2860 passed.
…ivate arity A double for _load_urdf_robot / _load_usd_robot that declares exactly the four positional parameters the method has today fails as soon as the real signature grows - and it fails in the worst shape available: 'Failed to load URDF robot arm: fake_load_urdf() takes 4 positional arguments but 5 were given', a production-looking error envelope produced entirely by the stand-in. Measured composing this branch with the floating-base branch, which adds a fix_base parameter to that method: two tests here failed while both branches merged with no textual conflict, which is the semantic-composition shape AGENTS.md documents (strands-labs#1763/strands-labs#1766). What these tests are about is which loader the dispatch selects and with which path, so those stay captured positionally and anything added later is absorbed.
…s their consumers read The SimEngine.get_observation schema now documents "<joint_name>.vel", additive beside the position key. It was previously undocumented at the ABC and lived only in the MuJoCo implementation (emitted since strands-labs#761) - which is exactly how two backends shipped without it: Isaac read get_joint_positions() and never get_joint_velocities(), and Newton read joint_qd into a local for its floating-base twist and never emitted a scalar-joint entry from it. The gap broke real consumers three different ways, each on a policy that works unchanged on MuJoCo: the WBC balance controller degraded to zero joint velocities with a one-time warning - open-loop on the quantity it exists to feed back; the microduck and ProtoMotions observation packers raised KeyError; and an RL SimEnv with .vel in its actor_obs_keys refused at reset. Isaac reads velocities under their own handler so a handle predating get_joint_velocities (or a None read) degrades to positions-only rather than taking the already-read positions down with it - the schema requires joint state even when other reads fail - and None is omitted rather than zeroed: a present-but-zero velocity reads as a real measurement and silences the WBC warning that guards the absent case. Newton indexes joint_qd by _joint_dof_index, NOT _joint_coord_index: a free joint upstream shifts the two apart (7 position coords vs 6 velocity dofs), so the coord map would silently read a neighbouring joint's velocity for every joint downstream of the free one. The regression test plants a trap value at the wrong slot. And Newton's noise pass now splits by the .vel suffix exactly as MuJoCo's _apply_obs_noise does - joint_vel_std was accepted and documented by its set_obs_noise all along while the pass applied joint_pos_std to every entry, so the parameter configured a channel that did not exist and, once .vel entries exist, the unsplit pass would put position noise on them. Also corrected: the WBC policy docstring claimed MuJoCo's observation exposes joint positions only - false since strands-labs#761 - sending anyone diagnosing a zero-velocity gait toward the one backend that was fine. GPU-verified on nvcr.io/nvidia/isaac-sim:6.0.1 (A10G): j1.vel reads 0 at rest, jumps to exactly 10.0 - the URDF's declared velocity limit, which confirms both the units (rad/s) and that it is the true dof channel - after a send_action(1.2), decays 9.51/8.75/8.05/7.41 as the PD converges, and returns to 0 settled at j1=1.1998. Two earlier probe runs read a nonsense -44.2 rad/s on a static joint; the diagnosis chased reset()'s articulation revive before a per-step trace showed the probe URDF itself was self-colliding (the child link's box centered at the joint, wedged inside the parent's collision box - a jammed joint fighting interpenetration until PhysX put it to sleep). The backend was fine; the probe geometry was not. 11 new tests, 8 failing on pre-fix code (3 controls). Isaac + Newton + WBC + microduck + ProtoMotions + RL suites: 12826 passed. Whole-tree graders 15 = 15 against clean main.
…act query exists
eval_policy(success_fn="contact") resolves to the predicate DSL's contact_any,
whose never-raise contract turned a backend's NotImplementedError into False -
every tick, at DEBUG. On a backend whose get_contacts is still the SimEngine
raising stub (Isaac, Newton), the full evaluation ran to completion - GPU-hours
on Isaac - and reported success_rate: 0.0 with success_measured: True: a wrong
answer shaped exactly like a policy that failed every episode, with nothing
anywhere saying success was never measurable. evaluate's own docstring names
success_fn="contact" as the way to measure real task success, which is what
sent callers into it.
Three layers, one change:
- The resolver refuses up front. _resolve_success_fn("contact") raises
ValueError - which evaluate already returns as its structured error envelope
- when the backend's get_contacts is the base stub, BEFORE any rollout is
spent. The test is structural (did the subclass override the stub?) rather
than a probe call, because a real backend's get_contacts can fail for
world-lifecycle reasons that say nothing about the capability.
- The predicates say why they answer False. The contact read now has one owner
(_read_contacts, shared by contact_any and contact_between), which treats
NotImplementedError as the permanent fact it is - a WARNING once per backend
class, naming the remedy - instead of a per-tick DEBUG line indistinguishable
from a transient failure. The DSL stays never-raise: it is reachable directly
through benchmark specs, where refusing is not this layer's call. Transient
failures keep their DEBUG degraded mode, so operators are not trained to
ignore the capability warning.
- describe() stops advertising raising stubs. The base advertisement of
load_scene / randomize / set_obs_noise / get_contacts is now conditional on
the subclass overriding the stub. Isaac re-published all of them while their
calls raised NotImplementedError; Newton only avoided it by building its
describe() from scratch - a per-backend workaround for a base-class defect.
Gated structurally, so a backend that gains one starts advertising it with no
second edit, and MuJoCo advertises all four exactly as before.
The premise test asserting the unconditional advertisement is replaced rather
than deleted, per AGENTS.md: the discoverability it pinned still holds on
backends that implement the methods, and it now pins the conditional rule in
both directions.
10 new tests; pre-fix exactly the 5 controls pass and the 5 behaviour pins
fail. Full tests/simulation/: 14268 passed, 3 pre-existing MuJoCo GL failures.
Whole-tree graders 15 = 15 against clean main.
…t was built from
The IK solve runs on a compiled MuJoCo model, resolved until now from exactly
one place: the robot's data_config, through the registry. The file the robot
was actually BUILT from - the URDF add_robot imported, or the MJCF an imported
USD was converted from, both of which MuJoCo compiles - was discarded at add
time. Two costs, one loud and one silent:
- loud: a robot added via a bare urdf_path was refused move_to outright
("robot 'arm' has no data_config"), and a robot added by registry NAME was
refused too, because the name path stored data_config=None unless the caller
spelled it a second time;
- silent: a data_config naming a registry model that differs from the loaded
asset while sharing every joint name solved on the wrong kinematics, and the
convergence check runs by FK on the IK model itself - so the solve CONFIRMED
a pose the stage end-effector does not hold.
_RobotState now records description_path (URDF, or the pre-conversion MJCF;
None for a plain USD, which MuJoCo cannot compile) and _load_ik_mjcf prefers it
over the registry lookup - the IK model is the simulating file by construction.
The data_config path survives as the fallback for description-less robots (all
seven resolution refusals pinned by test_move_to_ik_model_resolution.py live on
it, unchanged - 75/75 still green). When both sources resolve to different
files the description wins with a WARNING naming both; same-file, the common
case by construction on the name path, stays silent. A description that fails
to compile is a structured error, not a silent registry fallback - falling back
would knowingly reintroduce the divergent-model solve this removes.
Stacked on feat/isaac-add-robot-resolves-a-real-asset, whose registry
resolution the name path's recording rides on.
GPU-verified on nvcr.io/nvidia/isaac-sim:6.0.1 (A10G): a 3-DOF arm added via
bare urdf_path, registered nowhere, solves move_to to [0.18, 0, 0.42] in 24
steps - and the end-effector's world pose read off the USD stage, independent
of anything the IK stack computed, measures the same 0.0296 m error the IK
reported, to four decimals: the IK model's FK and the stage agree because they
are the same kinematics. The first probe run refused with a genuine
"unreachable, residual 0.0539 m" verdict computed ON the recorded URDF - wrong
target for a 3-DOF arm, and itself evidence the resolution had already
switched, since the pre-fix text was "has no data_config".
10 new tests, 9 failing on pre-fix code (the passer is the registry-fallback
control). Isaac suite 2030 passed; whole-tree graders 15 = 15.
…ted, not inferred
load_urdf / load_mjcf / load_usd are exported in __all__, documented on the
Isaac docs page, exercised by ~17 test modules, and called by nothing inside
the package - the shape rule 10 ("no dead code") flags, and a shape a future
audit would flag again. Adjudicated: they stay, as the published
description-introspection API (parse a robot file into a joints/bodies report)
that the cross-backend parity suites also grade joint vocabulary, axis
defaults, free-joint spellings and pose conventions against. A caller-grep of
the package finds none BECAUSE the callers are library users and tests/:
add_robot builds articulations through Isaac's own importers and never calls
them, which the module docstring and the docs Loaders bullet now both state,
so the next reader does not assume add_robot goes through these and inherit
their semantics by mistake.
The rest of loaders.py was never in question: load_mjcf_scene_objects /
SceneObject are the load_scene path's parser, called from
strands_robots.simulation.isaac.simulation directly.
No behaviour change. Isaac suite 2020 passed, including the docstring-xref
grader that caught this very commit naming a sibling by filename.
…sumed missing actions
Both were the SimEngine raising stubs while
docs/simulation/domain-randomization.md and ~30 example call sites drive
randomize() and ~10 drive set_obs_noise() through the backend-agnostic surface
- the identical script randomized on MuJoCo and Newton and raised
NotImplementedError on Isaac.
Signatures mirror the MuJoCo reference exactly (names, defaults, parameter
order), so the derived-inventory shared-order grader held them to AGENTS.md
item 19 the moment they appeared. Unknown keywords refused by name; axis flags
on boolean_flag_error; ranges, position_noise and seed validated before
anything is written. Axes on this backend (measured on isaacsim 6.0.1): object
displayColor resampled (the handles ship no color setter; the USD attribute is
what RTX reads); UsdLux light intensity scaled in [0.5, 1.5] of base + color
resampled; dynamic-object mass scaled from base and each DISTINCT physics
material's friction scaled once (deduplicated by prim path); dynamic objects
teleported to base + uniform xy offset, z preserved (a z offset buries or
drops - and a buried body on this backend was measured to fling to 1e10 m).
The GPU verification caught this shipping broken, in exactly the class
AGENTS.md's truthiness rules name: the first-touch base registry was read with
getattr(self, "_dr_base", None) or {} - and __init__ pre-creates that dict
EMPTY, which is falsy - so a real engine rebuilt the base every call and
COMPOUNDED: mass_range=(2,2) applied twice took 0.105 kg to 0.421 kg (x4)
instead of 0.211 (x2), and one seed stopped reproducing one sample set. The
__new__-skeleton unit fixture could not see it: having no _dr_base at all, it
took the branch that stored the dict, and passed. Fixed with an is-None read,
pinned by a constructor-built engine test measured to fail on the falsy
spelling, and re-verified live: base x 2.0 exact.
set_obs_noise is applied by get_observation through a suffix-keyed pass
identical in shape to MuJoCo's: joint_pos_std on plain floats, joint_vel_std on
.vel floats, camera_jitter_px as an integer-pixel roll (pixels relocated, never
invented), base_* lists untouched.
GPU-verified 10/10 on nvcr.io/nvidia/isaac-sim:6.0.1 (A10G): every axis writes
what its report records, the RTX frame visibly changes under colors+lighting,
and a jittered frame rides get_observation. Two probe iterations were probe
bugs, recorded for honesty: the robot was added AFTER two objects, so its
articulation initialized against the invalidated tensor view (the defect the
step-refuses branch documents) - robots first; and a reorder script broke the
probe's indentation.
57 unit tests (the whole file fails pre-fix: the module did not exist);
isaac suite + shared-order grader 2202 passed; whole-tree graders 15 = 15
after the docstring-cites-a-test-filename grader caught this commit's own
module docstring and was obeyed.
The method was the SimEngine raising stub - which is what made eval_policy(success_fn="contact") unmeasurable on Isaac and every contact_* predicate answer False on a backend that simulates contacts perfectly well. With the override in place, the contact-success resolver's structural check admits Isaac automatically and describe() advertises the capability through the conditional gate - both keyed on the override existing. Mechanism, measured before any code was written: a PhysxContactReportAPI applied BEFORE the reset that builds the physics view produces per-pair headers with per-point position/separation/impulse; applied mid-simulation it produces nothing, 0 headers over every later step. So enrollment lives in add_object (threshold 0 - the question is "touching?", not "hit hard?"), and a pair is reported when either actor is enrolled: object<->ground and object<->robot (a grasp) are covered without touching robot prims. The handle-level contact APIs raise without a constructor-time contact view - a measured dead end. Records arrive in the exact shape MuJoCo emits and the predicate DSL reads (geom1/geom2 as names, dist, pos, active) plus impulse. Two semantics settled by measurement: - active requires a NONZERO IMPULSE. PhysX reports a speculative pair inside the contact offset as CONTACT_PERSIST at a plainly positive separation: a cube resting on another cube reported a "persisting" pair with the ground plane 0.12 m below it, so event-type-only active answers "touching" for bodies visibly apart - the exact proximity-for-touch substitution MuJoCo's active flag exists to prevent, and contact_between(top_cube, ground) would have answered True. A resting contact's impulse is ~3e-3 N*s; a speculative pair's is zero. The first GPU run shipped the event-only reading and the verification caught it. - the report is cached per step. PhysX hands the events once per fetch, so a second get_contacts call without an intervening step would read an empty report and answer "No contacts." for a cube still resting on the ground. GPU-verified 11/11 on nvcr.io/nvidia/isaac-sim:6.0.1 (A10G), including the speculative pair inactive beside two force-carrying pairs, contact_any / contact_between correct on the live engine through the shared DSL, and the same-step cache serving the populated answer. 12 unit tests over the pure translation (including the per-header data-offset walk), the cache, the refusals, and the add_object enrollment.
…de state The pin asserted Isaac does NOT advertise randomize / set_obs_noise / get_contacts - true when this gate landed, and falsified the moment sibling branches implemented those very methods, at which point the conditional describe() gate correctly advertises them and the hardcoded absence assertion turns a correct advertisement into a red test. Found by composing the twelve open branches; every branch was green alone. The equivalence actually owed is advertised-iff-overridden, and the test now asserts exactly that per method, with load_scene keeping it from being vacuously satisfied by absences.
Isaac Sim needs an RT-core GPU and the docs offered no way to get one, so
"you need an RT-core GPU" was the end of the sentence. examples/isaac_on_aws
stands a working Isaac host up from nothing and tears it down again:
./provision.sh g5.2xlarge (A10G), driver, docker, the pinned NGC image
./run_smoke.sh packs the local tree, runs 14 checks on the GPU
./teardown.sh terminates the instance and removes the security group
The deployment is OPTIONAL and nothing here is on the import path. The backend
resolves and runs against any local Isaac Sim install exactly as before;
create_simulation("isaac") neither knows nor cares whether the runtime came
from this example.
run_smoke.sh is what makes this a reference rather than a snippet: it exercises
the backend surface end to end on real hardware - create_world, a bare-URDF
add_robot, add_object, reset, step, a cube that actually falls, joint position
AND velocity observations, get_contacts, raycast, apply_force, randomize, and
both RTX colour and depth carrying real pixels. 14/14 on a fresh instance.
Two fixes the first end-to-end run found, neither of which a unit test could:
- ubuntu-drivers install --gpgpu RETURNS 0 WHILE INSTALLING NOTHING on this
AMI. No nvidia package, no kernel module, no nvidia-smi - and because the
exit code was a success, the "|| apt-get install nvidia-driver-535-server"
fallback never fired, so the instance reached the image pull with no driver
and the failure surfaced as a GPU that was simply absent. The driver is now
named explicitly. It loads without a reboot: the smoke run saw the A10G
immediately after modprobe.
- teardown.sh printed the security-group API's raw JSON to the operator's
terminal on the happy path, because the 2>/dev/null guard covered stderr
only while the AWS CLI writes its result to stdout.
Verified twice: once resuming the half-provisioned instance the first run left
behind, and once as a clean provision -> run_smoke -> teardown cycle with the
corrected script, which completed in about ten minutes, reported 14/14, and
left nothing running. Both instances confirmed terminated.
…resolves-a-real-asset
…ase-and-base-observation
…lves-on-the-loaded-asset
…bservation-on-every-backend
…s-not-measured-on-a-backend-without-contacts
…y-detects-a-kit-app
This was referenced Sep 12, 2026
This was referenced Sep 14, 2026
Member
|
State since the 09-11 note: this is now |
This was referenced Sep 16, 2026
Collaborator
Author
@cagataycali let me work on it |
Two conflicts, both resolved as unions rather than by taking a side, because each file has a side whose verbatim adoption is wrong and silent. `simulation/base.py` - the describe() methods mapping. This branch hoists the mapping into a `methods` local so a gate can pop the verbs the base class holds only as raising stubs; main goes on editing the literal in place, so the two shapes conflict by construction. Taking main's side compiles and passes ruff while quietly discarding the gate: `methods` is still built above the marker, still popped, then thrown away in favour of the inline literal. Kept the hoisted form, and carried main's strands-labs#3701 entry into it - that entry derives the bundled-benchmark roster with `_bundled_benchmark_roster()` where this branch still spelled three of the five benchmarks by hand. That divergence sits ~200 lines above the conflict markers, auto-merged, so resolving only the marked region would have reverted strands-labs#3701 unnoticed; `test_describe_names_every_bundled _benchmark_and_its_robot` is what would have caught it. The resolved mapping is now 29 entries matching origin/main by key, order and value. `simulation/isaac/recording.py` - the resume path. Main's strands-labs#3785 renamed the assignment to `recorder` so `_arm_dataset_recorder` owns the state write; this branch changed the line above it to verify the resumed schema against the base-expanded names. Two independent edits in one hunk. Taking this branch's side leaves `recorder` unbound where the new helper reads it. Kept both. Also rewrote four comment citations as `:mod:` roles. Main added `tests/test_docstring_module_xrefs.py` since the merge base, which grades comments as well as docstrings and forbids naming an internal module by source filename; the two Isaac files at the centre of this branch cited `mujoco/recording.py`, `newton/recording.py`, `mujoco/rendering.py` and `newton/simulation.py`, all four of which resolve to shipping modules. That grader sits under `testpaths = ["tests"]`, so it is inside the one required check: without this the resolved merge lands red. Measured both ways - reverting one citation fails the grader naming exactly those offenders, restoring it passes 27/27. Verified on the merged tree: ruff check clean, ruff format clean (2184 files), mypy clean (2184 files), whole-tree graders 5890 passed / 49 skipped, full suite 55857 passed / 337 skipped / 1 failed. That one failure, `test_cleanup_runs_before_the_interpreter_tears_down.py::...::test_the_exit_hook _releases_it`, fails identically on unmodified origin/main in the same environment, so nothing here regressed it.
yinsong1986
reviewed
Sep 17, 2026
yinsong1986
left a comment
yinsong1986
left a comment
Contributor
There was a problem hiding this comment.
Summary
Large Isaac-backend alignment PR: add_robot resolves registry names through the shared resolve_model() and imports MJCF via isaacsim.asset.importer.mjcf (joint-name parity with MuJoCo), floating-base robots gain the four base_* observation keys (body-frame angular velocity) and the matching dataset columns, status surfaces report resolved runtime state beside the requested config, several surfaces now refuse work they cannot complete instead of reporting success, and new capability lands (get_contacts, apply_force, raycast, randomize, set_obs_noise, description-based move_to IK) plus the examples/isaac_on_aws reference deployment. The three inline [MUST FIX] comments are all in the same family the PR itself fixes elsewhere: an error path that crashes instead of returning the error envelope, a staleness gate applied to one of four time-advancing loops, and a resume path that zero-fills the new base columns.
What's good
- The cross-backend hoists (
unknown_model_msgtobase.py, thedescribe()stub gating, joint-velocity observations) preserve MuJoCo/Newton semantics — all four gated capabilities resolve through the MRO correctly on all three backends, and Newton's from-scratchdescribe()is untouched. - The predicate DSL keeps its documented never-raise contract while
_resolve_success_fn("contact")fail-fasts inside the existingValueError-> error-envelope seam, per the no-silent-zero rule. provision.sh/teardown.shquote every AWS-response variable, the security group has no ingress rules, IMDSv2 is required, andtest_no_provisioned_instance_state_is_committed.pyenforces the state-file ban repo-wide.
Verification suggestions
- After addressing the staleness-gate comment: on the A10G box,
add_object(is_static=False)thenrun_policy(...)with no interveningreset()— confirm an honest refusal rather than a success envelope over a frozen scene. - For the recording comment:
start_recordinga floating-base robot, stop,start_recordingagain withoverwrite=False(resume), append an episode while the robot falls, and assert the appended episode'sbase_pos.zcolumn is non-constant.
| stop_exc, | ||
| ) | ||
| try: | ||
| World.clear_instance() |
Contributor
There was a problem hiding this comment.
[MUST FIX] World.clear_instance() raises UnboundLocalError when the failure precedes the World import, escaping the error-dict contract and masking the original error.
World is bound only by the function-local imports (~line 1609), inside the same try this handler covers — and the first fallible statement in that try is self._app = _get_or_create_simulation_app(...). SimulationApp(merged) raising RuntimeError/OSError (bad GPU, driver, display — the common real launch failures for this backend) lands in this handler with World never bound. The self._world is not None guard above correctly skips stop(), but this line then evaluates World.clear_instance() and raises UnboundLocalError — a NameError subclass, in neither the inner (RuntimeError, OSError, AttributeError) tuple nor the outer one — so it escapes create_world entirely, replacing the actionable launch error with cannot access local variable 'World'.
Blocking: crash on the changed error path, violating the action-handler contract (AGENTS.md > Review Learnings (#85) > "Return error dicts, never raise"), triggered by exactly the failure class this cleanup was added to handle. Fix: pre-bind World = None before the try (or guard on the name being bound) and skip clear_instance() when the import never ran — if World was never imported, this call registered no singleton, so there is nothing to clear.
| # (``_converge_render``, ``_refresh_all_render_products``) does | ||
| # not, because it advances no time. | ||
| if getattr(self, "_applied_wrenches", None): | ||
| self._reapply_wrenches() | ||
| self._world.step(render=bool(render_on and last)) |
Contributor
There was a problem hiding this comment.
[MUST FIX] The _physics_view_stale refusal guards only step(); this substep loop — and run_multi_policy._apply_all_and_step (~line 5926) and _warmup_camera (~line 6658) — still ticks the stale tensor view under a success envelope.
Reachable flow: add_object(is_static=False) sets _physics_view_stale = True (line 3407); the caller then drives run_policy(...) — which reaches physics through send_action, this backend's primary drive path — with no intervening reset(). step() would refuse; this loop advances _sim_time/_step_count, replays wrenches into a view that no longer covers the scene, and returns status="success" while the action silently does not apply — exactly the "single silent failure this backend had" that the step() refusal comment (line 2184) and changelog.d/3343-isaac-step-refuses-an-unreset-scene.md document, including the measured 2-minute hang on the post-remove articulation read. The wrench-replay work in this same PR already enumerated these time-advancing call sites and stated the rule ("every tick that advances _sim_time replays"); the staleness refusal needs the same rule applied. Related: step()'s per-batch re-check (line 2232) re-confirms only _world_created after releasing the lock between batches, so a worker-thread dynamic add mid-step(N) ticks the stale view for every remaining batch.
Blocking: silent data corruption — a policy rollout (and any recording of it) over an un-simulated scene reported as success, violating AGENTS.md rules 5/6 (never warn-and-continue when the system will behave unexpectedly; no silent defaults on error). Fix is mechanical: the same refusal at the three time-advancing sites (or an early return for _warmup_camera), plus _physics_view_stale in the per-batch re-check.
| state_names_full = list(joint_names) + [ | ||
| f"{src}.{comp}" for src, comps in base_state_specs for comp in comps | ||
| ] | ||
| self._verify_resume_schema( |
Contributor
There was a problem hiding this comment.
[MUST FIX] The resume path silently records zeros for all 13 base columns of a floating-base robot.
_DatasetRecorder.resume() never sets _state_source_keys — only create() does, via extra_state_specs (dataset_recorder.py:1424) — so a resumed recorder's add_frame falls back to the on-disk expanded schema names (base_pos.x ... base_ang_vel.z). The observation carries base_pos as a vector (that is what makes the create path work), so observation.get("base_pos.x") is None and the state_vals.append(0.0) fill fires (dataset_recorder.py:~1780) for every base component of every appended frame. The vector length still matches the schema, the recording hook passes required_action_keys (which disables the missing-state-column refusal), and this state_names_full validation change is precisely what admits the append — so every resumed floating-base episode records the base at the origin, at rest, under status: success. That is the "zero pose the robot is not in" corruption undriven_robot_state's own docstring names, and the base-blind-dataset defect changelog.d/3343-isaac-floating-base-and-base-observation.md cites as the reason for extra_state_specs — reintroduced on the append path this PR enables.
Blocking: silent dataset corruption (AGENTS.md rule 6 — no silent zero-valued defaults on error), undetectable from the schema and only discoverable by inspecting recorded values. Fix: give the resumed recorder the same source-key knowledge as create — e.g. set _state_source_keys from list(joint_names) + [src for src, _ in base_state_specs] after resume() (or add an extra_state_specs parameter to resume()), and pin it with a resume-append round-trip asserting a non-constant base column.
…urth the sweep found
All three verified by reproduction before being fixed, and each fix
mutation-tested: removed individually, its own pin goes red.
**A launch failure is reported, not replaced by a crash in its own cleanup.**
`create_world`'s handler tears the `World` singleton down, but `World` is bound
by a function-local import INSIDE the same `try`, after the first fallible
statement in it. `SimulationApp` raising RuntimeError/OSError - no GPU, no
driver, no display, i.e. the common real launch failure here - reached the
handler with the name unbound, so `World.clear_instance()` raised
`UnboundLocalError`. That is a `NameError` subclass, in neither except tuple and
deliberately so ("programming bugs propagate"), so it escaped `create_world`
entirely: a method whose contract is to return an error dict crashed, and the
actionable launch error became `cannot access local variable 'World'`.
Reproduced by stubbing the app factory. A separate `world_cls` is pre-bound
above the `try`, because rebinding `World` by import is a redefinition mypy
refuses. The teardown is skipped when it is still None - not on the grounds that
no singleton can exist (`_SIMULATION_APP` can be None while a World was built
outside this module's tracking) but because the pre-fix code raised AT the
`clear_instance()` call, so zero clears were ever performed on that path.
**Every tick refuses a stale tensor view, not just `step`.** The reviewer named
four sites; there are five. `send_action` is the reachable one - it is the drive
path a rollout reaches physics through, so `add_object(is_static=False)` then
`run_policy(...)` with no `reset()` advanced `_sim_time` over a scene PhysX no
longer covered and returned `status="success"` while the action did not apply.
The fifth is `motion_primitives._primitive_tick`, in a different file, whose own
comment calls it a peer of `step` and `send_action`; unguarded it burned its
budget and then blamed the SERVO, reporting "residual 0.3000 rad" for a joint
that was never going to move. Guarded at `send_action`, `run_multi_policy`
(preflight envelope plus a mid-loop raise, since the per-step hop returns None),
`_warmup_camera` (early False - it is declared `-> bool` and documents "never
raises"), `step`'s per-batch re-check (the lock is released between batches, so
a worker thread's dynamic add lands mid-`step(N)`), and the primitives' shared
`_primitive_resolve_robot` / `_primitive_abort_reason`.
One owner for the refusal, `_physics_view_stale_error`, MODULE-LEVEL and taking
the engine as an argument. It was a method first and that broke 32 existing
cross-backend tests: several drive `IsaacSimulation.step` with a
`types.SimpleNamespace` as `self`, which carries the flag but no methods. Same
reason `_resolved_physics_dt` is a function; the flag is read through `getattr`
for the 24 modules that build the engine with `__new__`.
**A resumed recording writes the base it observed - on every backend.** This one
is not Isaac-only. `resume()` never set `_state_source_keys` and cannot derive
them: it inherits the EXPANDED column names from disk, and nothing there says
which source a run of components was flattened from. So the fallback read
`base_pos.x`, the observation carries `base_pos` as a vector, `.get()` answered
None, and the zero-fill supplied 0.0 per component per frame. Nothing raised:
the flattened width still matched the schema, and the backends' hooks pass
`required_action_keys`, which disables the missing-column refusal. Reproduced
against `DatasetRecorder` directly, no backend involved - an observation
carrying `base_pos=[1.0, 2.0, 9.0]` recorded `[0, 0, 0]` while the joint columns
beside it recorded correctly, which is what kept it invisible. All three
backends pass `extra_state_specs` to `create` and all three `resume`, so the fix
is in the shared recorder and all three pass the sources; the call sites are
graded from source so a fourth backend is held to it on arrival.
Verified on this tree: ruff clean, ruff format clean, mypy clean (2187 files),
whole-tree graders 5890 passed / 49 skipped, full suite 55907 passed / 337
skipped / 2 failed. Neither failure is attributable: the MuJoCo atexit one fails
identically on unmodified origin/main in this environment, and
`test_hardware_task_loop_dispatch.py::...::test_on_the_nested_branch` is a
threading flake - 1 failure in 3 runs of that file on an unchanged tree, and it
imports none of the six changed modules.
yinsong1986
reviewed
Sep 17, 2026
yinsong1986
left a comment
yinsong1986
left a comment
Contributor
There was a problem hiding this comment.
Summary
Large Isaac-backend alignment series: add_robot resolves registry names through the shared resolve_model() and imports MJCF via isaacsim.asset.importer.mjcf for joint-name parity with MuJoCo; floating-base robots gain the four base_* observation keys (body-frame angular velocity, matching Newton and MuJoCo) and the corresponding dataset columns; status surfaces report resolved runtime state (device, physics_dt) beside the requested config; several surfaces now refuse work they cannot complete (get_frame without a depth annotator, ticking over a stale tensor view, contact-based success on a backend whose get_contacts is the raising stub); and new capability lands (get_contacts, apply_force, raycast, randomize, set_obs_noise, move_to solving IK on the loaded description) plus examples/isaac_on_aws as a reference deployment with a hardware smoke test.
What's good
- Head commit
fb4fe3e8addresses all three prior [MUST FIX] findings, and each fix checks out on inspection:create_world's failure handler pre-bindsworld_clsbefore thetryand gates teardown on it under separate narrow handlers; the stale-view refusal has one module-level owner (_physics_view_stale_error) guardingstep(with a per-batch re-check inside the lock),send_action,run_multi_policy,_warmup_camera, and the motion primitives;DatasetRecorder.resumenow takesjoint_names/extra_state_specsand all three backends pass them. - Per-change
changelog.d/fragments with measurements, mutation-tested pins, no host paths in tests, and the AWS example scripts quote every API-derived value, use a zero-ingress security group, SSM-only access, and IMDSv2-required — clean on the injection/secrets sweep.
Verification suggestions
- For the inline finding on
mjcf_assets.py: reproduce with two processes callingconvert_mjcf_to_usdon the same uncached MJCF against a sharedcache_dir(athreading.Barrier-style start with a stubbed slow importer works without a GPU) and assert the first process's returned path still exists after the second returns.
| f"return a path nothing can reference." | ||
| ) | ||
|
|
||
| _remove_tree(target_root) |
Contributor
There was a problem hiding this comment.
[MUST FIX] The install step can destroy a completed cache entry another process is actively using.
_remove_tree(target_root)
os.replace(staging, target_root)The cache root is shared cross-process (~/.strands_robots/asset_cache/usd_robots), and the pid-suffixed staging directory at line 247 shows concurrent converters are an intended case — but the install step defeats it. When two processes miss the marker for the same key and both convert (parallel test session on a cold cache, or several sims launching on one host), the loser's _remove_tree(target_root) deletes the winner's finished entry — after the winner wrote its marker, returned final, and its add_robot referenced that USD into a live stage. USD composes payloads lazily, so a read landing in the rmtree+replace window fails or composes the robot without its meshes, in a process whose own conversion was entirely correct. A second shape of the same race: if the loser's rmtree lands between the winner's os.replace and its marker write (line 273), the winner's open(marker, "w") raises FileNotFoundError out of a conversion that succeeded. Both contradict the module's own atomicity contract ("a torn cache entry is one a later call would trust", "no reader sees a directory without one").
Why this can't be deferred: it is a race mutating shared persisted state without a lock (AGENTS.md > must-fix > data corruption: "race conditions mutating shared state without locks"), on a cache this PR introduces and populates on every user's first run — and the failure surfaces as a nondeterministic stage-load error (or silently incomplete geometry) in a different, correct process, which is close to undebuggable in the field.
Fix: never delete a completed entry. After conversion, re-check the marker; if a valid entry appeared while converting, discard staging and return the existing path. Install only into an absent target_root via os.rename, treating FileExistsError/OSError(ENOTEMPTY) as "lost the race" (re-read the winner's marker and return it). Both converters produce identical content for a given key, so the winner's entry is always the right answer.
# Conflicts: # docs/recording.md
yinsong1986
reviewed
Sep 17, 2026
yinsong1986
left a comment
yinsong1986
left a comment
Contributor
There was a problem hiding this comment.
Summary
Large Isaac-backend alignment series bringing the backend onto the shared SimEngine contract: add_robot resolves registry names through the same resolve_model() MuJoCo uses and gains MJCF support via a new content-addressed MJCF-to-USD conversion cache (mjcf_assets.py); floating-base robots are detected from the MJCF <freejoint> and report the four base_* observation keys (body-frame angular velocity, matching Newton/MuJoCo) plus the matching dataset columns on both create and resume; status surfaces report resolved runtime state (device vs device_requested, read-back physics_dt) instead of echoing config; get_contacts, apply_force, raycast, randomize, set_obs_noise and description-based move_to IK land with input-domain validation throughout; and examples/isaac_on_aws/ adds an SSM-only (no-ingress, IMDSv2-required) reference deployment with a 26-check hardware smoke. The concerns from the prior review rounds are addressed at this head: the create_world failure handler pre-binds the World symbol and splits stop()/clear_instance() into separate handlers, the _physics_view_stale refusal now covers all physics-advancing sites (send_action, run_multi_policy, _warmup_camera, motion primitives) via a single module-level owner, and DatasetRecorder.resume takes joint_names/extra_state_specs (backward-compatible trailing keyword defaults) with all three backends passing them.
What's good
- Prior-review fixes each landed with a pinning regression test (per AGENTS.md "Pin every reviewed fix"), and the pump/render walks snapshot registries under the lock as the changelog claims.
- Input validation is centralized on shared domains (
coerce_pose_vector,entity_name_error,boolean_flag_error) before anything reaches PhysX or USD prim paths. - The AWS example is genuinely no-ingress (SSM only, empty security group,
HttpTokens=required), andtest_no_provisioned_instance_state_is_committed.pyguards the state file. - No host paths, no secrets, no non-ASCII in user-facing strings across the +16k diff.
Verification suggestions
- For the inline cache-digest finding:
python -c "from strands_robots.simulation.isaac.mjcf_assets import _asset_digest; print(_asset_digest('<assets>/lekiwi/lekiwi/lekiwi.xml'))"before and after touching<assets>/lekiwi/so_arm100/so_arm100.xml— the digest does not change, so the cached USD is served stale.
| """ | ||
| root = os.path.dirname(os.path.abspath(mjcf_path)) | ||
| manifest = hashlib.sha256() | ||
| for dirpath, dirnames, filenames in os.walk(root): |
Contributor
There was a problem hiding this comment.
[MUST FIX] The cache digest walks only the MJCF's own directory, but the shipped registry's nested-layout assets reference files outside it — so the cache serves a stale or wrongly-shared USD with status: success, which is exactly the staleness this docstring says the digest exists to prevent ("hand back a stale USD after any change that did not touch the one file named").
root = os.path.dirname(os.path.abspath(mjcf_path))
...
for dirpath, dirnames, filenames in os.walk(root):Measured against the assets the registry actually resolves (AGENTS.md > Registry conventions documents these nested layouts):
asimov_v0->xmls/asimov.xml, which declaresmeshdir="../assets/meshes"— every mesh PhysX simulates is outside the digest root.lekiwi->lekiwi/lekiwi.xml, which contains<include file="../so_arm100/so_arm100.xml"/>— the entire arm's joint and geom definitions are outside the digest root.- Same shape for
jvrc,aliengo,unitree_a1,reachy_mini(xml/- andmjcf/-subdir entry points).
Concrete trigger: an asset re-download or upstream update changes so_arm100.xml (or any mesh under ../assets/) while the entry-point file's directory is byte-identical -> same key -> convert_mjcf_to_usd returns the USD built from the old description, silently. The converse collision also holds: two trees whose entry-point subdirs are identical but whose sibling assets differ share one cache key. This is must-fix as silent data corruption in a persisted on-disk cache: the wrong geometry/kinematics is simulated under a success envelope with nothing anywhere to detect it, and it defeats the joint-name-parity guarantee this module exists to provide (the wrong so_arm100.xml is precisely a wrong joint vocabulary). Note this is distinct from the install-race concern already raised at line 270 in a prior review — that one is about concurrent installs; this one is about the key being computed over the wrong file set.
Resolution: hash the closure of files the MJCF actually references — resolve <include> targets (the _mjcf_model_toplevel splice in loaders.py already walks these) and the meshdir/assetdir trees — or, more conservatively, walk from the asset root (resolve_urdf's registered base directory) rather than dirname(mjcf_path).
Member
|
State today: MERGEABLE again after the rebase and the required check is green on 06480c7 (call-test-lint SUCCESS). Two gates left, both human: 5 of 9 review threads are unresolved, and there is no approving review. @rmncardoso could you resolve the five, and @yinsong1986 review after that. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The Isaac backend implements the same
SimEnginecontract as MuJoCo and Newton, andthis brings its behaviour in line with that contract in a few places where the three
had drifted apart.
add_robotnow resolves a robot name through the sameresolve_model()the MuJoCobackend uses, so one name means one description file and the two backends' joint
vocabularies stay in step. MJCF descriptions are imported through
isaacsim.asset.importer.mjcf, which reaches exact joint-name parity with MuJoCo onthe shipped registry.
A robot whose description declares a free root is now recorded as floating-base, so
get_observationreportsbase_pos/base_quat/base_lin_vel/base_ang_velas the schema specifies — with angular velocity in the body frame, matching the other
two backends and the convention locomotion policies are trained against. Those signals
also reach the dataset, so a recording of a legged robot carries its base state.
Status reporting now reads from the runtime rather than from the requested
configuration:
get_statereports the device PhysX resolved alongsidedevice_requested, and the physics timestep the world is integrating alongside the oneasked for. Where the two differ, both are visible.
A few surfaces now decline work they cannot complete instead of reporting success —
get_framewhen a camera has no depth annotator,stepwhen a dynamic body has beenadded since the last
reset(),set_joint_positionswhen no pump will apply thewrite. Each names what to do instead.
The rest is new capability:
get_contacts,apply_force,raycast,randomize,set_obs_noise,move_tosolving IK against the loaded description, joint-velocityobservations on all three backends, and
examples/isaac_on_aws/— a referencedeployment whose smoke test drives 26 checks against real hardware.
Each change's reasoning and measurements are in its
changelog.d/fragment.Verification
g5.2xlarge,nvcr.io/nvidia/isaac-sim:6.0.1)examples/isaac_gsandexamples/so101_curoboboth build and step, reporting fulljoint state
base_*keys present,body-frame angular velocity confirmed against an independent rotation matrix, 13 base
columns in the dataset schema
Scale: 109 files, +15269/−894, 77 non-merge commits.