Deprecates state properties calls#5423
Conversation
Add core MDP terms for body link pose and velocity fields so observation code can avoid packed state tensors. Update Pink IK base link lookup to read the explicit body link pose field.
AntoineRichard
changed the title
|
Addressed review feedback in
|
![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
Summary
This PR migrates three call sites from packed state tensors (body_link_state_w, root_state_w) to direct property accessors (body_link_pose_w, root_link_quat_w, body_link_vel_w). It adds new observation helpers for pose and velocity separately, and deprecates the combined get_all_robot_link_state() function for removal in IsaacLab 4.0. The changes are semantically correct and align with the codebase's direction toward explicit property access over tensor slicing.
Architecture Impact
Low impact, self-contained changes:
pink_task_space_actions.py: The Pink IK controller's base link frame lookup is internal to the action term. No downstream callers affected.dexsuite/mdp/rewards.py: Theorientation_command_error_tanhreward function is called by the reward manager based on task config. The semantic change is correct—root_link_quat_wprovides the same quaternion that was previously sliced fromroot_state_w[:, 3:7].pick_place/mdp/observations.py: New functions are exported via__init__.pyiand available to task configs. The deprecatedget_all_robot_link_statewrapper maintains backward compatibility.
Implementation Verdict
Ship it — Minor documentation improvement suggested.
Test Coverage
The PR description explicitly acknowledges that MDP tests were removed per review feedback and should be added in a separate PR. This is a gap:
- No regression test for
get_all_robot_link_state()deprecation warning - No test verifying
get_all_robot_link_pose()andget_all_robot_link_velocity()return correct shapes/values - The existing CI passes because the changes are semantically equivalent and existing task tests exercise the code paths indirectly
For a deprecation PR, the lack of explicit unit tests is acceptable given all CI passes, but the follow-up test PR should be tracked.
CI Status
All checks pass ✅. The comprehensive CI coverage (isaaclab core, isaaclab_tasks, training environments) provides confidence that the semantic changes are correct across backends.
Findings
🔵 Improvement: source/isaaclab_tasks/isaaclab_tasks/manager_based/manipulation/pick_place/mdp/observations.py:93 — Misleading variable name in deprecated function
The get_all_robot_link_state function's docstring is missing. While deprecated, it should still document what it returns for users who haven't migrated yet:
def get_all_robot_link_state(
env: ManagerBasedRLEnv,
) -> torch.Tensor:
"""Robot link states in the world frame (DEPRECATED).
.. deprecated:: 1.5.30
Use :func:`get_all_robot_link_pose` and :func:`get_all_robot_link_velocity` instead.
Will be removed in IsaacLab 4.0.
Returns:
Link states with shape ``[num_envs, num_bodies, 13]`` and layout
``[x, y, z, qx, qy, qz, qw, vx, vy, vz, wx, wy, wz]``.
"""🔵 Improvement: source/isaaclab_tasks/isaaclab_tasks/manager_based/manipulation/pick_place/mdp/observations.py:81-95 — Consider adding shape documentation consistency
The new get_all_robot_link_pose documents shape as [num_envs, num_bodies, 7] and get_all_robot_link_velocity as [num_envs, num_bodies, 6]. The deprecated function should document the combined shape [num_envs, num_bodies, 13] for completeness, but the torch.cat(..., dim=-1) correctly produces this.
🔵 Improvement: source/isaaclab_tasks/docs/CHANGELOG.rst:7-8 — Clarify which robots are affected
The changelog entry says "GR1T2 and Unitree G1 pick-place robot link pose and velocity MDP helpers" but the actual changes in observations.py are generic (hardcoded "robot" scene key). The changelog should either clarify these are generic helpers used by those tasks, or the functions should be documented as generic.
🟢 Verified Correct: source/isaaclab/isaaclab/envs/mdp/actions/pink_task_space_actions.py:221
The change from body_link_state_w.torch[:, self._base_link_idx, :7] to body_link_pose_w.torch[:, self._base_link_idx] is semantically equivalent. body_link_pose_w has shape [num_envs, num_bodies, 7] containing [x, y, z, qx, qy, qz, qw], which is exactly what the :7 slice was extracting from the state tensor.
🟢 Verified Correct: source/isaaclab_tasks/isaaclab_tasks/manager_based/manipulation/dexsuite/mdp/rewards.py:221
The change from root_state_w.torch[:, 3:7] to root_link_quat_w.torch is semantically equivalent. Both provide the root quaternion in [qx, qy, qz, qw] format. The root_link_quat_w property directly returns this without the slice overhead.
🟢 Verified Correct: source/isaaclab_tasks/isaaclab_tasks/manager_based/manipulation/pick_place/mdp/init.pyi:7-26
The stub file correctly exports the new functions and maintains the deprecated function for backward compatibility. The __all__ list and imports are properly synchronized.
Greptile SummaryThis PR reduces dependency on packed state tensors by switching call sites to read pose/velocity properties directly ( Confidence Score: 4/5Safe to merge; changes are semantically equivalent refactors with a clean deprecation path and no breaking behavior. All findings are P2 (style/non-critical). The core refactors are correct — body_link_pose_w returns the same 7-element pose as body_link_state_w[:7], and root_link_quat_w is equivalent to root_state_w[:, 3:7] for standard articulations. The compatibility shim is functionally correct but introduces a per-step torch.cat allocation for any remaining users of the deprecated function. observations.py — the torch.cat allocation in the deprecated shim and the per-step warning behaviour are worth a second look before IsaacLab 4.0 removal. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Observation / Reward Manager] -->|calls| B{Function}
B -->|new path| C[get_all_robot_link_pose
body_link_pose_w
shape: num_envs x num_bodies x 7]
B -->|new path| D[get_all_robot_link_velocity
body_link_vel_w
shape: num_envs x num_bodies x 6]
B -->|deprecated path| E[get_all_robot_link_state
emits DeprecationWarning]
E --> C
E --> D
E -->|torch.cat dim=-1| F[Returns num_envs x num_bodies x 13
for backward compatibility]
C --> G[Consumers: pick-place configs]
D --> G
H[Pink IK Action] -->|body_link_pose_w index| I[base_link_frame_in_world_origin
shape: num_envs x 7]
J[Dexsuite Reward] -->|root_link_quat_w| K[des_quat_w via quat_mul]
Reviews (1): Last reviewed commit: "Address state observation review feedbac..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| def get_all_robot_link_state( | ||
| env: ManagerBasedRLEnv, | ||
| ) -> torch.Tensor: | ||
| body_pos_w = env.scene["robot"].data.body_link_state_w.torch[:, :, :] | ||
| all_robot_link_pos = body_pos_w | ||
|
|
||
| return all_robot_link_pos | ||
| # TODO: Remove this compatibility helper in IsaacLab 4.0. | ||
| warnings.warn( | ||
| "`get_all_robot_link_state` is deprecated and will be removed in IsaacLab 4.0. " | ||
| "Use `get_all_robot_link_pose` and `get_all_robot_link_velocity` instead.", | ||
| DeprecationWarning, | ||
| stacklevel=2, | ||
| ) | ||
| return torch.cat((get_all_robot_link_pose(env), get_all_robot_link_velocity(env)), dim=-1) |
There was a problem hiding this comment.
DeprecationWarning is emitted inside get_all_robot_link_state, which is invoked by the observation manager on every environment step. Python's default warning filter (once) deduplicates on the (message, category, module, lineno) tuple, so the warning will fire exactly once per interpreter session from a fixed call site — which is fine for interactive use.
However, if the observation manager calls this function through a wrapper or dispatch table where the call-site line number varies (e.g., functools.partial, dynamic getattr, or vectorized dispatch), the filter key may differ and the warning can fire repeatedly. Consider adding a short note to the deprecation message or docstring advising users to suppress the warning in training loops with warnings.filterwarnings("ignore", category=DeprecationWarning).
| DeprecationWarning, | ||
| stacklevel=2, | ||
| ) | ||
| return torch.cat((get_all_robot_link_pose(env), get_all_robot_link_velocity(env)), dim=-1) |
There was a problem hiding this comment.
get_all_robot_link_state now issues torch.cat(...), allocating a fresh [num_envs, num_bodies, 13] tensor on every invocation, whereas the original code returned the existing body_link_state_w tensor zero-copy. Any task config still using robot_links_state will incur this allocation every step until migrated. Acceptable for a temporary shim, but worth noting for users who observe VRAM or step-time regressions before migrating.
# Conflicts: # source/isaaclab/config/extension.toml # source/isaaclab/docs/CHANGELOG.rst # source/isaaclab_tasks/config/extension.toml # source/isaaclab_tasks/docs/CHANGELOG.rst
2 participants
Description
Reduce higher-level dependency on packed state tensors in targeted IsaacLab call sites without changing existing task observation keys.
This PR:
body_link_pose_wdirectly instead of slicingbody_link_state_w;root_link_quat_wdirectly instead of slicingroot_state_w;get_all_robot_link_state()available for compatibility, but marks it deprecated for removal in IsaacLab 4.0;robot_links_statetask config entries unchanged.Fixes # (issue)
Type of change
Screenshots
N/A
Test Plan
./isaaclab.sh -p -m py_compile source/isaaclab/isaaclab/envs/mdp/__init__.pyi source/isaaclab/isaaclab/envs/mdp/actions/pink_task_space_actions.py source/isaaclab/isaaclab/envs/mdp/observations.py source/isaaclab_tasks/isaaclab_tasks/manager_based/manipulation/pick_place/mdp/__init__.pyi source/isaaclab_tasks/isaaclab_tasks/manager_based/manipulation/pick_place/mdp/observations.py source/isaaclab_tasks/isaaclab_tasks/manager_based/manipulation/dexsuite/mdp/rewards.py./isaaclab.sh -fgit diff --check origin/develop..HEADrg -n "body_link_state_w|root_state_w" source/isaaclab/isaaclab/envs/mdp/actions/pink_task_space_actions.py source/isaaclab_tasks/isaaclab_tasks/manager_based/manipulation/pick_place/mdp/observations.py source/isaaclab_tasks/isaaclab_tasks/manager_based/manipulation/dexsuite/mdp/rewards.py(no matches)./isaaclab.sh -pselects/usr/bin/python3.12withouttorch.Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists thereNotes:
DeprecationWarningtoget_all_robot_link_state()so users can migrate before IsaacLab 4.0.