Skip to content

Commit

Permalink
Changes all attribute names for classes in configclass to class_type (
Browse files Browse the repository at this point in the history
#161)

# Description

Currently, the code has a mix of `cls_name` and `cls` in the
configuration classes. Since `cls` is a reserved key from Python, we
should avoid using this name for attributes. This MR changes all the
occurrences to become `class_type` instead.

Fixes #141 

## Type of change

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

## Checklist

- [x] I have run the [`pre-commit` checks](https://pre-commit.com/) with
`./orbit.sh --format`
- [x] I have made corresponding changes to the documentation
- [x] My changes generate no new warnings
- [ ] I have added tests that prove my fix is effective or that my
feature works
- [x] I have updated the changelog and the corresponding version in the
extension's `config/extension.toml` file

---------

Co-authored-by: Mayank Mittal <mittalma@leggedrobotics.com>
  • Loading branch information
hhansen-bdai and Mayankm96 authored Sep 26, 2023
1 parent 17fc724 commit d332be8
Show file tree
Hide file tree
Showing 24 changed files with 113 additions and 54 deletions.
23 changes: 23 additions & 0 deletions docs/source/refs/contributing.rst
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,29 @@ For documentation, we adopt the `Google Style Guide <https://sphinxcontrib-napol
for docstrings. We use `Sphinx <https://www.sphinx-doc.org/en/master/>`__ for generating the documentation.
Please make sure that your code is well-documented and follows the guidelines.

Circular Imports
^^^^^^^^^^^^^^^^

Circular imports happen when two modules import each other, which is a common issue in Python.
You can prevent circular imports by adhering to the best practices outlined in this
`StackOverflow post <https://stackoverflow.com/questions/744373/circular-or-cyclic-imports-in-python>`__.

In general, it is essential to avoid circular imports as they can lead to unpredictable behavior.

However, in our codebase, we encounter circular imports at a sub-package level. This situation arises
due to our specific code structure. We organize classes or functions and their corresponding configuration
objects into separate files. This separation enhances code readability and maintainability. Nevertheless,
it can result in circular imports because, in many configuration objects, we specify classes or functions
as default values using the attributes ``class_type`` and ``func`` respectively.

To address circular imports, we leverage the `typing.TYPE_CHECKING
<https://docs.python.org/3/library/typing.html#typing.TYPE_CHECKING>`_ variable. This special variable is
evaluated only during type-checking, allowing us to import classes or functions in the configuration objects
without triggering circular imports.

It is important to note that this is the sole instance within our codebase where circular imports are used
and are acceptable. In all other scenarios, we adhere to best practices and recommend that you do the same.

Type-hinting
^^^^^^^^^^^^

Expand Down
2 changes: 1 addition & 1 deletion source/extensions/omni.isaac.orbit/config/extension.toml
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
[package]

# Note: Semantic Versioning is used: https://semver.org/
version = "0.9.5"
version = "0.9.6"

# Description
title = "ORBIT framework for Robot Learning"
Expand Down
10 changes: 10 additions & 0 deletions source/extensions/omni.isaac.orbit/docs/CHANGELOG.rst
Original file line number Diff line number Diff line change
@@ -1,6 +1,16 @@
Changelog
---------

0.9.6 (2023-09-26)
~~~~~~~~~~~~~~~~~~

Fixed
^^^^^

* Changed class-level configuration classes to refer to class types using ``class_type`` attribute instead
of ``cls`` or ``cls_name``.


0.9.5 (2023-09-25)
~~~~~~~~~~~~~~~~~~

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,11 @@
class ActuatorBaseCfg:
"""Configuration for default actuators in an articulation."""

cls: type[ActuatorBase] = MISSING
"""Actuator class."""
class_type: type[ActuatorBase] = MISSING
"""The associated actuator class.
The class should inherit from :class:`omni.isaac.orbit.actuators.actuator_base.ActuatorBase`.
"""

joint_names_expr: list[str] = MISSING
"""Articulation's joint names that are part of the group.
Expand Down Expand Up @@ -66,7 +69,7 @@ class ImplicitActuatorCfg(ActuatorBaseCfg):
The PD control is handled implicitly by the simulation.
"""

cls = actuator_pd.ImplicitActuator
class_type: type = actuator_pd.ImplicitActuator


"""
Expand All @@ -78,14 +81,14 @@ class ImplicitActuatorCfg(ActuatorBaseCfg):
class IdealPDActuatorCfg(ActuatorBaseCfg):
"""Configuration for an ideal PD actuator."""

cls = actuator_pd.IdealPDActuator
class_type: type = actuator_pd.IdealPDActuator


@configclass
class DCMotorCfg(IdealPDActuatorCfg):
"""Configuration for direct control (DC) motor actuator model."""

cls = actuator_pd.DCMotor
class_type: type = actuator_pd.DCMotor

saturation_effort: float = MISSING
"""Peak motor force/torque of the electric DC motor (in N-m)."""
Expand All @@ -95,7 +98,7 @@ class DCMotorCfg(IdealPDActuatorCfg):
class ActuatorNetLSTMCfg(DCMotorCfg):
"""Configuration for LSTM-based actuator model."""

cls = actuator_net.ActuatorNetLSTM
class_type: type = actuator_net.ActuatorNetLSTM
# we don't use stiffness and damping for actuator net
stiffness = None
damping = None
Expand All @@ -108,7 +111,7 @@ class ActuatorNetLSTMCfg(DCMotorCfg):
class ActuatorNetMLPCfg(DCMotorCfg):
"""Configuration for MLP-based actuator model."""

cls = actuator_net.ActuatorNetMLP
class_type: type = actuator_net.ActuatorNetMLP
# we don't use stiffness and damping for actuator net
stiffness = None
damping = None
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -492,8 +492,7 @@ def _process_actuators_cfg(self):
if len(joint_names) == self.num_joints:
joint_ids = ...
# create actuator collection
actuator_cls = actuator_cfg.cls
actuator: ActuatorBase = actuator_cls(
actuator: ActuatorBase = actuator_cfg.class_type(
cfg=actuator_cfg,
joint_names=joint_names,
joint_ids=joint_ids,
Expand All @@ -502,7 +501,7 @@ def _process_actuators_cfg(self):
)
# log information on actuator groups
carb.log_info(
f"Actuator collection: {actuator_name} with model '{actuator_cls.__name__}' and "
f"Actuator collection: {actuator_name} with model '{actuator_cfg.class_type.__name__}' and "
f"joint names: {joint_names} [{joint_ids}]."
)
# store actuator group
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
class ArticulationCfg(RigidObjectCfg):
"""Configuration parameters for an articulation."""

cls_name = Articulation
class_type: type = Articulation

@configclass
class InitialStateCfg(RigidObjectCfg.InitialStateCfg):
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,14 @@

import re
from abc import ABC, abstractmethod
from typing import Any, Sequence
from typing import TYPE_CHECKING, Any, Sequence

import omni.isaac.core.utils.prims as prim_utils
import omni.kit.app
import omni.physx

from .asset_base_cfg import AssetBaseCfg
if TYPE_CHECKING:
from .asset_base_cfg import AssetBaseCfg


class AssetBase(ABC):
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,13 @@
from __future__ import annotations

from dataclasses import MISSING
from typing import ClassVar
from typing_extensions import Literal

from omni.isaac.orbit.sim import SpawnerCfg
from omni.isaac.orbit.utils import configclass

from .asset_base import AssetBase


@configclass
class AssetBaseCfg:
Expand All @@ -29,8 +30,11 @@ class InitialStateCfg:
Defaults to (1.0, 0.0, 0.0, 0.0).
"""

cls_name: ClassVar[str] = MISSING
"""Class name of the asset."""
class_type: type[AssetBase] = MISSING
"""The associated asset class.
The class should inherit from :class:`omni.isaac.orbit.assets.asset_base.AssetBase`.
"""

prim_path: str = MISSING
"""Prim path (or expression) to the asset.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
class RigidObjectCfg(AssetBaseCfg):
"""Configuration parameters for a rigid object."""

cls_name = RigidObject
class_type: type = RigidObject

@configclass
class InitialStateCfg(AssetBaseCfg.InitialStateCfg):
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,12 @@
class CommandGeneratorBaseCfg:
"""Configuration for the base command generator."""

class_name: type[CommandGeneratorBase] = MISSING
"""The command generator class to use."""
class_type: type[CommandGeneratorBase] = MISSING
"""The associated command generator class to use.
The class should inherit from :class:`omni.isaac.orbit.command_generators.command_generator_base.CommandGeneratorBase`.
"""

resampling_time_range: tuple[float, float] = MISSING
"""Time before commands are changed [s]."""
debug_vis: bool = False
Expand All @@ -39,7 +43,7 @@ class CommandGeneratorBaseCfg:
class UniformVelocityCommandGeneratorCfg(CommandGeneratorBaseCfg):
"""Configuration for the uniform velocity command generator."""

class_name = UniformVelocityCommandGenerator
class_type: type = UniformVelocityCommandGenerator

asset_name: str = MISSING
"""Name of the asset in the environment for which the commands are generated."""
Expand Down Expand Up @@ -73,7 +77,7 @@ class Ranges:
class NormalVelocityCommandGeneratorCfg(UniformVelocityCommandGeneratorCfg):
"""Configuration for the normal velocity command generator."""

class_name = NormalVelocityCommandGenerator
class_type: type = NormalVelocityCommandGenerator
heading_command: bool = False # --> we don't use heading command for normal velocity command.

@configclass
Expand Down Expand Up @@ -104,7 +108,7 @@ class Ranges:
class TerrainBasedPositionCommandGeneratorCfg(CommandGeneratorBaseCfg):
"""Configuration for the terrain-based position command generator."""

class_name = TerrainBasedPositionCommandGenerator
class_type: type = TerrainBasedPositionCommandGenerator

asset_name: str = MISSING
"""Name of the asset in the environment for which the commands are generated."""
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ def __init__(self, cfg: ActuatorGroupCfg, view: ArticulationView):
# process configuration
# -- create actuator model
if self.model_type == "explicit":
actuator_model_cls = eval(self.cfg.model_cfg.cls_name)
actuator_model_cls = eval(self.cfg.model_cfg.class_type)
self.model = actuator_model_cls(
cfg=self.cfg.model_cfg,
num_actuators=self.num_actuators,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
class ActuatorGroupCfg:
"""Configuration for default group of actuators in an articulation."""

cls_name: ClassVar[str] = "ActuatorGroup"
class_type: ClassVar[str] = "ActuatorGroup"
"""Name of the associated actuator group class. Used to construct the actuator group."""

dof_names: list[str] = MISSING
Expand All @@ -46,7 +46,7 @@ class ActuatorGroupCfg:
class GripperActuatorGroupCfg(ActuatorGroupCfg):
"""Configuration for mimicking actuators in a gripper."""

cls_name: ClassVar[str] = "GripperActuatorGroup"
class_type: ClassVar[str] = "GripperActuatorGroup"

speed: float = MISSING
"""The speed at which gripper should close. (used with velocity command type.)"""
Expand Down Expand Up @@ -74,4 +74,4 @@ class GripperActuatorGroupCfg(ActuatorGroupCfg):
class NonHolonomicKinematicsGroupCfg(ActuatorGroupCfg):
"""Configuration for formulating non-holonomic base constraint."""

cls_name: ClassVar[str] = "NonHolonomicKinematicsGroup"
class_type: ClassVar[str] = "NonHolonomicKinematicsGroup"
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ def load_managers(self):
# prepare the managers
# note: this order is important since observation manager needs to know the command and action managers
# -- command manager
self.command_manager: CommandGeneratorBase = self.cfg.commands.class_name(self.cfg.commands, self)
self.command_manager: CommandGeneratorBase = self.cfg.commands.class_type(self.cfg.commands, self)
print("[INFO] Command Manager: ", self.command_manager)
# -- action manager
self.action_manager = ActionManager(self.cfg.actions, self)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ class JointPositionActionCfg(JointActionCfg):
See :class:`JointPositionAction` for more details.
"""

cls: type[ActionTerm] = joint_actions.JointPositionAction
class_type: type[ActionTerm] = joint_actions.JointPositionAction

use_default_offset: bool = True
"""Whether to use default joint positions configured in the articulation asset as offset.
Expand All @@ -56,7 +56,7 @@ class JointVelocityActionCfg(JointActionCfg):
See :class:`JointVelocityAction` for more details.
"""

cls: type[ActionTerm] = joint_actions.JointVelocityAction
class_type: type[ActionTerm] = joint_actions.JointVelocityAction

use_default_offset: bool = True
"""Whether to use default joint velocities configured in the articulation asset as offset.
Expand All @@ -73,7 +73,7 @@ class JointEffortActionCfg(JointActionCfg):
See :class:`JointEffortAction` for more details.
"""

cls: type[ActionTerm] = joint_actions.JointEffortAction
class_type: type[ActionTerm] = joint_actions.JointEffortAction


##
Expand Down Expand Up @@ -103,7 +103,7 @@ class BinaryJointPositionActionCfg(BinaryJointActionCfg):
See :class:`BinaryJointPositionAction` for more details.
"""

cls: type[ActionTerm] = binary_joint_actions.BinaryJointPositionAction
class_type: type[ActionTerm] = binary_joint_actions.BinaryJointPositionAction


@configclass
Expand All @@ -113,7 +113,7 @@ class BinaryJointVelocityActionCfg(BinaryJointActionCfg):
See :class:`BinaryJointVelocityAction` for more details.
"""

cls: type[ActionTerm] = binary_joint_actions.BinaryJointVelocityAction
class_type: type[ActionTerm] = binary_joint_actions.BinaryJointVelocityAction


##
Expand All @@ -128,7 +128,7 @@ class NonHolonomicActionCfg(ActionTermCfg):
See :class:`NonHolonomicAction` for more details.
"""

cls: type[ActionTerm] = non_holonomic_actions.NonHolonomicAction
class_type: type[ActionTerm] = non_holonomic_actions.NonHolonomicAction

body_name: str = MISSING
"""Name of the body which has the dummy mechanism connected to."""
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -257,7 +257,7 @@ def _prepare_terms(self):
f"Configuration for the term '{term_name}' is not of type ActionTermCfg. Received '{type(term_cfg)}'."
)
# create the action term
term = term_cfg.cls(term_cfg, self._env)
term = term_cfg.class_type(term_cfg, self._env)
# sanity check if term is valid type
if not isinstance(term, ActionTerm):
raise TypeError(f"Returned object for the term '{term_name}' is not of type ActionType.")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -103,11 +103,18 @@ class ManagerBaseTermCfg:
class ActionTermCfg:
"""Configuration for an action term."""

cls: type[ActionTerm] = MISSING
"""Class of the action term."""
class_type: type[ActionTerm] = MISSING
"""The associated action term class.
The class should inherit from :class:`omni.isaac.orbit.managers.action_manager.ActionTerm`.
"""

asset_name: str = MISSING
"""Name of the asset (object or robot) on which action is applied."""
"""The name of the scene entity.
This is the name defined in the scene configuration file. See the :class:`InteractiveSceneCfg`
class for more details.
"""


##
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -336,13 +336,13 @@ def _add_entities_from_cfg(self):
# terrains are special entities since they define environment origins
asset_cfg.num_envs = self.cfg.num_envs
asset_cfg.env_spacing = self.cfg.env_spacing
self.terrain = asset_cfg.cls_name(asset_cfg)
self.terrain = asset_cfg.class_type(asset_cfg)
elif isinstance(asset_cfg, ArticulationCfg):
self.articulations[asset_name] = asset_cfg.cls_name(asset_cfg)
self.articulations[asset_name] = asset_cfg.class_type(asset_cfg)
elif isinstance(asset_cfg, RigidObjectCfg):
self.rigid_objects[asset_name] = asset_cfg.cls_name(asset_cfg)
self.rigid_objects[asset_name] = asset_cfg.class_type(asset_cfg)
elif isinstance(asset_cfg, SensorBaseCfg):
self.sensors[asset_name] = asset_cfg.cls_name(asset_cfg)
self.sensors[asset_name] = asset_cfg.class_type(asset_cfg)
elif isinstance(asset_cfg, AssetBaseCfg):
# manually spawn asset
if asset_cfg.spawn is not None:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ class OffsetCfg:
"""

cls_name = Camera
class_type: type = Camera

offset: OffsetCfg = OffsetCfg()
"""The offset pose of the sensor's frame from the sensor's parent frame. Defaults to identity.
Expand Down
Loading

0 comments on commit d332be8

Please sign in to comment.