Warn during Step.run when the step hasn't been configured from CRDS

changes/198.feature.rst

@@ -0,0 +1 @@
+Warn if Step.run is called without configuring the step with parameters from CRDS.

src/stpipe/config_parser.py

@@ -241,6 +241,9 @@ def merge_config(into, new):
         inline_comments = {}
         comments = {}
 
+    if hasattr(new, "_from_crds") and not hasattr(into, "_from_crds"):
+        into._from_crds = True
+
     for key, val in new.items():
         if isinstance(val, Section):
             if key not in into:

src/stpipe/pipeline.py

@@ -216,6 +216,7 @@ def get_config_from_reference(cls, dataset, disable=None, crds_observatory=None)
             else:
                 logger.debug("No %s reference files found.", reftype.upper())
 
+        refcfg._from_crds = True
         return refcfg
 
     @classmethod

src/stpipe/step.py

@@ -5,6 +5,7 @@
 import gc
 import os
 import sys
+import warnings
 from collections.abc import Sequence
 from contextlib import contextmanager, suppress
 from functools import partial
@@ -38,6 +39,26 @@
 from .utilities import _not_set
 
 
+class NoCRDSParametersWarning(UserWarning):
+    """
+    Warning shown when a Step with CRDS parameters
+    is run without fetching those parameters.
+    """
+
+    pass
+
+
+def _warn_missing_crds_pars(step):
+    warnings.warn(
+        f"{step.__class__.__name__}.run was called without first getting "
+        "step parameters from CRDS. To create a Step instance using "
+        "CRDS parameters use "
+        "Step.from_config_section(Step.build_config(input)[0]) or "
+        "use Step.call which will create and use the instance.",
+        NoCRDSParametersWarning,
+    )
+
+
 class Step:
     """
     Step
@@ -79,6 +100,10 @@
     # log_records to be saved.
     _log_records_formatter = None
 
+    # If the expectation is that this step has a
+    # CRDS step parameters file set this flag to True
+    _warn_on_missing_crds_steppars = False
+
     @classmethod
     def get_config_reftype(cls):
         """
@@ -295,13 +320,15 @@
             else:
                 kwargs[k] = config[k]
 
-        return cls(
+        instance = cls(
             name=name,
             parent=parent,
             config_file=config_file,
             _validate_kwds=False,
             **kwargs,
         )
+        instance._params_from_crds = getattr(config, "_from_crds", False)
+        return instance
 
     def __init__(
         self,
@@ -419,6 +446,13 @@
         the running of each step.  The real work that is unique to
         each step type is done in the `process` method.
         """
+        if (
+            self._warn_on_missing_crds_steppars
+            and not get_disable_crds_steppars()
+            and self.parent is None
+            and not getattr(self, "_params_from_crds", False)
+        ):
+            _warn_missing_crds_pars(self)
         gc.collect()
 
         with log.record_logs(formatter=self._log_records_formatter) as log_records:
@@ -899,7 +933,9 @@
             )
         except (AttributeError, crds_client.CrdsError):
             logger.debug("%s: No parameters found", reftype.upper())
-            return config_parser.ConfigObj()
+            config = config_parser.ConfigObj()
+            config._from_crds = True
+            return config
         if ref_file != "N/A":
             logger.info("%s parameters found: %s", reftype.upper(), ref_file)
             ref = config_parser.load_config_file(ref_file)
@@ -910,11 +946,14 @@
             logger.debug(
                 "%s parameters retrieved from CRDS: %s", reftype.upper(), ref_pars
             )
+            ref._from_crds = True
 
             return ref
 
         logger.debug("No %s reference files found.", reftype.upper())
-        return config_parser.ConfigObj()
+        config = config_parser.ConfigObj()
+        config._from_crds = True
+        return config
 
     def set_primary_input(self, obj, exclusive=True):
         """

tests/test_config_parser.py

@@ -58,3 +58,25 @@ class Foo:
     assert "initial comment" in spec.initial_comment[0]
     assert "final comment" in spec.final_comment[0]
     assert "inline comment (with parentheses)" in spec.inline_comments["bar"]
+
+
+def _config_from_crds():
+    cfg = ConfigObj()
+    cfg._from_crds = True
+    return cfg
+
+
+@pytest.mark.parametrize(
+    "a, b, from_crds",
+    (
+        (_config_from_crds(), ConfigObj(), True),
+        (ConfigObj(), _config_from_crds(), True),
+        (ConfigObj(), ConfigObj(), False),
+    ),
+)
+def test_merge_preserves_from_crds(a, b, from_crds):
+    """
+    Test that merging configs preserves _from_crds
+    """
+    config_parser.merge_config(a, b)
+    assert getattr(a, "_from_crds", False) == from_crds

tests/test_hooks.py

@@ -7,6 +7,7 @@
 
 class ShovelPixelsStep(Step):
     class_alias = "shovelpixels"
+    _warn_on_missing_crds_steppars = False
 
     def process(self, input_data):
         self.log.info("Shoveling...")
@@ -15,6 +16,7 @@ def process(self, input_data):
 
 class CancelNoiseStep(Step):
     class_alias = "cancelnoise"
+    _warn_on_missing_crds_steppars = False
 
     def process(self, input_data):
         self.log.info("De-noising...")
@@ -23,6 +25,7 @@ def process(self, input_data):
 
 class HookStep(Step):
     class_alias = "myhook"
+    _warn_on_missing_crds_steppars = False
 
     spec = """
     param1 = string(default="bar")
@@ -37,6 +40,7 @@ def process(self, input_data):
 
 class MyPipeline(Pipeline):
     class_alias = "mypipeline"
+    _warn_on_missing_crds_steppars = False
 
     step_defs = {  # noqa: RUF012
         "shovelpixels": ShovelPixelsStep,

tests/test_step.py

@@ -2,15 +2,16 @@
 
 import logging
 import re
+import warnings
 from typing import ClassVar
 
 import asdf
 import pytest
 
 import stpipe.config_parser as cp
-from stpipe import cmdline
+from stpipe import cmdline, crds_client
 from stpipe.pipeline import Pipeline
-from stpipe.step import Step
+from stpipe.step import NoCRDSParametersWarning, Step
 
 
 # ######################
@@ -27,6 +28,9 @@ class SimpleStep(Step):
         output_ext = string(default='simplestep')
     """
 
+    def process(self, *args):
+        return args
+
 
 class SimplePipe(Pipeline):
     """A Pipeline with parameters and one step"""
@@ -41,6 +45,9 @@ class SimplePipe(Pipeline):
 
     step_defs: ClassVar = {"step1": SimpleStep}
 
+    def process(self, *args):
+        return args
+
 
 class LoggingPipeline(Pipeline):
     """A Pipeline that utilizes self.log
@@ -145,41 +152,57 @@ def config_file_list_arg_step(tmp_path):
 def _mock_step_crds(monkeypatch):
     """Mock various crds calls from Step"""
 
-    def mock_get_config_from_reference_pipe(dataset, disable=None):
-        return cp.config_from_dict(
-            {
-                "str1": "from crds",
-                "str2": "from crds",
-                "str3": "from crds",
-                "steps": {
-                    "step1": {
-                        "str1": "from crds",
-                        "str2": "from crds",
-                        "str3": "from crds",
+    class MockModel:
+        crds_observatory = "jwst"
+
+        def get_crds_parameters(self):
+            return {}
+
+        def __enter__(self):
+            return self
+
+        def __exit__(self, *args, **kwargs):
+            pass
+
+    def mock_datamodels_open(*args, **kwargs):
+        return MockModel()
+
+    def mock_get_reference_file(crds_parameters, reftype, crds_observatory):
+        return reftype
+
+    original_load_config = cp.load_config_file
+
+    def mock_load_config_file(ref_file):
+        cfg = {
+            "pars-simplestep": cp.config_from_dict(
+                {"str1": "from crds", "str2": "from crds", "str3": "from crds"},
+            ),
+            "pars-simplepipe": cp.config_from_dict(
+                {
+                    "str1": "from crds",
+                    "str2": "from crds",
+                    "str3": "from crds",
+                    "steps": {
+                        "step1": {
+                            "str1": "from crds",
+                            "str2": "from crds",
+                            "str3": "from crds",
+                        },
                     },
-                },
-            }
-        )
-
-    def mock_get_config_from_reference_step(dataset, disable=None):
-        return cp.config_from_dict(
-            {"str1": "from crds", "str2": "from crds", "str3": "from crds"}
-        )
-
-    def mock_get_config_from_reference_list_arg_step(dataset, disable=None):
-        return cp.config_from_dict({"rotation": "15", "pixel_scale": "0.85"})
-
-    monkeypatch.setattr(
-        SimplePipe, "get_config_from_reference", mock_get_config_from_reference_pipe
-    )
-    monkeypatch.setattr(
-        SimpleStep, "get_config_from_reference", mock_get_config_from_reference_step
-    )
-    monkeypatch.setattr(
-        ListArgStep,
-        "get_config_from_reference",
-        mock_get_config_from_reference_list_arg_step,
-    )
+                }
+            ),
+            "pars-listargstep": cp.config_from_dict(
+                {"rotation": "15", "pixel_scale": "0.85"}
+            ),
+        }.get(ref_file, None)
+        if cfg:
+            return cfg
+        return original_load_config(ref_file)
+
+    for StepClass in (SimpleStep, SimplePipe, ListArgStep):
+        monkeypatch.setattr(StepClass, "_datamodels_open", mock_datamodels_open)
+    monkeypatch.setattr(crds_client, "get_reference_file", mock_get_reference_file)
+    monkeypatch.setattr(cp, "load_config_file", mock_load_config_file)
 
 
 # #####
@@ -411,3 +434,28 @@ def test_log_records():
     pipeline.run()
 
     assert any(r == "This step has called out a warning." for r in pipeline.log_records)
+
+
+@pytest.mark.parametrize("StepClass", (SimpleStep, SimplePipe))
+def test_warning_for_missing_crds_pars(StepClass):
+    s = StepClass()
+    s._warn_on_missing_crds_steppars = True
+    with pytest.warns(NoCRDSParametersWarning):
+        s.run()
+
+
+@pytest.mark.parametrize("StepClass", (SimpleStep, SimplePipe))
+def test_no_warning_for_call(StepClass, _mock_step_crds, monkeypatch):
+    monkeypatch.setattr(StepClass, "_warn_on_missing_crds_steppars", True)
+    with warnings.catch_warnings():
+        warnings.simplefilter("error")
+        StepClass.call("foo")
+
+
+@pytest.mark.parametrize("StepClass", (SimpleStep, SimplePipe))
+def test_no_warning_for_build_config(StepClass, _mock_step_crds, monkeypatch):
+    s = StepClass.from_config_section(StepClass.build_config("foo")[0])
+    s._warn_on_missing_crds_steppars = True
+    with warnings.catch_warnings():
+        warnings.simplefilter("error")
+        s.run()