Repository: databio/pypiper (language: python)

Issue:
Critical bugs with reporting results
I just found 2 critical bugs with pypiper result reporting. Actually it may just be 2 outcomes from the same critical bug.

When you report a result *it doesn't overwrite the result if there's already one there*. This means there's no way to update something if an incorrect result was entered. In the past, results would be reported as many times as you ran the pipeline. All of these results are recorded. Now, the pipeline only reports the first result. If you re-report that result, it's simply lost.

When I'm re-running a pipeline to correct something, I get these:

```
These results exist for 'demo4': seqcol_digest
These results exist for 'demo4': Time
These results exist for 'demo4': Success
```

And they're not updated.  So, that's the first problem: reporting of results is limited to a single value now, and multiple reports is not recorded.

The second issue is related to it:

The result is not only not reported to the `results.yaml` file, it's *also* not reported to screen! So it doesn't even show up in the log file anymore. Results should be reported to screen *every time* a result report is requested.


## Solutions

The second issue should be easy to solve, just make sure pypiper is always printing the result. I'm not sure if this should happen in pypiper or in pipestat, but somewhere it has to happen.

The first problem is harder to solve, because it's a consequence of the switch from a simple tab-delimited stats.tsv file pypiper used to use, to the pipestat file. In pipestat, results are recorded *per sample*, not *per pipeline run*, which means there's going to be issues when you re-run a sample. The best I can think of is this:

1. In the short term, at the very least, the *last reported result* should be the one used, not the first reported result, as is currently happening. I guess this is as simple as forcing updates whenever you report an existing result.
2. In the longer term, pipestat should offer the option to include a history of results, and these should be stored somehow in the file (and database). This may not actually be too hard to implement; just add a 'history' function, and when something is overwritten, just move the old values into the history in a way that is an array, rather than a single value. Then, pipestat could offer a `clear history` function to remove old stuff, if desired, but otherwise, repeated reports of the same result will simply add to the history. 


Initial logging message about a stage is a bit odd
`isinstance(stage, Stage)` --> `print("Running stage: {}".format(stage)` yields something like "Running stage: Stage: ..."
Check that logfile for manager's logger doesn't collide with manager's own logfile
Close #212 
Close #211
Pipeline log file could collide with filepath given to logmuse
From consideration of #210 , it becomes apparent that a user could specify a path to a logfile for a pipeline manager's logger to write to that would collide with the path the manager will generate for its own log file. This would almost surely be inadvertent and would lead to unexpected behavior like log file overwrites, duplicate messages, and possibly a race condition on file writes.

To protect against this, I think that in any pipeline manager constructor, we should...
1. check that there's not this collision
2. if there is a collision, it should be an __exception__

It should be an exception rather than a warning because...
a) It's truly exceptional
b) It's almost surely unintended
c) It would lead to undesirable behavior
d) it would be at the _start_ of almost any pipeline, so raising an exception wouldn't risk wasting a lot of compute progress
e) it's unrecoverable -- there's not really a suitable automatic adjustment to make; the hypothetical filepaths in question (logmuse logfile and pypiper log file) may be expected to be present with _particular_ values, e.g. for consumption by other tools, and so doing something like injecting some distinguishing identifier into the filename suffix doesn't seem suitable

@nsheff would you accept (in principle, code review TBD ;) ) a pull request to this effect?
`pipestat_project_name` parameter to manager constructor is currently unused
Is this a known issue? What's its intended use?
Critical bugs with reporting results
I just found 2 critical bugs with pypiper result reporting. Actually it may just be 2 outcomes from the same critical bug.

When you report a result *it doesn't overwrite the result if there's already one there*. This means there's no way to update something if an incorrect result was entered. In the past, results would be reported as many times as you ran the pipeline. All of these results are recorded. Now, the pipeline only reports the first result. If you re-report that result, it's simply lost.

When I'm re-running a pipeline to correct something, I get these:

```
These results exist for 'demo4': seqcol_digest
These results exist for 'demo4': Time
These results exist for 'demo4': Success
```

And they're not updated.  So, that's the first problem: reporting of results is limited to a single value now, and multiple reports is not recorded.

The second issue is related to it:

The result is not only not reported to the `results.yaml` file, it's *also* not reported to screen! So it doesn't even show up in the log file anymore. Results should be reported to screen *every time* a result report is requested.


## Solutions

The second issue should be easy to solve, just make sure pypiper is always printing the result. I'm not sure if this should happen in pypiper or in pipestat, but somewhere it has to happen.

The first problem is harder to solve, because it's a consequence of the switch from a simple tab-delimited stats.tsv file pypiper used to use, to the pipestat file. In pipestat, results are recorded *per sample*, not *per pipeline run*, which means there's going to be issues when you re-run a sample. The best I can think of is this:

1. In the short term, at the very least, the *last reported result* should be the one used, not the first reported result, as is currently happening. I guess this is as simple as forcing updates whenever you report an existing result.
2. In the longer term, pipestat should offer the option to include a history of results, and these should be stored somehow in the file (and database). This may not actually be too hard to implement; just add a 'history' function, and when something is overwritten, just move the old values into the history in a way that is an array, rather than a single value. Then, pipestat could offer a `clear history` function to remove old stuff, if desired, but otherwise, repeated reports of the same result will simply add to the history. 


Replace read_yaml_data with yacman.load_yaml
We recently replaced a pipestat helper function (read_yaml_data) with yacman's load_yaml function.

Pypiper also relies on this pipestat function so it should also be replaced.

I will require Pypiper to have the latest prelease version and above of pipestat (v0.9.0a1) as well as yacman v0.9.3 and ubiquerg>=0.8.0


Discussion / hints:

Fix patch:
diff --git a/.gitignore b/.gitignore
index bc05997..c1e53a2 100644
--- a/.gitignore
+++ b/.gitignore
@@ -86,3 +86,6 @@ piper.egg-info/
 
 *ipynb_checkpoints*
 *.egg-info*
+
+
+example_pipelines/pipeline_output
diff --git a/README.md b/README.md
index 046003d..7e6db31 100644
--- a/README.md
+++ b/README.md
@@ -4,7 +4,7 @@
 
 [![Documentation Status](https://readthedocs.org/projects/pypiper/badge/?version=latest)](http://pypiper.readthedocs.org/en/latest/?badge=latest)
 [![Build Status](https://github.com/databio/pypiper/actions/workflows/run-pytest.yml/badge.svg)](https://github.com/databio/pypiper/actions/workflows/run-pytest.yml)
-[![PEP compatible](http://pepkit.github.io/img/PEP-compatible-green.svg)](http://pepkit.github.io)
+[![PEP compatible](https://pepkit.github.io/img/PEP-compatible-green.svg)](http://pepkit.github.io)
 [![pypi-badge](https://img.shields.io/pypi/v/piper)](https://pypi.org/project/piper)
 [![Code style: black](https://img.shields.io/badge/code%20style-black-000000.svg)](https://github.com/psf/black)
 
diff --git a/docs/changelog.md b/docs/changelog.md
index edbdfef..feba103 100644
--- a/docs/changelog.md
+++ b/docs/changelog.md
@@ -1,5 +1,13 @@
 # Changelog
 
+## [0.14.1] -- 2024-04-19
+### Changed
+- remove pipestat_project_name from PipelineManager parameters
+- refactor pipestat_sample_name to pipestat_record_identifier in PipelineManager parameters
+- update requirements for pipestat 0.9.0, ubiquerg 0.8.0, and yacman 0.9.3
+- set `force_overwrite` to default to true, Issue #209
+
+
 ## [0.14.0] -- 2023-12-22
 ### Changed
 - refactor for pipestat v0.6.0 release
diff --git a/example_pipelines/basic.py b/example_pipelines/basic.py
index 34a0d37..c7497b2 100755
--- a/example_pipelines/basic.py
+++ b/example_pipelines/basic.py
@@ -26,7 +26,7 @@
 tgt = "pipeline_output/test.out"
 
 # build the command
-cmd = "shuf -i 1-500000000 -n 10000000 > " + tgt
+cmd = f"shuf -i 1-500000000 -n 10000000 > {tgt}"
 
 # and run with run().
 pm.run(cmd, target=tgt)
@@ -34,7 +34,7 @@
 # Now copy the data into a new file.
 # first specify target file and build command:
 tgt = "pipeline_output/copied.out"
-cmd = "cp pipeline_output/test.out " + tgt
+cmd = f"cp pipeline_output/test.out {tgt}"
 pm.run(cmd, target=tgt)
 
 # You can also string multiple commands together, which will execute
diff --git a/example_pipelines/hello_pypiper.py b/example_pipelines/hello_pypiper.py
index 88abecf..17d424a 100755
--- a/example_pipelines/hello_pypiper.py
+++ b/example_pipelines/hello_pypiper.py
@@ -12,7 +12,7 @@
 
 # Now build a command-line command however you like, and pass it to pm.run()
 target_file = "hello_pypiper_results/output.txt"
-cmd = "echo 'Hello, Pypiper!' > " + target_file
+cmd = f"echo 'Hello, Pypiper!' > {target_file}"
 pm.run(cmd, target_file)
 
 pm.stop_pipeline()
diff --git a/pypiper/_version.py b/pypiper/_version.py
index 9e78220..f075dd3 100644
--- a/pypiper/_version.py
+++ b/pypiper/_version.py
@@ -1,1 +1,1 @@
-__version__ = "0.14.0"
+__version__ = "0.14.1"
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/manager.py b/pypiper/manager.py
index 67441d6..965945b 100644
--- a/pypiper/manager.py
+++ b/pypiper/manager.py
@@ -52,12 +52,13 @@
     default_pipestat_output_schema,
     result_formatter_markdown,
 )
-from pipestat.helpers import read_yaml_data
+from yacman import load_yaml
 
 __all__ = ["PipelineManager"]
 
 
 LOCK_PREFIX = "lock."
+LOGFILE_SUFFIX = "_log.md"
 
 
 class Unbuffered(object):
@@ -112,6 +113,12 @@ class PipelineManager(object):
         protect from a case in which a restart begins upstream of a stage
         for which a checkpoint file already exists, but that depends on the
         upstream stage and thus should be rerun if it's "parent" is rerun.
+    :param str pipestat_record_identifier: record_identifier to report results via pipestat
+    :param str pipestat_schema: output schema used by pipestat to report results
+    :param str pipestat_results_file: path to file backend for reporting results
+    :param str pipestat_config_file: path to pipestat configuration file
+    :param str pipestat_pipeline_type: Sample or Project level pipeline
+    :param pipestat_result_formatter: function used to style reported results, defaults to result_formatter_markdown
     :raise TypeError: if start or stop point(s) are provided both directly and
         via args namespace, or if both stopping types (exclusive/prospective
         and inclusive/retrospective) are provided.
@@ -136,8 +143,7 @@ def __init__(
         output_parent=None,
         overwrite_checkpoints=False,
         logger_kwargs=None,
-        pipestat_project_name=None,
-        pipestat_sample_name=None,
+        pipestat_record_identifier=None,
         pipestat_schema=None,
         pipestat_results_file=None,
         pipestat_config=None,
@@ -193,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
@@ -210,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
@@ -276,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.
@@ -330,7 +341,9 @@ def __init__(
         signal.signal(signal.SIGTERM, self._signal_term_handler)
 
         # pipestat setup
-        self.pipestat_record_identifier = pipestat_sample_name or DEFAULT_SAMPLE_NAME
+        self.pipestat_record_identifier = (
+            pipestat_record_identifier or DEFAULT_SAMPLE_NAME
+        )
         self.pipestat_pipeline_type = pipestat_pipeline_type or "sample"
 
         # don't force default pipestat_results_file value unless
@@ -631,88 +644,41 @@ def start_pipeline(self, args=None, multi=False):
         # Print out a header section in the pipeline log:
         # Wrap things in backticks to prevent markdown from interpreting underscores as emphasis.
         # print("----------------------------------------")
-        self.info("### Pipeline run code and environment:\n")
-        self.info(
-            "* " + "Command".rjust(20) + ":  " + "`" + str(" ".join(sys.argv)) + "`"
-        )
-        self.info("* " + "Compute host".rjust(20) + ":  " + platform.node())
-        self.info("* " + "Working dir".rjust(20) + ":  " + os.getcwd())
-        self.info("* " + "Outfolder".rjust(20) + ":  " + self.outfolder)
+        def logfmt(key, value=None, padding=16):
+            padded_key = key.rjust(padding)
+            formatted_val = f"`{value}`" if value else ""
+            return f"* {padded_key}: {formatted_val}"
 
-        self.timestamp("* " + "Pipeline started at".rjust(20) + ":  ")
+        self.info("### Pipeline run code and environment:\n")
+        self.info(logfmt("Command", str(" ".join(sys.argv))))
+        self.info(logfmt("Compute host", platform.node()))
+        self.info(logfmt("Working dir", os.getcwd()))
+        self.info(logfmt("Outfolder", self.outfolder))
+        self.info(logfmt("Log file", self.pipeline_log_file))
+        self.timestamp(logfmt("Start time"))
 
         self.info("\n### Version log:\n")
-        self.info("* " + "Python version".rjust(20) + ":  " + platform.python_version())
+        self.info(logfmt("Python version", platform.python_version()))
         try:
-            self.info(
-                "* "
-                + "Pypiper dir".rjust(20)
-                + ":  "
-                + "`"
-                + gitvars["pypiper_dir"].strip()
-                + "`"
-            )
-            self.info("* " + "Pypiper version".rjust(20) + ":  " + __version__)
-            self.info(
-                "* " + "Pypiper hash".rjust(20) + ":  " + str(gitvars["pypiper_hash"])
-            )
-            self.info(
-                "* "
-                + "Pypiper branch".rjust(20)
-                + ":  "
-                + str(gitvars["pypiper_branch"])
-            )
-            self.info(
-                "* " + "Pypiper date".rjust(20) + ":  " + str(gitvars["pypiper_date"])
-            )
+            self.info(logfmt("Pypiper dir", gitvars["pypiper_dir"].strip()))
+            self.info(logfmt("Pypiper version", __version__))
+            self.info(logfmt("Pypiper hash", gitvars["pypiper_hash"]))
+            self.info(logfmt("Pypiper branch", gitvars["pypiper_branch"]))
+            self.info(logfmt("Pypiper date", gitvars["pypiper_date"]))
             if gitvars["pypiper_diff"]:
-                self.info(
-                    "* "
-                    + "Pypiper diff".rjust(20)
-                    + ":  "
-                    + str(gitvars["pypiper_diff"])
-                )
+                self.info(logfmt("Pypiper diff", gitvars["pypiper_diff"]))
         except KeyError:
             # It is ok if keys aren't set, it means pypiper isn't in a  git repo.
             pass
 
         try:
-            self.info(
-                "* "
-                + "Pipeline dir".rjust(20)
-                + ":  "
-                + "`"
-                + gitvars["pipe_dir"].strip()
-                + "`"
-            )
-            self.info(
-                "* " + "Pipeline version".rjust(20) + ":  " + str(self.pl_version)
-            )
-            self.info(
-                "* "
-                + "Pipeline hash".rjust(20)
-                + ":  "
-                + str(gitvars["pipe_hash"]).strip()
-            )
-            self.info(
-                "* "
-                + "Pipeline branch".rjust(20)
-                + ":  "
-                + str(gitvars["pipe_branch"]).strip()
-            )
-            self.info(
-                "* "
-                + "Pipeline date".rjust(20)
-                + ":  "
-                + str(gitvars["pipe_date"]).strip()
-            )
+            self.info(logfmt("Pipeline dir", gitvars["pipe_dir"].strip()))
+            self.info(logfmt("Pipeline version", self.pl_version))
+            self.info(logfmt("Pipeline hash", gitvars["pipe_hash"]).strip())
+            self.info(logfmt("Pipeline branch", gitvars["pipe_branch"]).strip())
+            self.info(logfmt("Pipeline date", gitvars["pipe_date"]).strip())
             if gitvars["pipe_diff"] != "":
-                self.info(
-                    "* "
-                    + "Pipeline diff".rjust(20)
-                    + ":  "
-                    + str(gitvars["pipe_diff"]).strip()
-                )
+                self.info(logfmt("Pipeline diff", gitvars["pipe_diff"]).strip())
         except KeyError:
             # It is ok if keys aren't set, it means the pipeline isn't a git repo.
             pass
@@ -1593,7 +1559,7 @@ def _report_profile(
             myfile.write(message_raw + "\n")
 
     def report_result(
-        self, key, value, nolog=False, result_formatter=None, force_overwrite=False
+        self, key, value, nolog=False, result_formatter=None, force_overwrite=True
     ):
         """
         Writes a key:value pair to self.pipeline_stats_file.
@@ -1640,7 +1606,7 @@ def report_object(
         annotation=None,
         nolog=False,
         result_formatter=None,
-        force_overwrite=False,
+        force_overwrite=True,
     ):
         """
         Writes a key:value pair to self.pipeline_stats_file. Note: this function
@@ -1862,7 +1828,7 @@ def _refresh_stats(self):
         """
 
         if os.path.isfile(self.pipeline_stats_file):
-            _, data = read_yaml_data(path=self.pipeline_stats_file, what="stats_file")
+            data = load_yaml(filepath=self.pipeline_stats_file)
 
             for key, value in data[self._pipestat_manager.pipeline_name][
                 self._pipestat_manager.pipeline_type
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,
         )
 
diff --git a/pypiper/utils.py b/pypiper/utils.py
index 8973466..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."
+            "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.
diff --git a/requirements/requirements-docs.txt b/requirements/requirements-docs.txt
index ef5b4e6..9ae381f 100644
--- a/requirements/requirements-docs.txt
+++ b/requirements/requirements-docs.txt
@@ -2,5 +2,5 @@ mkdocs>=1.0
 markdown-include
 pydoc-markdown
 piper
-pipestat>=0.6.0
+pipestat>=0.9.0a1
 https://github.com/databio/mkdocs-databio/archive/master.zip
\ No newline at end of file
diff --git a/requirements/requirements-pypiper.txt b/requirements/requirements-pypiper.txt
index 9a35f34..180ad3d 100644
--- a/requirements/requirements-pypiper.txt
+++ b/requirements/requirements-pypiper.txt
@@ -1,6 +1,6 @@
 logmuse>=0.2.4
 psutil
 pandas
-ubiquerg>=0.4.5
-yacman
-pipestat>=0.6.0
+ubiquerg>=0.8.0
+yacman>=0.9.3
+pipestat>=0.9.0a1


Test patch:
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."""