Inline crds log interface instead and check for previous console log

Author: melanieclarke

src/stpipe/crds_client.py

@@ -2,10 +2,6 @@
 This module defines functions that connect the core crds package
 to stpipe, tailoring it to provide results in the forms required
 by stpipe.
-
-WARNING:  stpipe and crds have circular dependencies.  Do not use crds imports
-directly in modules other than this crds_client so that dependency order and
-general integration can be managed here.
 """
 
 import re
@@ -26,16 +22,6 @@
 ]
 
 
-def remove_crds_log_handler():
-    """Remove the default stream handler for the CRDS logger."""
-    log.remove_console_handler()
-
-
-def restore_crds_log_handler():
-    """Restore the default stream handler for the CRDS logger."""
-    log.add_console_handler()
-
-
 def get_multiple_reference_paths(parameters, reference_file_types, observatory):
     """Aligns stpipe requirements with CRDS library top level interfaces.
 

src/stpipe/log.py

@@ -12,8 +12,9 @@
 
 from astropy.extern.configobj import validate
 from astropy.extern.configobj.configobj import ConfigObj
+from crds.core import log as crds_log
 
-from . import config_parser, crds_client
+from . import config_parser
 
 STPIPE_ROOT_LOGGER = "stpipe"
 DEFAULT_FORMAT = "%(asctime)s - %(name)s - %(levelname)s - %(message)s"
@@ -102,6 +103,7 @@ def __init__(
             self.handlers.append(BreakHandler(self.break_level))
 
         self._previous_level = {}
+        self._previous_crds_console = False
 
     def get_handler(self, handler_str):
         """
@@ -136,7 +138,7 @@ def apply(self, log_names=None):
         - "py.warnings": If provided, Python warnings are captured and
           logged (``logging.captureWarnings(True)``).
         - "CRDS": If provided, the default stream handler for the CRDS
-          logger is removed.
+          logger is removed if needed.
 
         Parameters
         ----------
@@ -149,7 +151,9 @@ def apply(self, log_names=None):
         if "py.warnings" in log_names:
             logging.captureWarnings(True)
         if "CRDS" in log_names:
-            crds_client.remove_crds_log_handler()
+            self._previous_crds_console = crds_log.THE_LOGGER.console is not None
+            if self._previous_crds_console:
+                crds_log.remove_console_handler()
         for log_name in log_names:
             # Don't reapply configuration, to avoid overwriting the
             # previously recorded level.
@@ -173,8 +177,9 @@ def undo(self, log_names=None, close_handlers=True):
 
         - "py.warnings": If provided, Python warnings are no longer captured
           and logged (``logging.captureWarnings(False)``).
-        - "CRDS": If provided, the default stream handler for the CRDS
-          logger is restored.
+        - "CRDS": If provided and if the default stream handler for the CRDS
+          logger was present when configuration was applied, it will be
+          restored.
 
         Parameters
         ----------
@@ -207,8 +212,9 @@ def undo(self, log_names=None, close_handlers=True):
             LogConfig.applied = None
         if "py.warnings" in log_names:
             logging.captureWarnings(False)
-        if "CRDS" in log_names:
-            crds_client.restore_crds_log_handler()
+        if "CRDS" in log_names and self._previous_crds_console:
+            self._previous_crds_console = False
+            crds_log.add_console_handler()
 
     @contextmanager
     def context(self, log_names=None):

tests/test_logger.py

@@ -3,11 +3,11 @@
 import warnings
 
 import pytest
+from crds.core import log as crds_log
 
 import stpipe.cmdline
 from stpipe import Step
 from stpipe import log as stpipe_log
-from stpipe.crds_client import log as crds_log
 from stpipe.pipeline import Pipeline
 
 
@@ -592,7 +592,8 @@ def get_stpipe_loggers():
     assert logging._warnings_showwarning is None
 
 
-def test_crds_log(capsys):
+@pytest.mark.parametrize("restore_console", [True, False])
+def test_crds_log(capsys, restore_console):
     class CRDSLoggingStep(LoggingStep):
         def process(self):
             logger.info(STEP_INFO)
@@ -606,12 +607,19 @@ def process(self):
         def get_stpipe_loggers():
             return ("stpipe", "CRDS")
 
-    # Before the step call, the crds logger has a stream handler
     crds_logger = logging.getLogger("CRDS")
-    assert len(crds_logger.handlers) == 1
-    assert isinstance(crds_logger.handlers[0], logging.StreamHandler)
+    if restore_console:
+        # Before the step call, the crds logger has a stream handler
+        assert len(crds_logger.handlers) == 1
+        assert isinstance(crds_logger.handlers[0], logging.StreamHandler)
+    else:
+        # remove any current console handler
+        crds_log.remove_console_handler()
+
+        # Before the step call, the crds logger has no handlers
+        assert len(crds_logger.handlers) == 0
 
-    # During the step call, the CRDS handler is removed and
+    # During the step call, the CRDS handler is removed if necessary and
     # the stpipe handler is attached
     CRDSLoggingStep.call()
 
@@ -627,6 +635,11 @@ def get_stpipe_loggers():
             assert capt.err.count(msg) == 0
 
     # After the call is complete, the stpipe logger is removed and the CRDS
-    # stream logger is restored
-    assert len(crds_logger.handlers) == 1
-    assert isinstance(crds_logger.handlers[0], logging.StreamHandler)
+    # stream logger is restored if it was previously present
+    if restore_console:
+        assert len(crds_logger.handlers) == 1
+        assert isinstance(crds_logger.handlers[0], logging.StreamHandler)
+    else:
+        assert len(crds_logger.handlers) == 0
+        # Clean up: restore a console logger
+        crds_log.add_console_handler()