diff --git a/changes/198.feature.rst b/changes/198.feature.rst new file mode 100644 index 00000000..1b0b0629 --- /dev/null +++ b/changes/198.feature.rst @@ -0,0 +1 @@ +Warn if Step.run is called without configuring the step with parameters from CRDS. diff --git a/src/stpipe/config_parser.py b/src/stpipe/config_parser.py index cb1af2a1..018b5ef9 100644 --- a/src/stpipe/config_parser.py +++ b/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: diff --git a/src/stpipe/pipeline.py b/src/stpipe/pipeline.py index 70eaf00c..c4890da2 100644 --- a/src/stpipe/pipeline.py +++ b/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 diff --git a/src/stpipe/step.py b/src/stpipe/step.py index f956c454..d2483d17 100644 --- a/src/stpipe/step.py +++ b/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 @@ class Step: # 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 @@ def from_config_section( 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 @@ def run(self, *args): 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 @@ def get_config_from_reference(cls, dataset, disable=None, crds_observatory=None) ) 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 @@ def get_config_from_reference(cls, dataset, disable=None, crds_observatory=None) 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): """ diff --git a/tests/test_config_parser.py b/tests/test_config_parser.py index d1e5cbb5..86640367 100644 --- a/tests/test_config_parser.py +++ b/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 diff --git a/tests/test_hooks.py b/tests/test_hooks.py index 4d7d40a6..a19bdb93 100644 --- a/tests/test_hooks.py +++ b/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, diff --git a/tests/test_step.py b/tests/test_step.py index 972f84a9..0fa152c6 100644 --- a/tests/test_step.py +++ b/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()