Split tasks into core/contrib#5891
Conversation
There was a problem hiding this comment.
Review: Split tasks into core/contrib
Overall Assessment: Well-structured refactoring that introduces a clean core/contrib separation. The architectural direction is sound — flat task families under core/ and contrib/ with a clear one-way dependency rule (contrib → core, never the reverse). The Gym environment IDs are preserved, making this backward-compatible at the gym.make() level while only breaking direct Python imports.
🔍 Findings
1. No Backward-Compatibility Shim for Removed Import Paths
Severity: Medium (Design Decision)
Files: source/isaaclab_tasks/isaaclab_tasks/__init__.py
The old isaaclab_tasks.direct.* and isaaclab_tasks.manager_based.* module paths are completely removed with no deprecation warning or re-export shim. Any downstream user code that does from isaaclab_tasks.manager_based.manipulation.lift... will immediately break with ModuleNotFoundError.
Context: This is documented as a .major.rst breaking change and the changelog provides migration examples. The Gym IDs are preserved, so gym.make("Isaac-...") continues to work.
Suggestion: Consider whether a thin compatibility layer (e.g., a isaaclab_tasks/manager_based/__init__.py that raises DeprecationWarning with migration instructions) would ease the transition for users who pin to specific Isaac Lab versions. This is a trade-off between clean breaks and migration ergonomics — purely a product decision.
2. Core Test File Imports from Contrib (test_hydra.py)
Severity: Low
File: source/isaaclab_tasks/test/core/test_hydra.py
The test_hydra.py file lives under test/core/ but imports UnitreeGo1RoughEnvCfg from isaaclab_tasks.contrib.velocity.config.go1.rough_env_cfg. While test code is not subject to the "core never imports contrib" rule (that applies to task families), this creates a coupling where changes to contrib could break core tests.
Recommendation: If this test is specifically testing Hydra preset resolution (not Go1 itself), consider using a core env config that also has physics presets, or document that certain core tests have contrib dependencies.
3. Determinism Test Coverage Reduced for Direct Workflow
Severity: Low
File: source/isaaclab_tasks/test/core/test_environment_determinism.py
The determinism test for locomotion was narrowed from three environments (Isaac-Velocity-Flat-Anymal-C-v0, Isaac-Velocity-Rough-Anymal-C-v0, Isaac-Velocity-Rough-Anymal-C-Direct-v0) down to two (Isaac-Velocity-Flat-Anymal-D-v0, Isaac-Velocity-Rough-Anymal-D-v0). This removes determinism coverage for the Direct workflow variant entirely.
Recommendation: Consider adding a core direct-workflow task (e.g., Isaac-Cartpole-Direct-v0 or Isaac-Ant-Direct-v0) to maintain coverage of both workflow types.
4. _task_tier() Returns None for Non-String Entry Points
Severity: Low
File: source/isaaclab_tasks/test/env_test_utils.py
The new _task_tier() function determines tier by checking if the env_cfg_entry_point string starts with isaaclab_tasks.core or isaaclab_tasks.contrib. If a task registers a callable (not a string) or uses an entry point from a different package, this returns None — which means tier="core" or tier="contrib" filters will silently exclude that task from testing.
Impact: Currently all IsaacLab tasks use string entry points, so this is not an active issue. But if future tasks use callable entry points, they would silently disappear from filtered test runs.
5. Build Wheel CI Failure (Infrastructure)
Severity: Info
CI Status: ❌ Build Wheel failing at "Detect wheel-relevant changes" step
The wheel build fails during change detection (not during the actual build). This appears to be a CI infrastructure issue related to processing the large file list (731 changed files), not a code issue in this PR.
✅ What Looks Good
- Gym ID preservation: Environment IDs are unchanged —
gym.make("Isaac-...")is unaffected. - Dependency direction enforced: Contrib envs correctly import from core (e.g.,
contrib/lift/config/franka/ik_abs_env_cfg.pyinherits fromcore.lift.config.franka.joint_pos_env_cfg), never the reverse. - Test structure mirrors source:
test/core/andtest/contrib/with atierparameter insetup_environment()is clean. - Changelog is thorough: Breaking change is clearly documented with before/after import examples.
- Cross-package updates complete:
isaaclab_mimic,isaaclab_rl,isaaclabtests,isaaclab_tasks_experimental, scripts, and documentation all updated consistently. - Flat directory structure: Eliminating the nested
manager_based/manipulation/lift/→core/lift/reduces path length significantly. - Env removal cleanup: Quadcopter, Teddy Bear, and NoVelObs variant removals are properly propagated through tests, benchmarks, and scripts.
conftest.pyaddition: Properly handles test-suite path resolution for the new directory structure.- pre-commit and broken-links CI passing: Code style and documentation links are valid.
Update (commit 45185d6): The new commit fixes the CI wheel build failure noted in finding #5 above. The fix replaces printf ... | grep pipelines with here-string-based grep ... <<< "$files" to avoid SIGPIPE under set -o pipefail when processing large file lists. This is a correct and clean fix — no new issues introduced. Findings #1–#4 remain as-is (design decisions / low-severity observations, not blocking).
Update (commit 3b16c79): ✅ Finding #2 (core test importing from contrib) is now fixed — test_hydra.py imports UnitreeGo2RoughEnvCfg from isaaclab_tasks.core.velocity.config.go2 instead of the previous contrib path. No new issues introduced in this commit.
Update (commit 424f2ca): Test path fix — _REPO_ROOT in test_newton_solver_preset_names.py and test_train_scripts_deterministic.py updated from parents[3] to parents[4] to account for the additional directory nesting under test/core/. Correct and necessary fix. No new issues introduced. Findings #1, #3, #4 remain as low-severity observations (not blocking).
The develop merge brought in isaac-sim#5891 (split tasks tests into core/contrib), which relocated every test file under test/core/ or test/contrib/ and left only shared helpers + conftest at the test-suite root. The new test_cartpole_training_smoke.py predated that split and was the lone test_*.py still at top-level. Cartpole is a core task, so move it under test/core/ to match the new layout, and bump its _REPO_ROOT path depth from parents[3] to parents[4] (matching the sibling core/ tests).
Description
Splits isaaclab_tasks into core and contrib. Directory structure is flat, limiting excessive nested intermediate folders.
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there