Fix pre- and post-hooks (#133)

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

.github/workflows/ci.yml

@@ -27,6 +27,8 @@ jobs:
         - macos: py311-xdist
         - linux: py311-cov-xdist
           coverage: 'codecov'
+        - linux: py311-downstreamdeps-cov-xdist
+          coverage: 'codecov'
         - linux: py312-xdist-nolegacypath
   test_downstream:
     uses: OpenAstronomy/github-actions-workflows/.github/workflows/tox.yml@v1

CHANGES.rst

@@ -10,6 +10,9 @@
   ``get_crds_parameters`` [#142]
 - Fix bug in handling of ``pathlib.Path`` objects as ``Step`` inputs [#143]
 - Log readable Step parameters [#140]
+- Fix handling of functions and subprocesses as Step pre- and post-hooks.  Add
+  ability to pass imported python functions and ``Step`` subclasses directly as
+  hooks. And allow ``Step`` subclass instances with set parameters as hooks. [#133]
 
 0.5.1 (2023-10-02)
 ==================

src/stpipe/config_parser.py

@@ -80,7 +80,7 @@ def _output_file_check(path):
 
 
 def _is_datamodel(value):
-    """Verify that value is either is a DataModel."""
+    """Verify that value is a DataModel."""
     if isinstance(value, AbstractDataModel):
         return value
 

src/stpipe/function_wrapper.py

@@ -9,8 +9,12 @@ class FunctionWrapper(Step):
     This Step wraps an ordinary Python function.
     """
 
-    def __init__(self, func, *args):
-        Step.__init__(self, func.__name__, *args)
+    spec = """
+    output_ext = string(default="fits")
+    """
+
+    def __init__(self, func, *args, **kwargs):
+        Step.__init__(self, func.__name__, *args, **kwargs)
 
         self._func = func
 

src/stpipe/hooks.py

@@ -1,40 +1,108 @@
 """
 Pre- and post-hooks
 """
-import contextlib
-import types
+import ast
+import inspect
 
-from . import utilities
+from . import function_wrapper, utilities
 from .step import Step
 
 
-def hook_from_string(step, type, num, command):  # noqa: A002
-    name = f"{type}_hook{num:d}"
+def hook_from_string(step, hooktype, num, command):
+    """
+    Generate hook from string, function, Step or Step instance
 
-    step_class = None
-    with contextlib.suppress(Exception):
-        step_class = utilities.import_class(command, Step, step.config_file)
+    Parameters
+    ----------
+    step : `stpipe.step.Step`
+        Parent step instance to which this hook is attached, i.e. "self"
+    hooktype : str, "pre" or "post"
+        Type of hook pre-hook , or post-hook
+    num : int
+        The number, in order, of the pre- or post-hook in the list
+    command : str or `stpipe.step.Step` instance
 
-    if step_class is not None:
-        return step_class(name, parent=step, config_file=step.config_file)
+    Returns
+    -------
+    `stpipe.step.Step`
+    """
+    name = f"{hooktype}_hook{num:d}"
 
+    step_class = None
     step_func = None
-    with contextlib.suppress(Exception):
-        step_func = utilities.import_class(
-            command, types.FunctionType, step.config_file
-        )
 
-    if step_func is not None:
-        from . import function_wrapper
+    # hook is a string of the fully-qualified name of a class or function
+    if isinstance(command, str):
+        try:
+            # String points to a Step subclass
+            step_class = utilities.import_class(
+                command, subclassof=Step, config_file=step.config_file
+            )
+        except ImportError:
+            # String is possibly a subproc, so handle this later
+            pass
+        except AttributeError:
+            # String points to an instance of a Step
+            # So import the class
+            class_string, _, params = command.partition("(")
+            step_class = utilities.import_class(
+                class_string, subclassof=Step, config_file=step.config_file
+            )
+            # Then convert rest of string to args and instantiate the class
+            kwargs_string = params.strip(")")
+            expr = ast.parse(f"dict({kwargs_string}\n)", mode="eval")
+            kwargs = {kw.arg: ast.literal_eval(kw.value) for kw in expr.body.keywords}
+            return step_class(**kwargs)
+        except TypeError:
+            # String points to a function
+            step_func = utilities.import_func(command)
+        else:
+            if step_class.class_alias is not None:
+                name = step_class.class_alias
+            return step_class(name, parent=step, config_file=step.config_file)
+
+        # hook is a string of the fully-qualified name of a function
+        if step_func is not None:
+            return function_wrapper.FunctionWrapper(
+                step_func, parent=step, config_file=step.config_file
+            )
 
-        return function_wrapper.FunctionWrapper(
-            step_func, parent=step, config_file=step.config_file
-        )
+    # hook is an already-imported Step subclass
+    if inspect.isclass(command) and issubclass(command, Step):
+        step_class = command
+        if step_class.class_alias is not None:
+            name = step_class.class_alias
+        return step_class(name, parent=step, config_file=step.config_file)
+
+    # hook is an instance of a Step subclass
+    if isinstance(command, Step):
+        if command.class_alias is not None:
+            command.name = command.class_alias
+        else:
+            command.name = name
+        return command
 
+    # hook is a command-line script or system call
     from .subproc import SystemCall
 
     return SystemCall(name, parent=step, command=command)
 
 
-def get_hook_objects(step, type_, hooks):
-    return [hook_from_string(step, type_, i, hook) for i, hook in enumerate(hooks)]
+def get_hook_objects(step, hooktype, hooks):
+    """
+    Get list of pre- or post-hooks for a step
+
+    Parameters
+    ----------
+    step : `stpipe.step.Step`
+        instance to which this is a hook
+    hooktype : str, "pre" or "post"
+        strings, to indicate whether it is a pre- or post-hook
+    hooks : str or class
+        path to executable script, or Step class to run as hook
+
+    Returns
+    -------
+    list of callables that can be run as a hook
+    """
+    return [hook_from_string(step, hooktype, i, hook) for i, hook in enumerate(hooks)]

src/stpipe/step.py

@@ -42,8 +42,8 @@ class Step:
     """
 
     spec = """
-    pre_hooks          = string_list(default=list())
-    post_hooks         = string_list(default=list())
+    pre_hooks          = list(default=list())        # List of Step classes to run before step
+    post_hooks         = list(default=list())        # List of Step classes to run after step
     output_file        = output_file(default=None)   # File to save output to.
     output_dir         = string(default=None)        # Directory path for output files
     output_ext         = string()                    # Default type of output

src/stpipe/subproc.py

@@ -1,6 +1,7 @@
 import os
 import subprocess
 
+from .datamodel import AbstractDataModel
 from .step import Step
 
 
@@ -11,30 +12,24 @@ class SystemCall(Step):
 
     spec = """
     # SystemCall is a step to run external processes as Steps.
-
     # The command can pass along arguments that were passed to the step.
     # To refer to positional arguments, use {0}, {1}, {2}, etc.
     # To refer to keyword arguments, use {keyword}.
-    command = string() # The command to execute
 
+    command = string() # The command to execute
     env = string_list(default=list()) # Environment variables to define
-
     log_stdout = boolean(default=True) # Do we want to log STDOUT?
-
     log_stderr = boolean(default=True) # Do we want to log STDERR?
-
     exitcode_as_exception = boolean(default=True) # Should a non-zero exit code be converted into an exception?
-
     failure_as_exception = boolean(default=True) # If subprocess fails to run at all, should that be an exception?
+    output_ext = string(default="fits")
     """  # noqa: E501
 
     def process(self, *args):
-        from .. import datamodels  # noqa: TID252
-
         newargs = []
         for i, arg in enumerate(args):
-            if isinstance(arg, datamodels.DataModel):
-                filename = f"{self.qualified_name}.{i:04d}.{arg.getfileext()}"
+            if isinstance(arg, AbstractDataModel):
+                filename = f"{self.qualified_name}.{i:04d}.{self.output_ext}"
                 arg.save(filename)
                 newargs.append(filename)
             else:
@@ -75,3 +70,6 @@ def process(self, *args):
 
             if self.exitcode_as_exception and err != 0:
                 raise OSError(f"{cmd_str!r} returned error code {err}")
+        finally:
+            p.stdout.close()
+            p.stderr.close()

src/stpipe/utilities.py

@@ -4,6 +4,7 @@
 import inspect
 import os
 import sys
+import types
 
 from . import entry_points
 
@@ -54,7 +55,7 @@ def import_class(full_name, subclassof=object, config_file=None):
 
     try:
         full_name = full_name.strip()
-        package_name, sep, class_name = full_name.rpartition(".")
+        package_name, _, class_name = full_name.rpartition(".")
         if not package_name:
             raise ImportError(f"{full_name} is not a Python class")
         imported = __import__(
@@ -86,6 +87,39 @@ def import_class(full_name, subclassof=object, config_file=None):
     return step_class
 
 
+def import_func(full_name):
+    """
+    Import the Python class `full_name` given in full Python package format,
+    e.g.::
+
+        package.subpackage.subsubpackage.function_name
+
+    Return the imported function.
+    """
+    full_name = full_name.strip()
+    package_name, _, func_name = full_name.rpartition(".")
+    if not package_name:
+        raise ImportError(f"{full_name} is not a fully qualified path to function")
+    imported = __import__(
+        package_name,
+        globals(),
+        locals(),
+        [
+            func_name,
+        ],
+        level=0,
+    )
+
+    step_func = getattr(imported, func_name)
+
+    if not isinstance(step_func, types.FunctionType):
+        raise TypeError(
+            f"Object {func_name} from package {package_name} is not a function"
+        )
+
+    return step_func
+
+
 def get_spec_file_path(step_class):
     """
     Given a Step (sub)class, divine and return the full path to the

tests/conftest.py

@@ -0,0 +1,15 @@
+import os
+from pathlib import Path
+
+import pytest
+
+
+@pytest.fixture()
+def tmp_cwd(tmp_path):
+    """Perform test in a pristine temporary working directory."""
+    old_dir = Path.cwd()
+    os.chdir(tmp_path)
+    try:
+        yield tmp_path
+    finally:
+        os.chdir(old_dir)

tests/test_abstract_datamodel.py

@@ -9,15 +9,15 @@
 
 def test_roman_datamodel():
     roman_datamodels = pytest.importorskip("roman_datamodels.datamodels")
-    import roman_datamodels.tests.util as rutil
+    from roman_datamodels.maker_utils import mk_level2_image
 
-    roman_image_tree = rutil.mk_level2_image()
+    roman_image_tree = mk_level2_image()
     image_model = roman_datamodels.ImageModel(roman_image_tree)
     assert isinstance(image_model, AbstractDataModel)
 
 
 def test_jwst_datamodel():
-    jwst_datamodel = pytest.importorskip("jwst.datamodels")
+    jwst_datamodel = pytest.importorskip("stdatamodels.jwst.datamodels")
     image_model = jwst_datamodel.ImageModel()
     assert isinstance(image_model, AbstractDataModel)