From bddeddcb220dc0ab8515945b6e514dd6ccd77d0a Mon Sep 17 00:00:00 2001 From: Vince Reuter Date: Sun, 18 Feb 2024 14:01:41 +0100 Subject: [PATCH 01/11] simplify syntax --- pypiper/manager.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/pypiper/manager.py b/pypiper/manager.py index 67441d6..7e9dc2d 100644 --- a/pypiper/manager.py +++ b/pypiper/manager.py @@ -193,10 +193,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 From 005869e2a2896b99fa573d726d0c2594fdd04141 Mon Sep 17 00:00:00 2001 From: Vince Reuter Date: Sun, 18 Feb 2024 14:03:44 +0100 Subject: [PATCH 02/11] separate what can and can't cause exception --- pypiper/manager.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pypiper/manager.py b/pypiper/manager.py index 7e9dc2d..7a73f0c 100644 --- a/pypiper/manager.py +++ b/pypiper/manager.py @@ -223,10 +223,11 @@ def __init__( 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") + else: + self.debug("Logger set with logmuse.logger_via_cli") # Keep track of an ID for the number of processes attempted self.proc_count = 0 From d8504f6bf972c1d38e603f535af84e69edef8b9b Mon Sep 17 00:00:00 2001 From: Vince Reuter Date: Sun, 18 Feb 2024 14:12:15 +0100 Subject: [PATCH 03/11] copy the kwargs to avoid modification --- pypiper/manager.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pypiper/manager.py b/pypiper/manager.py index 7a73f0c..00c6653 100644 --- a/pypiper/manager.py +++ b/pypiper/manager.py @@ -8,6 +8,7 @@ """ import atexit +import copy import datetime import errno import glob @@ -208,7 +209,7 @@ def __init__( self.testmode = params["testmode"] # Set up logger - logger_kwargs = logger_kwargs or {} + logger_kwargs = copy.deepcopy(logger_kwargs) or {} default_logname = ".".join([__name__, self.__class__.__name__, self.name]) if not args: # strict is only for logger_via_cli. From 28a608aa36fb34ddab13e3f5eeeda04652b18579 Mon Sep 17 00:00:00 2001 From: Vince Reuter Date: Sun, 18 Feb 2024 14:12:45 +0100 Subject: [PATCH 04/11] homogenize messaging about logger establishment --- pypiper/manager.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/pypiper/manager.py b/pypiper/manager.py index 00c6653..0f160ae 100644 --- a/pypiper/manager.py +++ b/pypiper/manager.py @@ -224,11 +224,13 @@ def __init__( logger_kwargs.setdefault("name", default_logname) try: self._logger = logmuse.logger_via_cli(args) - except logmuse.est.AbsentOptionException: + except logmuse.est.AbsentOptionException as e: self._logger = logmuse.init_logger("pypiper", level="DEBUG") - self.debug("logger_via_cli failed; Logger set with logmuse.init_logger") + logger_builder_method = "init_logger" + self.debug(f"logger_via_cli failed: {e}") else: - self.debug("Logger set with logmuse.logger_via_cli") + logger_builder_method = "logger_via_cli" + self.debug(f"Logger set with logmuse.{logger_builder_method}") # Keep track of an ID for the number of processes attempted self.proc_count = 0 From 9b2ccbc5f83894b1731a45b9ca282c160591a12f Mon Sep 17 00:00:00 2001 From: Vince Reuter Date: Sun, 18 Feb 2024 15:00:42 +0100 Subject: [PATCH 05/11] organize logger construction for pipeline manager, and homogenize treatment of arguments across branches --- pypiper/manager.py | 33 ++++++++++++++++----------------- 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/pypiper/manager.py b/pypiper/manager.py index 0f160ae..76bb306 100644 --- a/pypiper/manager.py +++ b/pypiper/manager.py @@ -209,28 +209,27 @@ def __init__( self.testmode = params["testmode"] # Set up logger - logger_kwargs = copy.deepcopy(logger_kwargs) or {} + logger_kwargs = logger_kwargs or {} 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 = logmuse.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) - except logmuse.est.AbsentOptionException as e: - self._logger = logmuse.init_logger("pypiper", level="DEBUG") - logger_builder_method = "init_logger" - self.debug(f"logger_via_cli failed: {e}") - else: - logger_builder_method = "logger_via_cli" - self.debug(f"Logger set with logmuse.{logger_builder_method}") + self._logger = logmuse.init_logger(name, **logger_kwargs) + self.debug(f"Logger set with logmuse.{logger_builder_method}") # Keep track of an ID for the number of processes attempted self.proc_count = 0 From 76ffdc6ba7988b5e62d510c09e37b052d0f5167a Mon Sep 17 00:00:00 2001 From: Vince Reuter Date: Sun, 18 Feb 2024 15:00:58 +0100 Subject: [PATCH 06/11] tweak syntax --- pypiper/utils.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pypiper/utils.py b/pypiper/utils.py index 8973466..37b396c 100644 --- a/pypiper/utils.py +++ b/pypiper/utils.py @@ -788,7 +788,7 @@ def pipeline_filepath(pm, filename=None, suffix=None): 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 "") From 7baa056fda3d35a83c022565223b9f75f38ec466 Mon Sep 17 00:00:00 2001 From: Vince Reuter Date: Sun, 18 Feb 2024 15:01:20 +0100 Subject: [PATCH 07/11] use default strict=False behavior for logger via CLI --- pypiper/manager.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pypiper/manager.py b/pypiper/manager.py index 76bb306..80b8d1e 100644 --- a/pypiper/manager.py +++ b/pypiper/manager.py @@ -215,7 +215,7 @@ def __init__( if args: logger_builder_method = "logger_via_cli" try: - self._logger = logmuse.logger_via_cli(args, **logger_kwargs) + 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}") @@ -229,7 +229,7 @@ def __init__( except KeyError: name = default_logname self._logger = logmuse.init_logger(name, **logger_kwargs) - self.debug(f"Logger set with logmuse.{logger_builder_method}") + self.debug(f"Logger set with {logger_builder_method}") # Keep track of an ID for the number of processes attempted self.proc_count = 0 From a83025bc550f8d224bf3e352c26cbe6c8400dfc8 Mon Sep 17 00:00:00 2001 From: Vince Reuter Date: Sun, 18 Feb 2024 15:05:48 +0100 Subject: [PATCH 08/11] move up logfile setting to set up solution for #212 --- pypiper/manager.py | 9 +++++---- pypiper/utils.py | 2 -- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/pypiper/manager.py b/pypiper/manager.py index 80b8d1e..7473d97 100644 --- a/pypiper/manager.py +++ b/pypiper/manager.py @@ -8,7 +8,6 @@ """ import atexit -import copy import datetime import errno import glob @@ -208,6 +207,11 @@ 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="_log.md") + # Set up logger logger_kwargs = logger_kwargs or {} default_logname = ".".join([__name__, self.__class__.__name__, self.name]) @@ -276,10 +280,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. diff --git a/pypiper/utils.py b/pypiper/utils.py index 37b396c..1c65f59 100644 --- a/pypiper/utils.py +++ b/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." ) - filename = (filename or pm.name) + (suffix or "") # Note that Pipeline and PipelineManager define the same outfolder. From ba423a428ff8279b508951efdc20d1206c1f14c1 Mon Sep 17 00:00:00 2001 From: Vince Reuter Date: Sun, 18 Feb 2024 15:08:48 +0100 Subject: [PATCH 09/11] check that logfile for logger doesn't match manager's own logfile; #212 --- pypiper/manager.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/pypiper/manager.py b/pypiper/manager.py index 7473d97..6cae014 100644 --- a/pypiper/manager.py +++ b/pypiper/manager.py @@ -214,6 +214,10 @@ def __init__( # 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]) self._logger = None if args: From 97821e986a5c248cba2dd885ace7d34bf3d5f3e5 Mon Sep 17 00:00:00 2001 From: Vince Reuter Date: Sun, 18 Feb 2024 15:16:23 +0100 Subject: [PATCH 10/11] test solution for #212 --- pypiper/manager.py | 3 ++- .../test_manager_constructor.py | 19 ++++++++++++++++++- 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/pypiper/manager.py b/pypiper/manager.py index 6cae014..df661b3 100644 --- a/pypiper/manager.py +++ b/pypiper/manager.py @@ -58,6 +58,7 @@ LOCK_PREFIX = "lock." +LOGFILE_SUFFIX = "_log.md" class Unbuffered(object): @@ -210,7 +211,7 @@ def __init__( # 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="_log.md") + self.pipeline_log_file = pipeline_filepath(self, suffix=LOGFILE_SUFFIX) # Set up logger logger_kwargs = logger_kwargs or {} diff --git a/tests/pipeline_manager/test_manager_constructor.py b/tests/pipeline_manager/test_manager_constructor.py index 0792bf1..b327fb2 100644 --- a/tests/pipeline_manager/test_manager_constructor.py +++ b/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.""" From fcb02facd2da19b3e129769a67729f35a571696d Mon Sep 17 00:00:00 2001 From: Vince Reuter Date: Sun, 18 Feb 2024 15:50:28 +0100 Subject: [PATCH 11/11] apply new black formatting --- pypiper/const.py | 1 - pypiper/ngstk.py | 3 +-- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/pypiper/const.py b/pypiper/const.py index 2749529..0bfbdb6 100644 --- a/pypiper/const.py +++ b/pypiper/const.py @@ -1,6 +1,5 @@ """ Pypiper constants. """ - CHECKPOINT_EXTENSION = ".checkpoint" DEFAULT_SAMPLE_NAME = "DEFAULT_SAMPLE_NAME" PIPELINE_CHECKPOINT_DELIMITER = "_" diff --git a/pypiper/ngstk.py b/pypiper/ngstk.py index 329b321..b607913 100755 --- a/pypiper/ngstk.py +++ b/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, )