Unverified Commit 5232b449 authored by Mayank Mittal's avatar Mayank Mittal Committed by GitHub

Fixes bug in deeper inheritance in configclass (#94)

# Description

This MR fixes the issue with multiple inheritances in the
`omni.isaac.orbit.utils.configclass` decorator. Earlier, if the
inheritance tree was more than one level deep and the lowest-level
configuration class was not updating its values from the middle-level
classes.

This was happening because in the middle-level classes, the default
values were being updated to dataclass fields and were not visible
anymore in the dictionary of the class itself. The configclass now
checks for their values in the `__dataclass_fields__` of the class as
well.

This was a regression in behavior but has been fixed now with a unit
test to capture deeper inheritance in configclass.

## 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
- [x] 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
parent a666312e
[package] [package]
# Note: Semantic Versioning is used: https://semver.org/ # Note: Semantic Versioning is used: https://semver.org/
version = "0.7.2" version = "0.7.3"
# Description # Description
title = "ORBIT framework for Robot Learning" title = "ORBIT framework for Robot Learning"
......
Changelog Changelog
--------- ---------
0.7.3 (2023-07-25)
~~~~~~~~~~~~~~~~~~
Fixed
^^^^^
* Fixed the issue with multiple inheritance in the :class:`omni.isaac.orbit.utils.configclass` decorator.
Earlier, if the inheritance tree was more than one level deep and the lowest level configuration class was
not updating its values from the middle level classes.
0.7.2 (2023-07-24) 0.7.2 (2023-07-24)
~~~~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~
......
...@@ -260,9 +260,10 @@ def _process_mutable_types(cls): ...@@ -260,9 +260,10 @@ def _process_mutable_types(cls):
class_members[key] = f class_members[key] = f
# iterate over base class data fields # iterate over base class data fields
# in previous call, things that became a dataclass field were removed from class members # in previous call, things that became a dataclass field were removed from class members
# so we need to add them back here as a dataclass field directly
for key, f in base.__dict__.get("__dataclass_fields__", {}).items(): for key, f in base.__dict__.get("__dataclass_fields__", {}).items():
# store class member # store class member
if not isinstance(f, type) and key not in class_members: if not isinstance(f, type):
class_members[key] = f class_members[key] = f
# check that all annotations are present in class members # check that all annotations are present in class members
......
...@@ -8,7 +8,7 @@ import os ...@@ -8,7 +8,7 @@ import os
import unittest import unittest
from dataclasses import MISSING, asdict, field from dataclasses import MISSING, asdict, field
from functools import wraps from functools import wraps
from typing import ClassVar, List, Type from typing import Callable, ClassVar, List, Type
from omni.isaac.orbit.utils.configclass import configclass from omni.isaac.orbit.utils.configclass import configclass
from omni.isaac.orbit.utils.dict import class_to_dict, update_class_from_dict from omni.isaac.orbit.utils.dict import class_to_dict, update_class_from_dict
...@@ -143,14 +143,17 @@ class ParentDemoCfg: ...@@ -143,14 +143,17 @@ class ParentDemoCfg:
b = 2 # type annotation missing on purpose b = 2 # type annotation missing on purpose
c: RobotDefaultStateCfg = MISSING # add new missing field c: RobotDefaultStateCfg = MISSING # add new missing field
j: List[str] = MISSING # add new missing field j: List[str] = MISSING # add new missing field
func: Callable = MISSING # add new missing field
@configclass @configclass
class ChildDemoCfg(ParentDemoCfg): class ChildDemoCfg(ParentDemoCfg):
"""Dummy child configuration with missing fields.""" """Dummy child configuration with missing fields."""
func = dummy_function1 # set default value for missing field
c = RobotDefaultStateCfg() # set default value for missing field c = RobotDefaultStateCfg() # set default value for missing field
func_2: Callable = MISSING # add new missing field
d: int = MISSING # add new missing field d: int = MISSING # add new missing field
k: List[str] = ["c", "d"] k: List[str] = ["c", "d"]
e: ViewerCfg = MISSING # add new missing field e: ViewerCfg = MISSING # add new missing field
...@@ -158,6 +161,14 @@ class ChildDemoCfg(ParentDemoCfg): ...@@ -158,6 +161,14 @@ class ChildDemoCfg(ParentDemoCfg):
dummy_class = DummyClass dummy_class = DummyClass
@configclass
class ChildChildDemoCfg(ChildDemoCfg):
"""Dummy child configuration with missing fields."""
func_2 = dummy_function2
d = 2 # set default value for missing field
""" """
Configuration with class inside. Configuration with class inside.
""" """
...@@ -529,6 +540,19 @@ class TestConfigClass(unittest.TestCase): ...@@ -529,6 +540,19 @@ class TestConfigClass(unittest.TestCase):
# check variables # check variables
cfg = ChildDemoCfg(a=20, d=3, e=ViewerCfg(), j=["c", "d"]) cfg = ChildDemoCfg(a=20, d=3, e=ViewerCfg(), j=["c", "d"])
self.assertEqual(cfg.func, dummy_function1)
self.assertEqual(cfg.a, 20)
self.assertEqual(cfg.b, 2)
self.assertEqual(cfg.d, 3)
self.assertEqual(cfg.j, ["c", "d"])
def test_config_double_inheritance(self):
"""Tests that inheritance works properly when inheriting twice."""
# check variables
cfg = ChildChildDemoCfg(a=20, d=3, e=ViewerCfg(), j=["c", "d"])
self.assertEqual(cfg.func, dummy_function1)
self.assertEqual(cfg.func_2, dummy_function2)
self.assertEqual(cfg.a, 20) self.assertEqual(cfg.a, 20)
self.assertEqual(cfg.b, 2) self.assertEqual(cfg.b, 2)
self.assertEqual(cfg.d, 3) self.assertEqual(cfg.d, 3)
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment