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 structure by splitting the monolithic actuator_cfg.py file into three separate modules (actuator_base_cfg.py, actuator_pd_cfg.py, actuator_net_cfg.py), resolving a circular import that was blocking future actuator improvements. The changes maintain backward compatibility through a deprecation shim using __getattr__ in the old actuator_cfg.py file.

Key Changes

  • Extracted ActuatorBaseCfg into actuator_base_cfg.py
  • Extracted PD-based configs (Implicit, IdealPD, DCMotor, Delayed, Remotized) into actuator_pd_cfg.py
  • Extracted neural network configs (LSTM, MLP) into actuator_net_cfg.py
  • Updated actuator_base.py TYPE_CHECKING import to use actuator_base_cfg instead of actuator_cfg
  • Updated all dependent files across isaaclab_tasks and isaaclab_assets to use package-level imports
  • Version bumped to 0.48.4 with appropriate changelog entry

Issues Found

The deprecation shim in actuator_cfg.py has problematic logic on lines 30-33 that contains unreachable/incorrect code (already flagged in previous comments). This doesn't affect functionality since the primary code path (lines 14-29) works correctly, but should be cleaned up.

Confidence Score: 4/5

  • Safe to merge with minor cleanup recommended for the deprecation shim logic
  • The refactoring successfully resolves the circular import issue and maintains backward compatibility. All imports have been properly updated across dependent files. The main concern is the flawed logic in lines 30-33 of actuator_cfg.py which appears to be unreachable/incorrect dead code that should be removed for code clarity, though it doesn't affect functionality since the main deprecation path (lines 14-29) works correctly.
  • source/isaaclab/isaaclab/actuators/actuator_cfg.py needs cleanup of lines 30-33 (unreachable logic), though this is non-critical

Important Files Changed

File Analysis

Filename Score Overview
source/isaaclab/isaaclab/actuators/actuator_cfg.py 3/5 Replaced all config classes with deprecation shim using __getattr__ to redirect imports. Has logic issues in lines 30-33 that are unreachable/problematic.
source/isaaclab/isaaclab/actuators/actuator_base_cfg.py 5/5 New file created by extracting ActuatorBaseCfg from actuator_cfg.py. Clean separation with no issues found.
source/isaaclab/isaaclab/actuators/actuator_pd_cfg.py 5/5 New file with PD-based actuator configs extracted from actuator_cfg.py. Properly imports actuator_pd and ActuatorBaseCfg.
source/isaaclab/isaaclab/actuators/actuator_net_cfg.py 5/5 New file with neural network actuator configs extracted from actuator_cfg.py. Properly imports dependencies.
source/isaaclab/isaaclab/actuators/init.py 5/5 Updated to export all config classes from new separate modules. Maintains backward compatibility through package-level imports.
source/isaaclab/isaaclab/actuators/actuator_base.py 5/5 Updated TYPE_CHECKING import from actuator_cfg to actuator_base_cfg. This resolves the circular import issue.

Sequence Diagram

sequenceDiagram
    participant User as User Code
    participant OldCfg as actuator_cfg.py<br/>(deprecated)
    participant NewBase as actuator_base_cfg.py
    participant NewPD as actuator_pd_cfg.py
    participant NewNet as actuator_net_cfg.py
    participant Init as __init__.py
    participant Base as actuator_base.py

    Note over User,Base: New Import Path (Recommended)
    User->>Init: from isaaclab.actuators import ImplicitActuatorCfg
    Init->>NewPD: import from actuator_pd_cfg
    NewPD->>Base: import actuator_pd module
    Init-->>User: Return config class

    Note over User,Base: Legacy Import Path (Deprecated)
    User->>OldCfg: from actuator_cfg import ImplicitActuatorCfg
    OldCfg->>OldCfg: __getattr__("ImplicitActuatorCfg")
    OldCfg->>NewPD: Check if name in dir(actuator_pd_cfg)
    OldCfg->>User: warnings.warn(DeprecationWarning)
    OldCfg->>NewPD: getattr(actuator_pd_cfg, name)
    NewPD-->>OldCfg: Return config class
    OldCfg-->>User: Return config class

    Note over User,Base: Circular Import Resolution
    Base->>NewBase: TYPE_CHECKING import ActuatorBaseCfg
    Note over Base: No circular dependency:<br/>actuator_base.py → actuator_base_cfg.py<br/>(previously: actuator_base.py → actuator_cfg.py → actuator_base.py)
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

Copy link
Contributor

@Mayankm96 Mayankm96 left a comment

Choose a reason for hiding this comment

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

Could you also make sure the api docs are building correctly?

https://github.com/isaac-sim/IsaacLab/blob/main/docs/source/api/lab/isaaclab.actuators.rst

@Mayankm96 Mayankm96 changed the title Actuator Config Circular Import Remediation Breaks actuator config file into separate files to avoid circular imports Nov 12, 2025
@bmccann-bdai bmccann-bdai force-pushed the bcm/circular-import-remediation branch 2 times, most recently from 14f5347 to d6004fd Compare November 12, 2025 14:50
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
Copy link
Collaborator Author

Could you also make sure the api docs are building correctly?

https://github.com/isaac-sim/IsaacLab/blob/main/docs/source/api/lab/isaaclab.actuators.rst

I may not be clear on what the ask is here. The build docs check is completing successfully. I believe the deploy step is skipped until we merge. I don't know if, for example, autoclass directives are correctly displayed in the git rest api preview.

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

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.

19 files reviewed, 2 comments

Edit Code Review Agent Settings | Greptile

@bmccann-bdai bmccann-bdai force-pushed the bcm/circular-import-remediation branch from 815a22d to 23e95ab Compare November 12, 2025 21:37
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.

19 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@bmccann-bdai bmccann-bdai force-pushed the bcm/circular-import-remediation branch from 57155d0 to 1c5ab00 Compare November 13, 2025 14:21
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.

19 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@bmccann-bdai bmccann-bdai force-pushed the bcm/circular-import-remediation branch from 1c5ab00 to c32db68 Compare November 13, 2025 14:27
@jtigue-bdai jtigue-bdai reopened this Nov 13, 2025
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.

19 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@bmccann-bdai bmccann-bdai force-pushed the bcm/circular-import-remediation branch from c6af236 to 5014ea8 Compare November 13, 2025 15:03
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.

19 files reviewed, 2 comments

Edit Code Review Agent Settings | Greptile

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.

19 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

@bmccann-bdai bmccann-bdai force-pushed the bcm/circular-import-remediation branch from f17f16f to a332a31 Compare November 13, 2025 16:23
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.

20 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

@bmccann-bdai bmccann-bdai force-pushed the bcm/circular-import-remediation branch from a332a31 to dbc2f2d Compare November 13, 2025 16:30
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.

20 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@bmccann-bdai bmccann-bdai force-pushed the bcm/circular-import-remediation branch from dbc2f2d to 44deabe Compare November 13, 2025 16:39
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.

20 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

Copy link
Collaborator

@ooctipus ooctipus left a comment

Choose a reason for hiding this comment

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

Nice refactor! LGTM

Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Signed-off-by: Brian McCann <[email protected]>
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.

20 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

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.

20 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

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.

20 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

@kellyguo11 kellyguo11 merged commit 7e2aba2 into isaac-sim:main Nov 14, 2025
4 of 5 checks passed
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.

20 files reviewed, no comments

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.

5 participants