[Task Clean-up] Dexterous Part 9/9: Move the warp Direct task variants to isaaclab_tasks_experimental

[hujc7]

Review Map

• Exact changes: a stacked PR's page shows the cumulative diff of its dependency chain; the link pins the commit range that is the PR's own contribution.
• Links pin specific SHAs and can go stale after a branch update — the table on [DO-NOT-MERGE][Task Clean-up] Dexterous: lumped validation branch (series reference) #6324 is refreshed first.

PR	Status	Depends on	Exact changes
#6411 Part 1/9: Newton cloner/cubric/visualizer fixes		—	merged
#6412 Part 2/9: OVPhysX articulation + manager runtime		—	merged
#6413 Part 3/9: Reorient Direct, torch		—	changes
#6414 Part 4/9: MARL-to-single-agent fix + handover/camera Direct		#6413	changes
#6418 Part 5/9: Reorient manager counterparts		#6413	changes
#6421 Part 6/9: Handover + camera manager counterparts		#6413, #6414, #6418	changes
#6410 Part 7/9: Environment overview docs		#6421	changes
#6415 Part 8/9: Benchmark success-rate utilities		#6421	changes
📌 #6582 Part 9/9: Warp variants → experimental (draft; merges last) (this PR)		#6413	changes
#6324 [DO-NOT-MERGE] Lumped validation reference		ALL	—

Summary

• Moves the warp-first implementation of the Shadow reorientation and handover Direct environments (kernels, environments, kernel behavior tests) to isaaclab_tasks_experimental, following the torch-first mainline convention: torch task logic in isaaclab_tasks, warp implementations in the experimental track.
• The variants register under -Direct-Warp-v0 IDs and reuse the stable configurations and agents from isaaclab_tasks.core. The Allegro warp variant keeps its existing legacy registration in the experimental package.
• The diff is largely the review-polished warp implementation from the lumped validation branch [DO-NOT-MERGE][Task Clean-up] Dexterous: lumped validation branch (series reference) #6324, relocated with import-path updates.

Stacking

• Based on [Task Clean-up] Dexterous Part 3/9: Add success-rate metrics to the reorientation Direct tasks #6413's branch; this page shows the cumulative diff until [Task Clean-up] Dexterous Part 3/9: Add success-rate metrics to the reorientation Direct tasks #6413 merges. Merges last in the series.
• Deeper consolidation with the experimental warp track (DirectRLEnvWarp) and the performance focus for the experimental variants are follow-on scope, out of this series.

Validation

• 44/44 warp kernel behavior tests; pre-commit green. Full integrated training/validation evidence: [DO-NOT-MERGE][Task Clean-up] Dexterous: lumped validation branch (series reference) #6324.

[AntoineRichard]

[AI Review] Requesting changes. The current Part 9 head is not merge-ready: the new Gym registrations and environment modules contain stale imports, and the camera PhysX-default override is ineffective under the current PresetCfg resolution rules. The kernel tests are useful and pass on CPU/CUDA in CI, but they do not instantiate the registered environments; the current isaaclab_tasks [3/3] CI shard is red on the three Shadow Warp config loads.

Before re-review, please also tighten the source comments to durable functional constraints, remove hot-loop duplication/conversions called out inline, use concrete Warp array annotations on public launchers, and add an isaaclab_tasks changelog fragment for the camera-default change.

[AntoineRichard]

[AI Review] Blocking: shadow_hand_env_cfg.py no longer exists. All three Shadow Warp registrations in this file use this stale module name, and the current isaaclab_tasks [3/3] CI shard fails exactly these three config loads. Point them to shadow_hand_direct_env_cfg, then add a registry/config-load test covering every new ID so a successful kernel-only suite cannot mask broken entry points.

[AntoineRichard]

[AI Review] Blocking: reorient_task_base was removed; these constants now live in reorient_common. The TYPE_CHECKING import below also still names the removed shadow_hand_env_cfg instead of shadow_hand_direct_env_cfg. Once the registration strings are fixed, importing this environment will fail here, so please update both stale references and add an environment-entry import smoke test.

[AntoineRichard]

[AI Review] Blocking: handover_task_base.py does not exist in this head, and GOAL_POSITION_OFFSET has no current definition. This makes the new handover entry point fail on import. Please put the goal definition at a genuine shared boundary (configuration or a small common module used by both torch and Warp implementations) rather than restoring a single-consumer “task base,” and cover the registered entry point with a smoke test.

[AntoineRichard]

[AI Review][Pointed follow-up] The stable implementation currently hard-codes the goal at handover_env.py:73-77. That is the concrete duplication to remove: define one environment-frame GOAL_POSITION in handover_env_cfg.py or a handover_common.py, then make both HandoverEnv and HandoverWarpEnv consume it. Do not recreate the deleted handover_task_base.py solely to satisfy this import.

[AntoineRichard]

[AI Review] Blocking: assigning default on these PresetCfg instances does not change preset resolution. _preset_fields() explicitly prefers the class attribute, so the inherited class-level Newton defaults still win and the camera remains Newton-default. Define camera-specific preset types or declarative preset(...) fields whose class-level default is PhysX, and add a collect_presets(ShadowHandCameraEnvCfg()) assertion for physics, robot, and object defaults. The preceding seven-line rationale should also become one functional sentence, e.g. “Isaac RTX tiled cameras require the PhysX Fabric cloning path”; benchmark CLI history belongs in tests/docs, not this method.

[AntoineRichard]

[AI Review][Pointed follow-up] The exact precedence rule is in _preset_fields(): getattr(type(preset_obj), field_name) wins over the instance field. The existing regression test_collect_fields_prefers_class_attr_over_instance codifies that behavior. Add the new camera-specific assertion beside the shadow_hand_camera_presets fixture, checking that sim.physics.default, robot_cfg.default, and object_cfg.default resolve to their PhysX alternatives.

[AntoineRichard]

[AI Review] The comment immediately above says reward reuses the success flags and orientation distances computed in _get_dones, but this call passes object/goal quaternions and reorient_reward_kernel recomputes _rotation_distance and the tolerance comparison. This is both misleading documentation and duplicate GPU hot-loop work. Pass success_flags and orientation_error_buf into the reward path, matching the torch implementation, and remove the redundant quaternion evaluation.

[AntoineRichard]

[AI Review][Pointed follow-up] The stable torch path shows the intended API: ReorientDirectEnv._get_rewards() passes _success_flags and _orientation_error_buf into reorient_reward. By contrast, the Warp kernel recomputes both at reorient_kernels.py:283-288. Concretely, replace the object/target quaternion plus tolerance inputs with goal_reached: wp.array(dtype=wp.bool) and rotation_distance: wp.array(dtype=wp.float32), then use those values for the reset, bonus, and inverse-error reward.

[AntoineRichard]

[AI Review] The initialization comment claims the hot loop launches kernels without conversions, but wp.from_torch(self.actions[agent]) is created here and again in _hand_proprio_obs for both agents every step. Use persistent per-agent action tensors, copy incoming actions into them in _pre_physics_step, and cache their Warp views once, as the reorientation variant already does.

[AntoineRichard]

[AI Review] The left-hand reset block duplicates the right-hand block above almost statement-for-statement. Please extract a _reset_hand(hand, prev_targets, curr_targets, env_ids) helper (or iterate a per-agent state map). This is exactly the kind of copied lifecycle code that drifts during follow-up fixes and is avoidable within this PR.

[AntoineRichard]

[AI Review] This public launcher uses generic wp.array annotations for every array argument, contrary to the repository requirement to annotate Warp arrays with concrete dimensionality and dtype. Please specify the actual contracts here (wp.bool, wp.float32, wp.vec3f, wp.quatf, and 2D float actions) and on the return type. compute_cube_keypoints() has the same issue.

[AntoineRichard]

[AI Review][Pointed follow-up] The concrete annotation map is: reset_buf/goal_resets → wp.array(dtype=wp.bool), successes/consecutive_successes → wp.array(dtype=wp.float32), positions/origins → wp.array(dtype=wp.vec3f), quaternions → wp.array(dtype=wp.quatf), actions → wp.array2d(dtype=wp.float32), and the return → wp.array(dtype=wp.float32). For compute_cube_keypoints, use wp.array2d(dtype=wp.float32) for poses and wp.array2d(dtype=wp.vec3f) for keypoints. This makes the public launcher contract match the typed kernel signatures instead of discarding their information.

[AntoineRichard]

[AI Review] Please reduce this module docstring to the module’s functional purpose. The long “math root”/step-order narrative is maintenance-heavy, and it claims manager terms consume these kernels even though the current tree only imports them from the experimental environments/tests. Keep execution contracts beside the relevant launcher; remove historical validation and aspirational dependency narration.

[AntoineRichard]

[AI Review] This “math root” and “reverse dependency never exists” prose is architectural narration rather than a functional kernel contract. A concise module description plus precise function-level frame/unit comments is enough. In particular, retain useful invariants such as world-frame versus environment-frame positions, but remove commentary about how the codebase is intended to be organized.

[AntoineRichard]

[AI Review][Comment cleanup] Follow-up pass covering every comment/docstring block that violates the requested standard. Each inline note identifies the exact narration, restatement, stale claim, or hard-coded dimensional prose to remove or rewrite. Comments that encode coordinate frames, zero-copy lifetime, unavailable-sensor behavior, sticky state, or synchronization constraints are intentionally left untouched.

[AntoineRichard]

[AI Review][Comment cleanup] Replace this seven-line narrative with one durable functional constraint: # Isaac RTX tiled cameras require the PhysX Fabric cloning path. The failure history, num_envs scenario, CLI spelling, and benchmark path belong in the regression test or user documentation, not in __post_init__.

[AntoineRichard]

[AI Review][Comment cleanup] Make this module docstring self-contained. “See the reorient variant for the conventions” forces readers to inspect an unrelated module and does not describe a functional contract. A single sentence such as Warp Direct environment for the Shadow handover task. is sufficient.

[AntoineRichard]

[AI Review][Comment cleanup] Remove the labels on lines 49, 63, 70, 78, 83, and 91; each merely repeats the immediately following variable lookup/allocation. Also drop the historical mirrors HandoverCommand.__init__ clause on line 87. Once the goal definition is moved to a shared config/common boundary, its name should carry that relationship; if a comment remains, document only the coordinate frame or invariant.

[AntoineRichard]

[AI Review][Comment cleanup] This comment is factually incorrect: action views are still created with wp.from_torch(...) in _apply_hand_action and _hand_proprio_obs every step. After introducing persistent per-agent action buffers, rewrite this narrowly as # Cache Warp views of persistent Torch buffers. Until then, remove it rather than claiming a property the implementation does not have.

[AntoineRichard]

[AI Review][Comment cleanup] Remove # add hand..., # add ground plane, and # add lights; they only narrate the next constructor/call. Keep the EventManager constraint on line 145, but tighten it to the functional reason: # Register assets with the scene for EventManager randomization.

[AntoineRichard]

[AI Review][Comment cleanup] Keep the launch shape, but remove the speculative warp-uniformity explanation. The branch behavior is an implementation/performance claim without a benchmark or enforced invariant and makes this already-long observation docstring harder to scan.

[AntoineRichard]

[AI Review][Comment cleanup] Keep the launch shape and output layout, but remove the warp-uniformity narrative. It is an implementation/performance claim rather than the kernel’s functional contract and duplicates the same prose in full_obs_kernel.

[AntoineRichard]

[AI Review][Comment cleanup] Remove this line; handover_reward_kernel, goal_distance, and the immediately following exponential expression already state exactly what it says.

[AntoineRichard]

[AI Review][Comment cleanup] Remove this comment. The test inputs (q1, q1.clone()), assertion, and test name make the zero-distance case self-explanatory.

[AntoineRichard]

[AI Review][Comment cleanup] Remove this generic label. The _ref variable names and subsequent assertions already distinguish the Torch reference computation from the Warp result.

[AntoineRichard]

[AI Review][Pointed follow-up] Three additional places where the review should name the exact missing coverage or ownership boundary, rather than leaving the fixer to infer it.

[AntoineRichard]

[AI Review][Pointed follow-up] Part 9 also changes source/isaaclab_tasks/.../shadow_hand_camera_env_cfg.py, but this is the only fragment added by the Part 9 commit range. The inherited source/isaaclab_tasks/changelog.d/task-cleanup-dex-part03.minor.rst belongs to Part 3 and does not mention the camera default; after the stack is rebased, Part 9 will touch isaaclab_tasks with no fragment. Either move commit 386272f2 to its owning stable-task PR or add a separate source/isaaclab_tasks/changelog.d/<slug>.rst Fixed entry for the camera backend default.

[AntoineRichard]

[AI Review][Pointed follow-up] Keep these kernel tests Kit-free, but add a separate registration smoke test because these 44 cases never load an env_cfg_entry_point or import either environment class. The current CI failure proves the gap: parameterize the four new Gym IDs, resolve each config entry point, and import its environment entry point. That test would fail on all five stale references currently present (shadow_hand_env_cfg ×3, reorient_task_base, and handover_task_base) while preserving this file’s focused kernel contract.

[AntoineRichard]

[AI Review][Pointed follow-up] This reaches into EpisodeErrorRecorder._has_sample, a private field owned by isaaclab_tasks.core.utils, and makes the experimental kernel depend on that class’s internal storage layout. EpisodeErrorRecorder.update() is the public boundary and also owns shape/finite-value semantics. Expose an explicit Warp-compatible recorder API/buffer contract, or keep Warp-owned minimum/sample buffers in this environment and pass summarized values to the recorder at reset; do not bind a kernel output directly to another class’s private attribute.