Repository: cta-observatory/ctapipe (python)

Issue:
Component logging hierarchy setup is broken
**Describe the bug**
Setting up the hierarchy of loggers is currently broken (and at least since 0.12). 

The code in `Component` checks `self.parent` before calling `super().__init__()` so `self.parent` is always None and the hierarchy is not setup correctly.

**To Reproduce**
```
from ctapipe.core import Tool, Component


class Foo(Component):
    def __init__(self, **kwargs):
        super().__init__(**kwargs)
        print(self.log)


class Bar(Component):
    def __init__(self, **kwargs):
        super().__init__(**kwargs)
        print(self.log)
        self.foo = Foo(parent=self)

class Baz(Tool):
    name = 'baz'

    def setup(self):
        self.bar = Bar(parent=self)

    def start(self):
        print(self.log)


if __name__ == '__main__':
    Baz().run()
```

prints

```
<Logger __main__.Bar (WARNING)>
<Logger __main__.Foo (WARNING)>
<Logger ctapipe.baz (WARNING)>
```


**Expected behavior**
Hierarchy is setup correctly.



Pull request:
 Fix setting up logging hierarchy, fixes #1882 

Patch:
diff --git a/ctapipe/core/component.py b/ctapipe/core/component.py
index b4fa32fc..c82cfbb1 100644
--- a/ctapipe/core/component.py
+++ b/ctapipe/core/component.py
@@ -58,7 +58,7 @@ def non_abstract_children(base):
     -------
     non_abstract : dict
         dict of all non-abstract subclasses
-     """
+    """
     subclasses = base.__subclasses__() + [
         g for s in base.__subclasses__() for g in non_abstract_children(s)
     ]
@@ -143,8 +143,8 @@ class Component(Configurable, metaclass=AbstractConfigurableMeta):
 
         # set up logging (for some reason the logger registered by LoggingConfig
         # doesn't use a child logger of the parent by default)
-        if self.parent:
-            self.log = self.parent.log.getChild(self.__class__.__name__)
+        if parent is not None:
+            self.log = parent.log.getChild(self.__class__.__name__)
         else:
             self.log = getLogger(
                 self.__class__.__module__ + "." + self.__class__.__name__
@@ -203,7 +203,7 @@ class Component(Configurable, metaclass=AbstractConfigurableMeta):
         return subclasses
 
     def get_current_config(self):
-        """ return the current configuration as a dict (e.g. the values
+        """return the current configuration as a dict (e.g. the values
         of all traits, even if they were not set during configuration)
         """
         name = self.__class__.__name__
@@ -216,7 +216,7 @@ class Component(Configurable, metaclass=AbstractConfigurableMeta):
         return config
 
     def _repr_html_(self):
-        """ nice HTML rep, with blue for non-default values"""
+        """nice HTML rep, with blue for non-default values"""
         traits = self.traits()
         name = self.__class__.__name__
         lines = [
diff --git a/ctapipe/core/tool.py b/ctapipe/core/tool.py
index 71c8bbd0..d934da4c 100644
--- a/ctapipe/core/tool.py
+++ b/ctapipe/core/tool.py
@@ -192,7 +192,10 @@ class Tool(Application):
         self.version = version
         self.raise_config_file_errors = True  # override traitlets.Application default
 
-        self.log = logging.getLogger("ctapipe." + self.name)
+        # tools defined in other modules should have those modules as base
+        # logging name
+        module_name = self.__class__.__module__.split(".")[0]
+        self.log = logging.getLogger(f"{module_name}.{self.name}")
         self.trait_warning_handler = CollectTraitWarningsHandler()
         self.update_logging_config()
 


Test patch:
diff --git a/ctapipe/core/tests/test_component.py b/ctapipe/core/tests/test_component.py
index f871d893..2df00d5a 100644
--- a/ctapipe/core/tests/test_component.py
+++ b/ctapipe/core/tests/test_component.py
@@ -6,11 +6,11 @@ from traitlets.config.loader import Config
 import astropy.units as u
 import warnings
 
-from ctapipe.core import Component
+from ctapipe.core import Component, Tool
 
 
 def test_non_abstract_children():
-    """ check that we can find all constructable children """
+    """check that we can find all constructable children"""
     from ctapipe.core import non_abstract_children
 
     class AbstractBase(ABC):
@@ -72,13 +72,13 @@ def test_get_config_from_hierarchy():
 
 
 class SubComponent(Component):
-    """ An Example Component, this is the help text"""
+    """An Example Component, this is the help text"""
 
     value = Float(default_value=-1.0, help="float parameter").tag(config=True)
 
 
 class ExampleComponent(Component):
-    """ An Example Component, this is the help text"""
+    """An Example Component, this is the help text"""
 
     param = Float(default_value=1.0, help="float parameter").tag(config=True)
 
@@ -88,13 +88,13 @@ class ExampleComponent(Component):
 
 
 class ExampleSubclass1(ExampleComponent):
-    """ a subclass of ExampleComponent"""
+    """a subclass of ExampleComponent"""
 
     pass
 
 
 class ExampleSubclass2(ExampleComponent):
-    """ Another ExampleComponent """
+    """Another ExampleComponent"""
 
     description = "A shorter description"
     param = Float(default_value=3.0, help="float parameter").tag(config=True)
@@ -102,7 +102,7 @@ class ExampleSubclass2(ExampleComponent):
 
 
 def test_component_is_abstract():
-    """ check that we can make an abstract component """
+    """check that we can make an abstract component"""
 
     class AbstractComponent(Component):
         @abstractmethod
@@ -128,7 +128,7 @@ def test_component_simple():
 
 
 def test_component_kwarg_setting():
-    """ check that we can construct a component by setting traits via kwargs """
+    """check that we can construct a component by setting traits via kwargs"""
     comp = ExampleComponent(param=3)
     assert comp.param == 3
 
@@ -147,13 +147,13 @@ def test_component_kwarg_setting():
 
 
 def test_help():
-    """ check that component help strings are generated correctly """
+    """check that component help strings are generated correctly"""
     help_msg = ExampleComponent.class_get_help()
     assert "Default: 1.0" in help_msg
 
 
 def test_config():
-    """ check that components can be constructed by config dict """
+    """check that components can be constructed by config dict"""
     config = Config()
     config["ExampleComponent"] = Config()
     config["ExampleComponent"]["param"] = 199.0
@@ -162,7 +162,7 @@ def test_config():
 
 
 def test_config_baseclass():
-    """ check that parent and subclass configuration works """
+    """check that parent and subclass configuration works"""
     config = Config()
     config["ExampleComponent"] = Config()
     config["ExampleComponent"]["param"] = 199.0
@@ -191,7 +191,7 @@ def test_config_subclass2():
 
 
 def test_config_sibling1():
-    """ check sibling config """
+    """check sibling config"""
     config = Config()
     config["ExampleSubclass1"] = Config()
     config["ExampleSubclass1"]["param"] = 199.0
@@ -202,7 +202,7 @@ def test_config_sibling1():
 
 
 def test_config_sibling2():
-    """ check sibling config """
+    """check sibling config"""
     config = Config()
     config["ExampleSubclass2"] = Config()
     config["ExampleSubclass2"]["param"] = 199.0
@@ -213,7 +213,7 @@ def test_config_sibling2():
 
 
 def test_config_baseclass_then_subclass():
-    """ check base and subclass config """
+    """check base and subclass config"""
     config = Config()
     config["ExampleComponent"] = Config()
     config["ExampleComponent"]["param"] = 199.0
@@ -224,7 +224,7 @@ def test_config_baseclass_then_subclass():
 
 
 def test_config_subclass_then_baseclass():
-    """ check subclass and base config """
+    """check subclass and base config"""
     config = Config()
     config["ExampleSubclass1"] = Config()
     config["ExampleSubclass1"]["param"] = 229.0
@@ -235,7 +235,7 @@ def test_config_subclass_then_baseclass():
 
 
 def test_config_override():
-    """ check that we can override a trait set in the config """
+    """check that we can override a trait set in the config"""
     config = Config()
     config["ExampleComponent"] = Config()
     config["ExampleComponent"]["param"] = 199.0
@@ -244,7 +244,7 @@ def test_config_override():
 
 
 def test_config_override_subclass():
-    """ check that we can override a trait set in the config """
+    """check that we can override a trait set in the config"""
     config = Config()
     config["ExampleComponent"] = Config()
     config["ExampleComponent"]["param"] = 199.0
@@ -253,14 +253,14 @@ def test_config_override_subclass():
 
 
 def test_extra():
-    """ check that traits are settable """
+    """check that traits are settable"""
     comp = ExampleSubclass2(extra=229.0)
     assert comp.has_trait("extra") is True
     assert comp.extra == 229.0
 
 
 def test_extra_config():
-    """ check setting trait via config """
+    """check setting trait via config"""
     config = Config()
     config["ExampleSubclass2"] = Config()
     config["ExampleSubclass2"]["extra"] = 229.0
@@ -269,7 +269,7 @@ def test_extra_config():
 
 
 def test_unknown_traitlet_raises():
-    """ check that setting an incorrect trait raises an exception """
+    """check that setting an incorrect trait raises an exception"""
     with pytest.raises(TraitError):
         # the current traitlets version already warns about this
         # will be raising an error in the future, but we want the error
@@ -294,25 +294,25 @@ def test_extra_config_missing():
 
 
 def test_default():
-    """ check default values work"""
+    """check default values work"""
     comp = ExampleComponent()
     assert comp.param == 1.0
 
 
 def test_default_subclass():
-    """ check default values work in subclasses"""
+    """check default values work in subclasses"""
     comp = ExampleSubclass1()
     assert comp.param == 1.0
 
 
 def test_default_subclass_override():
-    """ check overrides work in subclasses"""
+    """check overrides work in subclasses"""
     comp = ExampleSubclass2()
     assert comp.param == 3.0
 
 
 def test_change_default():
-    """ check we can change a default value"""
+    """check we can change a default value"""
     old_default = ExampleComponent.param.default_value
     ExampleComponent.param.default_value = 199.0
     comp = ExampleComponent()
@@ -321,7 +321,7 @@ def test_change_default():
 
 
 def test_change_default_subclass():
-    """ check we can change a default value in subclass """
+    """check we can change a default value in subclass"""
     old_default = ExampleComponent.param.default_value
     ExampleComponent.param.default_value = 199.0
     comp = ExampleSubclass1()
@@ -330,7 +330,7 @@ def test_change_default_subclass():
 
 
 def test_change_default_subclass_override():
-    """ check override default value  """
+    """check override default value"""
     old_default = ExampleComponent.param.default_value
     ExampleComponent.param.default_value = 199.0
     comp = ExampleSubclass2()
@@ -339,7 +339,7 @@ def test_change_default_subclass_override():
 
 
 def test_help_changed_default():
-    """ check that the help text is updated if the default is changed """
+    """check that the help text is updated if the default is changed"""
     old_default = ExampleComponent.param.default_value
     ExampleComponent.param.default_value = 199.0
     help_msg = ExampleComponent.class_get_help()
@@ -359,7 +359,7 @@ def test_non_abstract_subclasses():
 
 
 def test_from_name():
-    """ Make sure one can construct a Component subclass by name"""
+    """Make sure one can construct a Component subclass by name"""
     subclass = ExampleComponent.from_name("ExampleSubclass1")
     assert isinstance(subclass, ExampleSubclass1)
     subclass = ExampleComponent.from_name("ExampleSubclass2")
@@ -367,14 +367,14 @@ def test_from_name():
 
 
 def test_from_name_config():
-    """ make sure one can construct a Component subclass by name + config"""
+    """make sure one can construct a Component subclass by name + config"""
     config = Config({"ExampleComponent": {"param": 229.0}})
     subclass = ExampleComponent.from_name("ExampleSubclass1", config=config)
     assert subclass.param == 229.0
 
 
 def test_component_current_config():
-    """ make sure one can get the full current configuration"""
+    """make sure one can get the full current configuration"""
     comp = ExampleComponent()
     full_config = comp.get_current_config()
     assert "ExampleComponent" in full_config
@@ -383,7 +383,7 @@ def test_component_current_config():
 
 
 def test_component_html_repr():
-    """ check the HTML repr for Jupyter notebooks """
+    """check the HTML repr for Jupyter notebooks"""
     comp = ExampleComponent()
     html = comp._repr_html_()
     assert len(html) > 10
@@ -434,3 +434,30 @@ def test_full_config():
     comp_from_config = MyComponent(config=config)
 
     assert dict_config == comp_from_config.get_current_config()
+
+
+def test_logging_hierarchy():
+    class Foo(Component):
+        def __init__(self, **kwargs):
+            super().__init__(**kwargs)
+
+    class Bar(Component):
+        def __init__(self, **kwargs):
+            super().__init__(**kwargs)
+            self.foo = Foo(parent=self)
+
+    class Baz(Tool):
+        name = "baz"
+
+        def setup(self):
+            self.bar = Bar(parent=self)
+
+        def start(self):
+            pass
+
+    baz = Baz()
+    baz.setup()
+
+    assert baz.log.name == "ctapipe.baz"
+    assert baz.bar.log.name == "ctapipe.baz.Bar"
+    assert baz.bar.foo.log.name == "ctapipe.baz.Bar.Foo"