Skip to content

Conversation

@bmccann-bdai
Copy link
Collaborator

Description

This PR breaks the actuator config into separate files in order to avoid a circular import which arises once the configuration is imported into the actuator base class for more than type hinting.

Type of change

  • Possible breaking change! The actuator configs have moved .py files and thus modules. They can still be imported directly from the isaaclab.actuators package. External references to these classes through the actuator_cfg module will not longer work. Instead, import directly from the isaaclab.actuators package.

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 the asset New asset feature or request label Nov 10, 2025
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Nov 10, 2025

Greptile Overview

Greptile Summary

This PR successfully refactors the actuator configuration modules to resolve circular import issues by splitting the monolithic actuator_cfg.py into three focused modules: actuator_base_cfg.py, actuator_pd_cfg.py, and actuator_net_cfg.py.

Key changes:

  • Created actuator_base_cfg.py with base configuration class
  • Created actuator_pd_cfg.py with PD-based actuator configs
  • Created actuator_net_cfg.py with neural network actuator configs
  • Updated all TYPE_CHECKING imports across actuator modules to reference new locations
  • Updated 8 external files to import from isaaclab.actuators package instead of actuator_cfg module
  • Simplified RemotizedPDActuator.__init__ to use *args, **kwargs

Critical issue found:
The old actuator_cfg.py file still exists with all original class definitions, creating duplicate definitions and potentially reintroducing the circular import issue. This file must be deleted.

Confidence Score: 2/5

  • This PR cannot be safely merged due to incomplete refactoring - the old actuator_cfg.py file was not deleted
  • The refactoring approach is sound and all imports have been properly updated. However, the old actuator_cfg.py file still exists alongside the new split modules, creating duplicate class definitions and potentially reintroducing the circular import issue this PR aims to fix. Once that file is deleted, this would be a clean refactoring with no other issues.
  • source/isaaclab/isaaclab/actuators/actuator_cfg.py must be deleted to complete the refactoring

Important Files Changed

File Analysis

Filename Score Overview
source/isaaclab/isaaclab/actuators/actuator_base_cfg.py 5/5 New file created with ActuatorBaseCfg extracted from old actuator_cfg.py. Clean implementation with no issues.
source/isaaclab/isaaclab/actuators/actuator_pd_cfg.py 5/5 New file with PD actuator configs extracted from old actuator_cfg.py. Properly imports from new modules.
source/isaaclab/isaaclab/actuators/actuator_net_cfg.py 5/5 New file with neural network actuator configs. Correctly inherits from DCMotorCfg in actuator_pd_cfg.
source/isaaclab/isaaclab/actuators/init.py 5/5 Updated imports to use new separate config modules instead of monolithic actuator_cfg.
source/isaaclab/isaaclab/actuators/actuator_base.py 5/5 Updated TYPE_CHECKING import to reference actuator_base_cfg instead of actuator_cfg.
source/isaaclab/isaaclab/actuators/actuator_pd.py 5/5 Updated TYPE_CHECKING imports and simplified RemotizedPDActuator.__init__ to use *args, **kwargs.
source/isaaclab/isaaclab/actuators/actuator_net.py 5/5 Updated TYPE_CHECKING import to reference actuator_net_cfg instead of actuator_cfg.

Sequence Diagram

sequenceDiagram
    participant User as External Code
    participant Init as actuators/__init__.py
    participant BaseCfg as actuator_base_cfg.py
    participant PDCfg as actuator_pd_cfg.py
    participant NetCfg as actuator_net_cfg.py
    participant PD as actuator_pd.py
    participant Net as actuator_net.py
    participant Base as actuator_base.py

    Note over User,Base: Before: Circular Import Issue
    Note over User: Old: from actuator_cfg import *
    Note over User: (actuator_cfg imported from actuator_pd/net)

    Note over User,Base: After: Clean Module Structure
    User->>Init: from isaaclab.actuators import ImplicitActuatorCfg
    Init->>PDCfg: import from actuator_pd_cfg
    PDCfg->>PD: import actuator_pd module
    PDCfg->>BaseCfg: import ActuatorBaseCfg
    Note over PDCfg: No circular dependency!
    
    User->>Init: from isaaclab.actuators import ActuatorNetLSTMCfg
    Init->>NetCfg: import from actuator_net_cfg
    NetCfg->>Net: import actuator_net module
    NetCfg->>PDCfg: import DCMotorCfg
    Note over NetCfg: Clean dependency chain!
    
    Note over Base,PD: TYPE_CHECKING imports prevent runtime circular imports
    Base-->>BaseCfg: TYPE_CHECKING: import ActuatorBaseCfg
    PD-->>PDCfg: TYPE_CHECKING: import configs
    Net-->>NetCfg: TYPE_CHECKING: import configs
Loading

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.

18 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@bmccann-bdai bmccann-bdai force-pushed the bcm/circular-import-remediation branch from 738c478 to 0dfbf47 Compare November 10, 2025 21:07
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.

Additional Comments (1)

  1. source/isaaclab/isaaclab/actuators/actuator_cfg.py, line 1-291 (link)

    logic: Old actuator_cfg.py file still exists but should be deleted

    This file was supposed to be deleted as part of the refactoring to split configs into separate modules. Having both the old monolithic file and the new split files creates:

    • Duplicate class definitions (all configs exist in two places)
    • Potential for import confusion
    • The circular import issue this PR aims to fix could persist through this file

    Delete this file since all classes are now properly exported through:

    • actuator_base_cfg.py
    • actuator_pd_cfg.py
    • actuator_net_cfg.py
    • __init__.py

18 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant