Updates core deprecation call sites#5409
Conversation
5a48b01 to
488c46a
Compare
Greptile SummaryThis PR migrates deprecated state/read/write helper APIs across core MDP actions and tests to their new Confidence Score: 5/5Safe to merge — all substitutions are mechanically correct and semantically equivalent to the deprecated calls they replace. Every changed call site was reviewed: pose slicing is equivalent (body_link_pose_w yields the same 7-element pose as body_link_state_w[:7]), joint state writes are split correctly before write_data_to_sim, root-state composition matches the old 13-element layout, and the NumPy 2.0 fix follows the canonical .detach().cpu().numpy() pattern. No logic changes, no new branches, no missing side effects were identified. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
subgraph old["Old (Deprecated) APIs"]
A1["body_link_state_w[:, idx, :7]"]
A2["write_joint_state_to_sim(pos, vel)"]
A3["write_root_state_to_sim(state)"]
A4["write_root_pose_to_sim(pose)"]
A5["write_root_velocity_to_sim(vel)"]
A6["set_forces_and_torques(...)"]
A7["set_joint_effort_target(...)"]
end
subgraph new["New (_index) APIs"]
B1["body_link_pose_w[:, idx, :7]"]
B2["write_joint_position_to_sim_index(pos)\nwrite_joint_velocity_to_sim_index(vel)"]
B3["write_root_link_pose_to_sim_index(pose)\nwrite_root_com_velocity_to_sim_index(vel)"]
B4["write_root_pose_to_sim_index(pose)"]
B5["write_root_velocity_to_sim_index(vel)"]
B6["set_forces_and_torques_index(...)"]
B7["set_joint_effort_target_index(...)"]
end
A1 -->|replaced by| B1
A2 -->|split into| B2
A3 -->|split into| B3
A4 -->|replaced by| B4
A5 -->|replaced by| B5
A6 -->|replaced by| B6
A7 -->|replaced by| B7
Reviews (1): Last reviewed commit: "fix: update core deprecation call sites" | Re-trigger Greptile |
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Summary
This PR migrates test files and one MDP action module off deprecated state/read/write helper APIs to the newer *_index variants. The changes are mechanical API updates replacing write_root_state_to_sim, write_root_pose_to_sim, write_root_velocity_to_sim, write_joint_state_to_sim, set_joint_effort_target, and set_forces_and_torques with their *_index counterparts. The version is bumped to 4.6.22 with a corresponding changelog entry.
Architecture Impact
Self-contained. All changes are in test files and one MDP action implementation. The deprecated APIs being replaced are wrappers that internally call the *_index methods, so behavior should be identical. No downstream consumers are affected since tests don't export APIs.
Implementation Verdict
Minor fixes needed — The migration is mostly correct, but there are a few inconsistencies and one potential bug in the test state capture logic.
Test Coverage
This PR is purely a deprecation cleanup, not a bug fix or new feature. The tests being modified are integration tests for the wrench composer and operational space controller. No new tests are needed, but the existing tests adequately exercise the migrated code paths.
CI Status
All relevant CI jobs pass (isaaclab core tests 1/3, 2/3, 3/3 ✅). The isaaclab_assets job was cancelled, likely due to unrelated infrastructure issues.
Findings
🟡 Warning: test_wrench_composer_vs_physx.py:571-581 — Inconsistent state capture vs reset pattern
The test captures initial state using root_link_pos_w, root_link_quat_w, and root_com_vel_w but then uses write_root_link_pose_to_sim_index and write_root_com_velocity_to_sim_index. This mixes link-frame pose with CoM-frame velocity, which is the intended pattern. However, the original code used root_state_w which contained root CoM data. Verify this doesn't change test semantics:
# Lines 571-581: Captures link pose + com velocity
initial_state_composer = torch.cat(
[
cube_composer.data.root_link_pos_w.torch,
cube_composer.data.root_link_quat_w.torch,
cube_composer.data.root_com_vel_w.torch, # CoM velocity
],
dim=-1,
).clone()This is actually correct — the link pose plus CoM velocity is the proper way to restore state when using the new APIs. No action needed.
🔵 Improvement: test_wrench_composer_integration.py:279-287 — Repetitive pattern could use helper
Multiple tests follow the same pattern of cloning pose, setting quaternion, writing pose, cloning velocity, zeroing it, writing velocity. Consider extracting a helper function to reduce duplication:
root_pose = cube_object.data.root_pose_w.torch[0].clone().unsqueeze(0)
root_pose[0, 3:7] = torch.tensor([0.7071, 0.0, 0.0, 0.7071], device=device)
cube_object.write_root_pose_to_sim_index(root_pose=root_pose)
root_vel = cube_object.data.root_vel_w.torch.clone()
root_vel[0, :] = 0.0
cube_object.write_root_velocity_to_sim_index(root_velocity=root_vel)This pattern appears ~10 times. A helper like teleport_cube(cube, pos, quat, zero_vel=True) would improve readability.
🔵 Improvement: test_operational_space.py:1500-1503 — Keyword argument style inconsistency
The migration uses positional style for some args and keyword style for others:
robot.write_joint_position_to_sim_index(position=default_joint_pos)
robot.write_joint_velocity_to_sim_index(velocity=default_joint_vel)
robot.set_joint_effort_target_index(target=zero_joint_efforts)The keyword style is good and consistent with the rest of the PR. This is actually correct.
🔵 Improvement: pink_task_space_actions.py:221 — Comment mentions "world origin" but variable name says "world_rf"
The comment says "Get base link frame pose in world origin" but the resulting variable is named base_link_frame_in_world_origin. After the fix, this reads from body_link_pose_w which is world-frame pose, making the comment accurate. The change from body_link_state_w.torch[:, self._base_link_idx, :7] to body_link_pose_w.torch[:, self._base_link_idx] is correct since body_link_pose_w is already shape (N, num_bodies, 7).
🔵 Improvement: CHANGELOG.rst:4-14 — Changelog entry is minimal but accurate
The changelog correctly documents the fix to PinkInverseKinematicsAction. The entry is appropriate for the scope of changes.
🟡 Warning: test_wrench_composer_integration.py — Missing .clone() on some pose reads could cause aliasing
At lines like 96 and 169:
root_pose = cube_object.data.root_pose_w.torch[0].clone().unsqueeze(0)This is correct. But at line 723:
root_pose = cube_object.data.default_root_pose.torch.clone()The default_root_pose is a property that returns a view. The .clone() is present, so this is safe. All pose reads appear properly cloned.
🔵 Improvement: test_interactive_scene.py:82-126 — Local variables created but immediately consumed
The pattern:
joint_pos = torch.rand_like(scene["robot"].data.joint_pos.torch)
joint_vel = torch.rand_like(scene["robot"].data.joint_pos.torch)
scene["robot"].write_joint_position_to_sim_index(position=joint_pos)
scene["robot"].write_joint_velocity_to_sim_index(velocity=joint_vel)This is cleaner than the original inline style and matches the idiom used elsewhere in the PR. Good change.
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Follow-up Review
The new commit (6b6beaa) appears identical to the previously reviewed commit. All previous findings remain valid:
- The migration from deprecated APIs to
*_indexvariants is correct - The
pink_task_space_actions.pyfix properly changes frombody_link_state_w.torch[:, self._base_link_idx, :7]tobody_link_pose_w.torch[:, self._base_link_idx] - The test state capture pattern in
test_wrench_composer_vs_physx.pycorrectly uses link pose + CoM velocity
Implementation Verdict
Ship it — The deprecation cleanup is mechanically correct and CI passes. The previous improvement suggestions (helper function extraction, etc.) are nice-to-haves but not blocking.
…ner/core-deprecation-warning-cleanup # Conflicts: # source/isaaclab/config/extension.toml # source/isaaclab/docs/CHANGELOG.rst
Summary
Verification
Rebased onto develop after PR #5304 merged.