Merge pull request #215 from databio/vr/logfile-212

Check that logfile for manager's logger doesn't collide with manager's own logfile

pypiper/const.py

@@ -1,6 +1,5 @@
 """ Pypiper constants. """
 
-
 CHECKPOINT_EXTENSION = ".checkpoint"
 DEFAULT_SAMPLE_NAME = "DEFAULT_SAMPLE_NAME"
 PIPELINE_CHECKPOINT_DELIMITER = "_"

pypiper/manager.py

@@ -58,6 +58,7 @@
 
 
 LOCK_PREFIX = "lock."
+LOGFILE_SUFFIX = "_log.md"
 
 
 class Unbuffered(object):
@@ -198,10 +199,7 @@ def __init__(
         # If no starting point was specified, assume that the pipeline's
         # execution is to begin right away and set the internal flag so that
         # run() is let loose to execute instructions given.
-        if not self.start_point:
-            self._active = True
-        else:
-            self._active = False
+        self._active = not self.start_point
 
         # Pipeline-level variables to track global state and pipeline stats
         # Pipeline settings
@@ -215,26 +213,37 @@ def __init__(
         self.output_parent = params["output_parent"]
         self.testmode = params["testmode"]
 
+        # Establish the log file to check safety with logging keyword arguments.
+        # Establish the output folder since it's required for the log file.
+        self.outfolder = os.path.join(outfolder, "")  # trailing slash
+        self.pipeline_log_file = pipeline_filepath(self, suffix=LOGFILE_SUFFIX)
+
         # Set up logger
         logger_kwargs = logger_kwargs or {}
+        if logger_kwargs.get("logfile") == self.pipeline_log_file:
+            raise ValueError(
+                f"The logfile given for the pipeline manager's logger matches that which will be used by the manager itself: {self.pipeline_log_file}"
+            )
         default_logname = ".".join([__name__, self.__class__.__name__, self.name])
-        if not args:
+        self._logger = None
+        if args:
+            logger_builder_method = "logger_via_cli"
+            try:
+                self._logger = logger_via_cli(args, **logger_kwargs)
+            except logmuse.est.AbsentOptionException as e:
+                # Defer logger construction to init_logger.
+                self.debug(f"logger_via_cli failed: {e}")
+        if self._logger is None:
+            logger_builder_method = "init_logger"
+            # covers cases of bool(args) being False, or failure of logger_via_cli.
             # strict is only for logger_via_cli.
-            kwds = {k: v for k, v in logger_kwargs.items() if k != "strict"}
+            logger_kwargs = {k: v for k, v in logger_kwargs.items() if k != "strict"}
             try:
-                name = kwds.pop("name")
+                name = logger_kwargs.pop("name")
             except KeyError:
                 name = default_logname
-            self._logger = logmuse.init_logger(name, **kwds)
-            self.debug("Logger set with logmuse.init_logger")
-        else:
-            logger_kwargs.setdefault("name", default_logname)
-            try:
-                self._logger = logmuse.logger_via_cli(args)
-                self.debug("Logger set with logmuse.logger_via_cli")
-            except logmuse.est.AbsentOptionException:
-                self._logger = logmuse.init_logger("pypiper", level="DEBUG")
-                self.debug("logger_via_cli failed; Logger set with logmuse.init_logger")
+            self._logger = logmuse.init_logger(name, **logger_kwargs)
+        self.debug(f"Logger set with {logger_builder_method}")
 
         # Keep track of an ID for the number of processes attempted
         self.proc_count = 0
@@ -281,10 +290,7 @@ def __init__(
         #   self.output_parent = os.path.join(os.getcwd(), self.output_parent)
 
         # File paths:
-        self.outfolder = os.path.join(outfolder, "")  # trailing slash
         self.make_sure_path_exists(self.outfolder)
-        self.pipeline_log_file = pipeline_filepath(self, suffix="_log.md")
-
         self.pipeline_profile_file = pipeline_filepath(self, suffix="_profile.tsv")
 
         # Stats and figures are general and so lack the pipeline name.

pypiper/ngstk.py

@@ -153,8 +153,7 @@ def get_file_size(self, filenames):
             return sum([self.get_file_size(filename) for filename in filenames])
 
         return round(
-            sum([float(os.stat(f).st_size) for f in filenames.split(" ")])
-            / (1024**2),
+            sum([float(os.stat(f).st_size) for f in filenames.split(" ")]) / (1024**2),
             4,
         )
 

pypiper/utils.py

@@ -785,12 +785,10 @@ def pipeline_filepath(pm, filename=None, suffix=None):
         filename as given or determined by the pipeline name, and suffix
         appended if given.
     """
-
     if filename is None and suffix is None:
         raise TypeError(
-            "Provide filename and/or suffix to create " "path to a pipeline file."
+            "Provide filename and/or suffix to create path to a pipeline file."
         )
-
     filename = (filename or pm.name) + (suffix or "")
 
     # Note that Pipeline and PipelineManager define the same outfolder.

tests/pipeline_manager/test_manager_constructor.py

@@ -1,10 +1,11 @@
 """ Test effects of construction of a pipeline manager. """
 
 import argparse
+import os
 
 import pytest
 
-from pypiper.manager import CHECKPOINT_SPECIFICATIONS
+from pypiper.manager import CHECKPOINT_SPECIFICATIONS, LOGFILE_SUFFIX
 from tests.helpers import named_param
 
 __author__ = "Vince Reuter"
@@ -24,6 +25,22 @@ def test_manager_starts_in_null_checkpoint_state(get_pipe_manager, checkpoint_ty
     assert getattr(pm, checkpoint_type) is None
 
 
+def test_logger_logfile_collision_with_manager_logfile_is_expected_error__issue_212(
+    get_pipe_manager, tmpdir
+):
+    pipe_name = "test_issue212"
+    with pytest.raises(ValueError) as err_ctx:
+        get_pipe_manager(
+            name=pipe_name,
+            logger_kwargs={
+                "logfile": os.path.join(tmpdir.strpath, pipe_name + LOGFILE_SUFFIX)
+            },
+        )
+    assert str(err_ctx.value).startswith(
+        f"The logfile given for the pipeline manager's logger matches that which will be used by the manager itself"
+    )
+
+
 class ManagerConstructorCheckpointSpecificationTests:
     """Tests for manager's constructor's ability to parse and set
     checkpoint specifications, which can determine aspects of control flow."""