Skip to content

Adds strict type checking and clear split between cfg and implementation#4718

Merged
ooctipus merged 3 commits intoisaac-sim:developfrom
ooctipus:feature/develop/strict_type_checking
Feb 26, 2026
Merged

Adds strict type checking and clear split between cfg and implementation#4718
ooctipus merged 3 commits intoisaac-sim:developfrom
ooctipus:feature/develop/strict_type_checking

Conversation

@ooctipus
Copy link
Collaborator

Description

Adds strict type checking and split between cfg and implementaion

Fixes # (issue)

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Screenshots

Please attach before and after screenshots of the change if applicable.

Checklist

  • I have read and understood the contribution guidelines
  • I have run the pre-commit checks with ./isaaclab.sh --format
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • I have updated the changelog and the corresponding version in the extension's config/extension.toml file
  • I have added my name to the CONTRIBUTORS.md or my name already exists there

@github-actions github-actions bot added enhancement New feature or request asset New asset feature or request isaac-mimic Related to Isaac Mimic team labels Feb 25, 2026
@ooctipus ooctipus force-pushed the feature/develop/strict_type_checking branch 2 times, most recently from 90b17f8 to 245a723 Compare February 25, 2026 11:48
@ooctipus ooctipus marked this pull request as ready for review February 25, 2026 11:49
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Feb 25, 2026

Greptile Summary

This PR separates configuration dataclasses from their implementation counterparts across the IsaacLab codebase, and defers heavy simulation-backend imports (omni, carb, pxr, isaacsim) so that config files can be loaded without a running Kit/simulator. Key changes include:

  • Cfg/implementation split: Extracts *Cfg dataclasses into dedicated *_cfg.py files for controllers, devices, markers, visualization markers, env tasks (ant, cartpole, franka_cabinet, humanoid, quadcopter, shadow_hand, pre_trained_policy_action), and more.
  • Deferred imports: Moves omni.kit.app, omni.physx, carb, pxr, isaacsim imports behind has_kit() guards or into local scope at point of use across env, manager, sensor, and MDP modules.
  • has_kit() rework: Now checks sys.modules instead of attempting import omni.kit.app, avoiding forbidden side-effects during cfg-only loading.
  • String-to-callable auto-resolution: _validate() in configclass.py now resolves "module.path:ClassName" strings into actual callables at validation time, enabling cfg files to reference implementation classes as strings instead of direct imports.
  • New attach_cascading utility: Provides a reusable __getattr__/__dir__ pattern for cascading namespace packages.
  • TYPE_CHECKING guards: MDP modules (observations, rewards, events, curriculums, terminations) now import asset/sensor types only under TYPE_CHECKING.

Issues found:

  • The keyboard device cfg split is incomplete: Se2KeyboardCfg and Se3KeyboardCfg are duplicated (new *_cfg.py files created, but old definitions in se2_keyboard.py/se3_keyboard.py not removed, and keyboard/__init__.py not updated).
  • Several task env cfg extractions are incomplete: CartpoleEnvCfg, CartpoleCamera*EnvCfg, and CartDoublePendulumEnvCfg were copied to new *_cfg.py files but the old definitions in the implementation files were not removed. The basic Isaac-Cartpole-Direct-v0 entry point still references the old module.
  • sensor_base.py adds local imports without removing the existing top-level imports, creating redundancy.

Confidence Score: 3/5

  • This PR is mostly safe but has incomplete migrations that result in duplicate class definitions across several files, which should be resolved before merging.
  • The core architectural changes (deferred imports, has_kit() rework, TYPE_CHECKING guards) are well-implemented and safe. However, the cfg extraction was applied inconsistently: some environments (ant, franka_cabinet, humanoid, quadcopter, shadow_hand) were migrated completely, while others (cartpole, cart_double_pendulum, keyboard devices) have duplicate cfg class definitions across old and new files. The duplicate classes have subtly different type annotations (e.g., type[DeviceBase] vs type | str), which could cause confusion. No runtime breakage is expected from the duplicates since the old code paths still work, but the incomplete state suggests the PR needs cleanup before merge.
  • Pay close attention to se2_keyboard_cfg.py, se3_keyboard_cfg.py (duplicate definitions not cleaned), cartpole/__init__.py (inconsistent entry point), cartpole_env_cfg.py, cartpole_camera_env_cfg.py, cart_double_pendulum_env_cfg.py (duplicate cfgs), and sensor_base.py (redundant imports).

Important Files Changed

Filename Overview
source/isaaclab/isaaclab/utils/configclass.py Adds string-to-callable auto-resolution in _validate(). Silent error suppression may mask typos. Side-effect in validation function is unusual.
source/isaaclab/isaaclab/utils/module.py New utility for cascading namespace packages. Well-structured with caching via dict write-through. Clean implementation.
source/isaaclab/isaaclab/utils/version.py has_kit() now avoids triggering imports via sys.modules check. Good improvement for cfg-only loading without Kit.
source/isaaclab/isaaclab/devices/keyboard/se2_keyboard_cfg.py New cfg file but old definition NOT removed from se2_keyboard.py. Duplicate class with different class_type annotation. keyboard/init.py not updated.
source/isaaclab/isaaclab/devices/keyboard/se3_keyboard_cfg.py Same issue as se2_keyboard_cfg.py - duplicate definition with se3_keyboard.py, init.py not updated.
source/isaaclab/isaaclab/envs/direct_rl_env.py Guards omni.kit.app and omni.physx behind has_kit(). Import order slightly shuffled but functionally correct.
source/isaaclab/isaaclab/envs/mdp/events.py Defers carb, omni.physics, pxr, isaacsim imports to point of use. Moves DeformableObject and TerrainImporter behind TYPE_CHECKING.
source/isaaclab/isaaclab/sensors/sensor_base.py Adds local imports for omni.kit.app and isaaclab_physx but doesn't remove existing top-level imports, creating redundancy.
source/isaaclab_tasks/isaaclab_tasks/direct/ant/ant_env.py Cfg class cleanly extracted to ant_env_cfg.py. Import chain properly updated.
source/isaaclab_tasks/isaaclab_tasks/direct/cartpole/init.py Camera env entry points updated to _cfg module, but basic CartpoleEnvCfg still references cartpole_env (line 23). Inconsistent migration.
source/isaaclab_tasks/isaaclab_tasks/direct/cartpole/cartpole_env_cfg.py New cfg file but CartpoleEnvCfg still duplicated in cartpole_env.py (not removed). Entry point not updated.
source/isaaclab_tasks/isaaclab_tasks/direct/cart_double_pendulum/cart_double_pendulum_env_cfg.py New cfg file but CartDoublePendulumEnvCfg still duplicated in cart_double_pendulum_env.py (not removed).
source/isaaclab_tasks/isaaclab_tasks/direct/franka_cabinet/franka_cabinet_env.py Cfg class cleanly extracted to franka_cabinet_env_cfg.py. Imports properly updated.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    subgraph "Before: Cfg + Impl coupled"
        A[env_cfg.py / env.py<br/>Combined file] -->|"import omni, carb, pxr"| B[Kit/Simulator Required]
        A -->|"contains Cfg + Env class"| C[Cannot load cfg<br/>without simulator]
    end

    subgraph "After: Cfg / Impl split"
        D[env_cfg.py<br/>Pure config dataclass] -->|"no simulator imports"| E[Loadable without Kit]
        F[env.py<br/>Implementation only] -->|"import omni, carb, pxr"| G[Kit required at runtime]
        F -->|"imports cfg from"| D
        H["configclass._validate()"] -->|"resolves 'mod:Class' strings"| I[string_to_callable]
        J["has_kit()"] -->|"sys.modules check"| K{"Kit running?"}
        K -->|"Yes"| L["import omni.kit.app"]
        K -->|"No"| M["Skip import"]
    end

    style C fill:#f99,stroke:#c00
    style E fill:#9f9,stroke:#0c0
Loading

Last reviewed commit: 5f0641c

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

75 files reviewed, 5 comments

Edit Code Review Agent Settings | Greptile

Comment on lines 296 to 298
if isinstance(value, str) and _CALLABLE_STR_RE.match(value):
with contextlib.suppress(ImportError, AttributeError, ValueError):
obj_dict[key] = string_to_callable(value)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Silent suppression of resolution errors in validation

The contextlib.suppress(ImportError, AttributeError, ValueError) silently swallows errors when a string matching the "module.path:ClassName" pattern fails to resolve. If a user has a typo in a callable string (e.g., "isaaclab.controllers.rmp_flw:RmpFlowController"), it will be silently left as a string. The actual failure will then surface much later with a cryptic error when the string is used where a callable is expected.

Consider at minimum logging a warning when resolution fails, so users can debug configuration typos more easily:

if isinstance(value, str) and _CALLABLE_STR_RE.match(value):
    try:
        obj_dict[key] = string_to_callable(value)
    except (ImportError, AttributeError, ValueError):
        import logging
        logging.getLogger(__name__).debug(
            "Could not resolve callable string %r for field %r", value, key
        )

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like a good idea!

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Feb 25, 2026

Additional Comments (1)

source/isaaclab_tasks/isaaclab_tasks/direct/cartpole/__init__.py
Inconsistent entry point migration

The camera-based cartpole environments (lines 36, 47, 58, 69, 80, 91) were updated to point to cartpole_camera_env_cfg, but the basic Isaac-Cartpole-Direct-v0 entry point at this line still references cartpole_env:CartpoleEnvCfg. The CartpoleEnvCfg definition remains in cartpole_env.py and was also added to the new cartpole_env_cfg.py, creating a duplicate.

This should be updated to:

        "env_cfg_entry_point": f"{__name__}.cartpole_env_cfg:CartpoleEnvCfg",

And the CartpoleEnvCfg class should be removed from cartpole_env.py.

Comment on lines 296 to 298
if isinstance(value, str) and _CALLABLE_STR_RE.match(value):
with contextlib.suppress(ImportError, AttributeError, ValueError):
obj_dict[key] = string_to_callable(value)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like a good idea!

@ooctipus ooctipus force-pushed the feature/develop/strict_type_checking branch 3 times, most recently from 48c0eaf to ae5b58e Compare February 25, 2026 20:57
@github-actions github-actions bot added the documentation Improvements or additions to documentation label Feb 25, 2026
@ooctipus ooctipus force-pushed the feature/develop/strict_type_checking branch from be1ecd0 to 11b05ff Compare February 26, 2026 02:13
@ooctipus ooctipus merged commit 97062f7 into isaac-sim:develop Feb 26, 2026
5 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

asset New asset feature or request documentation Improvements or additions to documentation enhancement New feature or request isaac-mimic Related to Isaac Mimic team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants