Remove isaaclab_tasks dependency from core test files - #6922
Remove isaaclab_tasks dependency from core test files#6922mataylor-nvidia wants to merge 3 commits into
Conversation
Rewrite four tests in source/isaaclab/test/ that unnecessarily imported isaaclab_tasks to load registered tasks via parse_env_cfg and gym.make: - test_outdated_sensor.py: replace Franka stack task with an inline ManagerBasedRLEnvCfg using FRANKA_PANDA_CFG and FrameTransformerCfg; rename test function to reflect actual behaviour (eef_pos staleness). - test_action_state_recorder_term_task_integration.py: replace Franka lift task with an inline CartpoleEnvCfg; ActionStateRecorderManagerCfg is already a core import and scene.get_state() always returns both articulation and rigid_object keys so compare_states works unchanged. - test_manager_based_rl_env_obs_spaces_task_integration.py: replace CartpoleEnvCfg, CartpoleCameraEnvCfg, AnymalCRoughEnvCfg, and resolve_presets with inline cfgs backed by CARTPOLE_CFG; the clip-to- obs-space-bounds test uses a mixed clipped/unclipped joint obs cfg rather than camera or ray-caster envs, since the tested logic is identical for all obs term types. - test_tiled_camera_env.py: move isaaclab_tasks imports inside the _launch_tests helper (all tests are @pytest.mark.skip, so the helper never executes at module load time). Move test_pink_ik.py and its test_ik_configs/ data directory to source/isaaclab_tasks/test/controllers/ because the test is inseparable from the GR1T2/G1 robot-specific Pink IK action term internals; the Path(__file__).parent reference is unchanged.
Greptile SummaryThe PR removes
Confidence Score: 5/5The PR appears safe to merge with no concrete changed-code defect identified. The inline environments satisfy the manager and sensor configuration contracts exercised by these tests, recorder callbacks remain available under direct construction, and the relocated or deferred task dependencies remain on valid paths. Important Files Changed
Reviews (1): Last reviewed commit: "Remove isaaclab_tasks dependency from co..." | Re-trigger Greptile |
There was a problem hiding this comment.
Isaac Lab Review Bot
The test dependency refactor appropriately removes task-package imports from core test modules and relocates the task-specific Pink IK coverage, but the rewritten FrameTransformer regression test no longer changes the robot pose during reset, making its stale-data assertion ineffective.
- Design and architecture: The dependency direction is improved: core tests use private inline configurations backed by
isaaclabandisaaclab_assets, while robot-specific Pink IK coverage moves toisaaclab_tasks. The stale-sensor test configuration needs adjustment so its reset actually exercises cache invalidation. - API: No public symbols, exports, CLI arguments, or configuration contracts are changed. The added environment configuration classes are private to their test modules, and both touched packages include
.skipchangelog fragments. - Implementation: The FrameTransformer test resets every joint with zero position and velocity offsets, leaving the robot at its existing default pose; the subsequent idle action also preserves that pose. Consequently, stale and freshly updated end-effector observations are numerically indistinguishable. The reset must move the robot to a demonstrably different pose before comparing reset-time and first-step observations.
Minor fixes needed. Posted 1 actionable finding inline.
Automated review; human maintainers own approval decisions.
| mode="reset", | ||
| params={ | ||
| "asset_cfg": SceneEntityCfg("robot"), | ||
| "position_range": (0.0, 0.0), |
There was a problem hiding this comment.
🟡 Warning · Implementation — Zero-range reset makes staleness assertion vacuous
reset_robot uses position_range/velocity_range of (0.0, 0.0), so the reset writes exactly the spawn/default joint state the FrameTransformer cache was already populated with, and idle JointPositionAction holds that pose. A stale post-reset sensor read is then numerically identical to a fresh one, so assert_close(pre_reset_eef_pos, post_reset_eef_pos) passes even if reset-time cache invalidation regresses. Use a non-zero position range so the post-reset pose differs.
Note on
|
…tests Signed-off-by: Matthew Taylor <mataylor@nvidia.com>
|
|
||
|
|
||
| @configclass | ||
| class _SceneCfg(InteractiveSceneCfg): |
There was a problem hiding this comment.
does the env cfg used in both tests matter? maybe it can be put in a test util file so it can be reused. also helps avoid the long import list at the file top
| @@ -1,142 +0,0 @@ | |||
| # Copyright (c) 2022-2026, The Isaac Lab Project Developers (https://github.com/isaac-sim/IsaacLab/blob/main/CONTRIBUTORS.md). | |||
There was a problem hiding this comment.
Is the reason fro the move parse_env_cfg? wondering if the -tasks dependency can be removed instead of move.
Summary
ManagerBasedRLEnvCfgusingFRANKA_PANDA_CFG+FrameTransformerCfg(both core); droppedgym.make,parse_env_cfg, andtask_nameparametrize. Renamed test function totest_eef_pos_not_stale_after_resetto reflect actual behaviour.CartpoleEnvCfg;ActionStateRecorderManagerCfgis already a core import so no task dependency remains.CartpoleEnvCfg,CartpoleCameraEnvCfg,AnymalCRoughEnvCfg, andresolve_presetswith inline cfgs backed byCARTPOLE_CFG. The clip-to-obs-space-bounds test now uses a mixed clipped/unclipped joint obs cfg — the tested logic is obs-type-agnostic so no camera or ray-caster env is needed.isaaclab_tasksimports inside_launch_tests(all test functions are@pytest.mark.skipso the helper is never called at module load time).test_ik_configs/: Moved tosource/isaaclab_tasks/test/controllers/— the test is inseparable from GR1T2/G1 robot-specific Pink IK action term internals.Test plan
uv run isaaclab -fpasses (pre-commit clean)test_outdated_sensor.pyexercised in isaacsim CI via@pytest.mark.isaacsim_citest_action_state_recorder_term_task_integration.pyexercised in integration CItest_manager_based_rl_env_obs_spaces_task_integration.pyexercised in integration CItest_pink_ik.pydiscovered by CI basename lookup (CI usesTEST_INCLUDE_FILESby filename, not path)