Merge dexsuite into lift - #6732
Conversation
github-actions
Bot
added
documentation
|
Too many files changed for review. ( Bypass the limit by tagging |
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
Isaac Lab Review Bot
The dexsuite-to-lift consolidation is internally coherent across registrations, moved modules, inheritance, runner factories, presets, and rendering tests, but several merge-blocking compatibility and runtime issues remain: public task/module APIs are removed without prior deprecation, ADR restores the invalid object-bound floor, Franka exposes an unsupported OVPhysX gravity path, and zero rotation tolerance produces invalid reward behavior. Required skip changelog fragments are also missing for three touched packages.
- Design and architecture: The unified lift/reorient hierarchy and shared runner factories reduce duplication appropriately. However, the shared OVPhysX preset is exposed to Franka while the required removal of runtime gravity randomization remains implemented only in the KukaAllegro-specific configuration; that backend workaround should be colocated with the shared preset behavior.
- API: The moved entry points and imports are internally consistent, but
Isaac-Lift-Cube-Frankaand the publicisaaclab_tasks.core.dexsuitepath are removed outright. Repository policy requires a prior deprecation, so deprecated forwarding registrations/modules must preserve these existing API surfaces before removal. - Implementation: The static out-of-bound floor is correctly derived from table geometry, but the enabled ADR curriculum overwrites it with the old 0.3 m floor at initial difficulty. The
rot_std=0.0orientation branch also divides by zero and cannot latch success with a strict< 0.0comparison. In addition, test-only changes toisaaclab,isaaclab_rl, andisaaclab_physxrequire package-level.skipchangelog fragments.
Significant concerns. Posted 5 actionable findings inline.
Automated review; human maintainers own approval decisions.
| ## | ||
|
|
||
| gym.register( | ||
| id="IsaacContrib-Lift-Cube-Franka", |
There was a problem hiding this comment.
🟡 Warning · Api — Task ID renamed without deprecation alias
Isaac-Lift-Cube-Franka is no longer registered anywhere; only IsaacContrib-Lift-Cube-Franka exists. Existing commands, scripts, and gym.make() callers fail with an unregistered-env error, and the isaaclab_tasks.core.dexsuite module path is removed the same way. Repository policy requires deprecating public symbols in a prior release, so keep the old ID (and dexsuite forwarding modules) as deprecated aliases that warn.
| # The floor must sit below the object's resting height on the table, otherwise the | ||
| # episode terminates as soon as the object settles. Derived from the table top so it | ||
| # cannot drift: TABLE_SLAB[2] is 0.255 and a resting 0.05 cube centres at 0.28. | ||
| "in_bound_range": {"x": (-1.5, 0.5), "y": (-2.0, 2.0), "z": (round(TABLE_SLAB[2] - 0.1, 6), 2.0)}, |
There was a problem hiding this comment.
🟡 Warning · Implementation — Curriculum restores old out-of-bound floor
The derived floor (~0.155) only holds for the static config. AdrCurriculumCfg.oob_adr still writes terminations.object_out_of_bound.params.in_bound_range.z with initial_value=(0.3, 2.0), and the curriculum is enabled by default at difficulty 0, so the floor reverts to 0.3 on the first reset and a cube resting at ~0.28 terminates again — now also penalized by early_termination. Derive oob_adr's initial value from the same constant.
| physx = default | ||
| # ``physx`` resolves to whichever PhysX runtime the launcher picks; these force one. | ||
| isaacsim_physx = default | ||
| ovphysx = OvPhysxCfg( |
There was a problem hiding this comment.
🟡 Warning · Design Architecture — Shared ovphysx preset omits gravity workaround
PhysicsCfg now offers ovphysx to every lift/reorient robot, but only KukaAllegroMixinCfg strips variable_gravity and gravity_adr for OVPhysX, which its own comment states has no runtime gravity setter. Isaac-Lift-Franka and Isaac-Reorient-Franka now advertise physics=ovphysx while EventCfg.variable_gravity still calls randomize_physics_scene_gravity. Move the omission next to the shared preset instead of one robot config.
| pos_dist = torch.linalg.norm(pos_err, dim=1) | ||
| contact_mask = contacts(env, contact_threshold, thumb_name, finger_names) | ||
|
|
||
| if rot_std is not None: |
There was a problem hiding this comment.
🟡 Warning · Implementation — Zero rotation tolerance divides by zero
The changelog documents rot_std=0.0 as an exact-orientation goal, but this branch computes rot_dist / rot_std. An exactly aligned pose yields 0/0 and a NaN reward, and rot_dist < 0.0 can never latch success, so the documented behavior is unreachable. Handle zero as an explicit exact-alignment comparison, or reject it, instead of evaluating the tanh quotient.
| "Isaac-Reach-UR10", | ||
| # Manipulation Lift | ||
| "Isaac-Lift-Cube-Franka", | ||
| "Isaac-Lift-Franka", |
There was a problem hiding this comment.
🔵 Suggestion · Implementation — Missing changelog fragments for touched packages
Fragments are added only under isaaclab_assets/changelog.d and isaaclab_tasks/changelog.d, but the PR also touches isaaclab, isaaclab_rl, and isaaclab_physx (test-only edits). Repository policy requires one fragment per touched package; add a <slug>.skip file under each of those three changelog.d directories.
9f896e5 to
2170682
Compare
2170682 to
740705b
Compare
| super().__init__(cfg, env) | ||
| body_ids = cfg.params["body_asset_cfg"].body_ids | ||
| if isinstance(body_ids, list): | ||
| body_ids = torch.tensor(body_ids, dtype=torch.long, device=env.device) |
There was a problem hiding this comment.
favor int32 for these. The API is long friendly (and fast since the last changes) but still no reason to use a long.
There was a problem hiding this comment.
Claude / Codex, if you read this, look for other occurencies like that in this PR. Also make sure bodies / joint indices are not acquired and converted within a call, this is expensive.
mmichelis
left a comment
mmichelis
left a comment
There was a problem hiding this comment.
Thanks for the lift env cleanup! Aside from one or two questions (and the CI fix), it looks great!
| self.scene.robot = OPENARM_UNI_CFG.replace(prim_path="{ENV_REGEX_NS}/Robot") | ||
|
|
||
| # Scope the joint-space terms to the OpenArm's own joints. | ||
| openarm_cfg = SceneEntityCfg("robot", joint_names=OPENARM_JOINTS) |
There was a problem hiding this comment.
Give each manager term its own SceneEntityCfg. SceneEntityCfg.resolve() mutates the configuration by populating joint_ids. Because openarm_cfg is shared here, resolving policy/joint_pos mutates it, then resolving policy/joint_vel fails because the populated IDs are compared against the original regex names. This prevents IsaacContrib-Lift-Cube-OpenArm from initializing and currently fails the contrib environment smoke test. Please construct or copy a separate SceneEntityCfg("robot", joint_names=OPENARM_JOINTS) for each term.
| return ((pos_w < self._lower) | (pos_w > self._upper)).any(dim=1) | ||
|
|
||
|
|
||
| def abnormal_robot_state(env: ManagerBasedRLEnv, asset_cfg: SceneEntityCfg = SceneEntityCfg("robot")) -> torch.Tensor: |
There was a problem hiding this comment.
calling this abnormal robot state is a bit general, it currently specifically checks joint velocity limits, not nans or something. The termination should indicate it is specifically for joint velocity limits.
| func=mdp.body_state_b, | ||
| noise=Unoise(n_min=-0.0, n_max=0.0), | ||
| # good behaving number for position in m, velocity in m/s, rad/s, | ||
| # and quaternion are unlikely to exceed -2 to 2 range |
There was a problem hiding this comment.
should we put a termination on this if it exceeds it instead of clipping the state?
There was a problem hiding this comment.
taht could be a good follow up
| object_point_cloud = ObsTerm( | ||
| func=mdp.object_point_cloud_b, | ||
| noise=Unoise(n_min=-0.0, n_max=0.0), | ||
| clip=(-2.0, 2.0), # clamp between -2 m to 2 m |
There was a problem hiding this comment.
what's the reason for the clamp here?
There was a problem hiding this comment.
thats the workspace limit to control the quality of state observation data
Restore int64 indices because LEAPP's ONNX GatherND export requires them.
4 participants
Description
Important
Confirm the pull request base before submitting. Target
developfor allcontributions. The
release/3.0.0-beta2branch is a frozen stable landingsnapshot and is not used for ongoing maintenance.
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context.
List any dependencies that are required for this change.
Fixes # (issue)
Type of change
Screenshots
Please attach before and after screenshots of the change if applicable.
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