Fix replay crash on trailing idle action in IK tasks - #6810
Conversation
Greptile SummaryThis PR prevents demonstration replay from applying a trailing idle action and makes absolute-pose IK commands robust to degenerate quaternions.
Confidence Score: 4/5The PR is not yet safe to merge because the previously reported adaptive-DLS diagnostic bypass remains unresolved. The adaptive-DLS path still sends non-finite Jacobians through Files Needing Attention: source/isaaclab/isaaclab/controllers/differential_ik.py Important Files Changed
Reviews (3): Last reviewed commit: "Drop adaptive DLS Jacobian guard and tig..." | Re-trigger Greptile |
There was a problem hiding this comment.
Isaac Lab Review Bot
The replay loop now avoids the trailing idle step, and the IK controller prevents zero-quaternion commands from producing NaNs. Two API details need correction before merge: the tolerance changes behavior for tiny nonzero quaternions, and the new absolute-pose fallback semantics are absent from the public contract.
- Design and architecture: The replay termination change preserves episode bookkeeping while removing the erroneous final step. The per-environment orientation fallback is appropriately localized, but its threshold should not broaden the fix beyond genuinely degenerate commands without an intentional compatibility decision.
- API:
set_commandpreviously normalized every nonzero quaternion, whereas quaternions below1e-6now select a fallback orientation. Additionally,ee_quatnow influences degenerate absolute-pose commands despite the docstring describing it as relevant only to position and relative-pose modes. Narrow the degeneracy condition or document the compatibility change, and document the new fallback behavior. - Implementation: The adaptive-DLS exception handling preserves the original exception for finite Jacobians and provides a clearer error for non-finite inputs. Tests cover zero-quaternion identity, current-orientation, and per-environment fallback paths, but do not cover compatibility for tiny nonzero quaternions. The required Isaac Lab changelog fragment is present.
Minor fixes needed. Posted 2 actionable findings inline.
Automated review; human maintainers own approval decisions.
AntoineRichard
left a comment
There was a problem hiding this comment.
AI-generated review — changes requested.
The replay termination and quaternion fallback behave correctly in the focused tests, but this needs another cleanup pass for showroom quality:
- Add direct regression coverage for the primary replay-loop fix. The controller tests do not prove that the extra idle step stays removed.
- Remove or redesign the unconditional adaptive-DLS finiteness diagnostic. Converting the device reduction to a Python boolean synchronizes the host on every controller step, and its comments/error overstate what can be inferred from a non-finite Jacobian.
- Keep comments, public documentation, tests, and the changelog functional and concise. The same incident narrative is currently repeated at every layer.
- Consolidate overlapping quaternion tests and remove the finite-Jacobian guard test already covered by the existing adaptive-DLS behavior test.
Verification: ./isaaclab.sh -p -m pytest source/isaaclab/test/controllers/test_differential_ik_features.py -q passed (21 tests) on the PR head; the targeted regression cases failed on the exact base commit; and ./isaaclab.sh -f passed. The three red CI jobs are unrelated rendering/numerical flakes or a task-environment process crash; all core IsaacLab shards passed.
replay_demos.py stepped the environment one extra time after every environment had exhausted its episodes: the step call sat outside the has_next_action check, so the loop applied the untouched idle action before terminating. No task defines idle_action, so it falls back to zeros -- and for absolute task-space tasks a zeros action carries a zero-norm quaternion. DifferentialIKController.set_command renormalized that command as quat / norm(quat), producing NaN. The NaN reached the joint position targets, diverged the articulation, and surfaced on the next decimation sub-step as an unrelated "torch.linalg.solve: the input matrix is singular" error -- after all demonstrations had replayed successfully. Stop the replay loop before the trailing step, and harden the controller so a degenerate command holds the current end-effector orientation instead of emitting NaN. The controller change is not redundant: with --num_envs > 1 an environment that finishes early keeps receiving the zero-quaternion idle action while the others replay, so it would diverge that environment and take down the whole batch. Also report a non-finite Jacobian in adaptive_dls by its actual cause rather than as an opaque LAPACK singular-matrix failure, which is what made this crash point away from the real defect. Reported by QA replaying IsaacContrib-Stack-Cube-SO101-IK-Abs-v0.
The non-finite Jacobian diagnostic was reached only when svdvals or solve raised LinAlgError, so it depended on backend error-reporting behavior. Backends that propagate NaN instead of raising bypassed it and let non-finite joint position targets reach the articulation. An Inf-valued Jacobian took that path even on CPU. Check finiteness up front instead, which covers both behaviors and drops the try/except. The check costs one device sync (~32us, flat in batch size); adaptive_dls is used only by the SO-101 teleop and replay task at frame rate, where that is immaterial. Extend the regression test over NaN, +Inf and -Inf, and add a case asserting a well-conditioned Jacobian is unaffected by the guard.
The 1e-6 norm cutoff was broader than the crash it fixes. A quaternion such as [0, 0, 0, 1e-7] normalizes cleanly to identity and did so before this branch, but the cutoff silently replaced it with the fallback orientation -- a behavior change beyond the zero-norm fix. Decide degeneracy by whether the normalization produced a finite result instead. This drops the magic constant and is exact at both ends: a zero quaternion (0/0) and a norm that underflows to zero (Inf) are still caught, while anything that normalizes keeps its previous meaning. A plain norm > 0 test would not cover underflow. Document the fallback in set_command, including ee_quat's role for absolute pose commands, which the docstring previously described as relevant only to position and relative-pose modes.
The replay-loop fix had no direct coverage: the controller tests still passed with the break removed, so the extra idle step could return unnoticed. Exercise replay_episodes_loop against a stub environment and dataset, asserting one env.step per recorded action and no trailing idle step. The script launches the simulator at import time, so the loop is extracted from the source and executed in isolation. Also trim the loop comment to the local, functional statement; the IK failure chain is incident history recorded in the PR.
The unconditional torch.isfinite(jacobian).all() check converted a device reduction to a Python bool on every adaptive-DLS control step, forcing a host sync solely to improve an exceptional error message. A non-finite Jacobian also does not prove the articulation diverged, and finite inputs do not guarantee finite downstream arithmetic, so the diagnostic claimed more than it could establish. Remove it along with its tests. State the quaternion fallback contract directly in the public docstring using :paramref:, replace the narrated comments with one functional line, consolidate the overlapping quaternion regressions, and make the changelog outcome-focused so it also covers the replay-loop fix.
4fe58a5 to
7798fef
Compare
Description
Replaying demonstrations for an absolute task-space (IK) task crashed after all
episodes had replayed successfully, with a
torch.linalg.solve ... input matrix is singularerror from the differential IK controller instead of exiting cleanly.Root cause is a trailing step past the end of the recorded data. In
replay_episodes_loop,env.step(actions)sat outside thehas_next_actioncheck, so once every environment had exhausted its episodes the loop applied the
untouched
idle_actionone last time before terminating. No task definesidle_action, so it falls back totorch.zeros(env.action_space.shape)— and foran absolute-pose action (
[pos_xyz, quat_xyzw, gripper]) a zeros action carries azero-norm quaternion.
DifferentialIKController.set_commandrenormalized it asquat / norm(quat),i.e.
0/0, producing a NaN target orientation. The NaN propagated into the jointposition targets, diverged the articulation, and only surfaced on the next
decimation sub-step as an unrelated "singular matrix" failure — which is why the
traceback points at the solver rather than at the defect.
Fixes:
scripts/tools/replay_demos.py: stop before stepping once every environment isexhausted, so the replay terminates without applying the idle action or running
another IK solve.
DifferentialIKController.set_command: a degenerate (zero-norm) commandedquaternion now holds the current end-effector orientation (identity when none was
supplied) instead of emitting NaN. Applied per-environment via a branchless
torch.where, so there is no added host sync on the training hot path.adaptive_dls: a non-finite Jacobian is reported by its actual cause instead ofthe opaque LAPACK singular-matrix / convergence failure. The check runs only on
the failure path, so the happy path is unchanged.
The controller change is not redundant with the script change: with
--num_envs > 1,an environment that finishes early keeps receiving the zero-quaternion idle action
every step while the others replay, which would diverge that environment and take
down the whole batch.
Reproduced and verified with
IsaacContrib-Stack-Cube-SO101-IK-Abs-v0. On theunfixed code the QA command produces a byte-for-byte identical traceback; with the
fix it prints
Finished replaying 1 episode.and exits 0.Note (out of scope):
idle_actiondefaulting to zeros is unsound for anyabsolute-pose task, since a zero quaternion is never a valid orientation. Seeding
the quaternion to identity would be a broader change affecting every task using the
replay script, so it is left for a separate PR.
Fixes # (issue)
Type of change
Checklist
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