From 2c8485102410f257bf34adf6e2eb1ccc30d65439 Mon Sep 17 00:00:00 2001 From: Brett Date: Fri, 11 Oct 2024 12:13:15 -0400 Subject: [PATCH 01/11] warn when run is called without crds parameters --- src/stpipe/config_parser.py | 3 +++ src/stpipe/step.py | 24 +++++++++++++++++++++--- 2 files changed, 24 insertions(+), 3 deletions(-) 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/step.py b/src/stpipe/step.py index f956c454..68d383bb 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,10 @@ from .utilities import _not_set +class NoCRDSParametersWarning(UserWarning): + pass + + class Step: """ Step @@ -295,13 +300,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 +426,12 @@ 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 ( + not get_disable_crds_steppars() + and self.parent is None + and not getattr(self, "_params_from_crds", False) + ): + warnings.warn("No CRDS parameters", NoCRDSParametersWarning) gc.collect() with log.record_logs(formatter=self._log_records_formatter) as log_records: @@ -899,7 +912,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 +925,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): """ From 4aec6dace55b9b516003f54481c7456361b1a269 Mon Sep 17 00:00:00 2001 From: Brett Date: Fri, 11 Oct 2024 13:09:34 -0400 Subject: [PATCH 02/11] add flag to control warning --- src/stpipe/step.py | 25 +++++++++++++++++++++++-- tests/test_hooks.py | 4 ++++ 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/src/stpipe/step.py b/src/stpipe/step.py index 68d383bb..2c39fe8d 100644 --- a/src/stpipe/step.py +++ b/src/stpipe/step.py @@ -39,10 +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({step}).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 in one method.", + NoCRDSParametersWarning + ) + + class Step: """ Step @@ -84,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): """ @@ -427,11 +447,12 @@ def run(self, *args): each step type is done in the `process` method. """ if ( - not get_disable_crds_steppars() + 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) ): - warnings.warn("No CRDS parameters", NoCRDSParametersWarning) + _warn_missing_crds_pars(self) gc.collect() with log.record_logs(formatter=self._log_records_formatter) as log_records: 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, From 6dbe5c74598f96733f89404b87b20b2d852a280e Mon Sep 17 00:00:00 2001 From: Brett Date: Fri, 11 Oct 2024 13:18:43 -0400 Subject: [PATCH 03/11] add test for warning --- tests/test_step.py | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/tests/test_step.py b/tests/test_step.py index 972f84a9..fac7d4d5 100644 --- a/tests/test_step.py +++ b/tests/test_step.py @@ -10,7 +10,7 @@ import stpipe.config_parser as cp from stpipe import cmdline from stpipe.pipeline import Pipeline -from stpipe.step import Step +from stpipe.step import NoCRDSParametersWarning, Step # ###################### @@ -27,6 +27,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 +44,9 @@ class SimplePipe(Pipeline): step_defs: ClassVar = {"step1": SimpleStep} + def process(self, *args): + return args + class LoggingPipeline(Pipeline): """A Pipeline that utilizes self.log @@ -411,3 +417,11 @@ 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() From bfe843a9c170656ff90fe87da298da187d244d70 Mon Sep 17 00:00:00 2001 From: Brett Date: Fri, 11 Oct 2024 13:27:40 -0400 Subject: [PATCH 04/11] add tests that call and run for a step build from crds doesn't warn --- src/stpipe/step.py | 4 ++-- tests/test_step.py | 30 +++++++++++++++++++++++++++--- 2 files changed, 29 insertions(+), 5 deletions(-) diff --git a/src/stpipe/step.py b/src/stpipe/step.py index 2c39fe8d..f134e694 100644 --- a/src/stpipe/step.py +++ b/src/stpipe/step.py @@ -39,12 +39,12 @@ from .utilities import _not_set - class NoCRDSParametersWarning(UserWarning): """ Warning shown when a Step with CRDS parameters is run without fetching those parameters. """ + pass @@ -55,7 +55,7 @@ def _warn_missing_crds_pars(step): "CRDS parameters use " "Step.from_config_section(Step.build_config(input)[0]) or " "use Step.call which will create and use the instance in one method.", - NoCRDSParametersWarning + NoCRDSParametersWarning, ) diff --git a/tests/test_step.py b/tests/test_step.py index fac7d4d5..13dde12a 100644 --- a/tests/test_step.py +++ b/tests/test_step.py @@ -2,6 +2,7 @@ import logging import re +import warnings from typing import ClassVar import asdf @@ -152,7 +153,7 @@ 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( + cfg = cp.config_from_dict( { "str1": "from crds", "str2": "from crds", @@ -166,14 +167,20 @@ def mock_get_config_from_reference_pipe(dataset, disable=None): }, } ) + cfg._from_crds = True + return cfg def mock_get_config_from_reference_step(dataset, disable=None): - return cp.config_from_dict( + cfg = cp.config_from_dict( {"str1": "from crds", "str2": "from crds", "str3": "from crds"} ) + cfg._from_crds = True + return cfg def mock_get_config_from_reference_list_arg_step(dataset, disable=None): - return cp.config_from_dict({"rotation": "15", "pixel_scale": "0.85"}) + cfg = cp.config_from_dict({"rotation": "15", "pixel_scale": "0.85"}) + cfg._from_crds = True + return cfg monkeypatch.setattr( SimplePipe, "get_config_from_reference", mock_get_config_from_reference_pipe @@ -425,3 +432,20 @@ def test_warning_for_missing_crds_pars(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() From 956a35356423cdb14ed3cec4e287b66c5956fded Mon Sep 17 00:00:00 2001 From: Brett Date: Fri, 11 Oct 2024 13:37:05 -0400 Subject: [PATCH 05/11] add changelog --- changes/198.general.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 changes/198.general.rst diff --git a/changes/198.general.rst b/changes/198.general.rst new file mode 100644 index 00000000..1b0b0629 --- /dev/null +++ b/changes/198.general.rst @@ -0,0 +1 @@ +Warn if Step.run is called without configuring the step with parameters from CRDS. From 138fa635a1ebc6ce4f41cd4f2355d58e0f75da2d Mon Sep 17 00:00:00 2001 From: Brett Date: Fri, 11 Oct 2024 13:43:50 -0400 Subject: [PATCH 06/11] use class name in warning --- src/stpipe/step.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/stpipe/step.py b/src/stpipe/step.py index f134e694..2a12a313 100644 --- a/src/stpipe/step.py +++ b/src/stpipe/step.py @@ -50,7 +50,7 @@ class NoCRDSParametersWarning(UserWarning): def _warn_missing_crds_pars(step): warnings.warn( - f"Step({step}).run was called without first getting " + 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 " From 743a24ad090e690b6c13188be6aeacf07412e756 Mon Sep 17 00:00:00 2001 From: Brett Date: Fri, 11 Oct 2024 13:50:40 -0400 Subject: [PATCH 07/11] also flag configs from Pipeline fetched from CRDS --- src/stpipe/pipeline.py | 1 + 1 file changed, 1 insertion(+) 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 From c18ed3b170cfc04c80d417f6b6da0253aca2ee4a Mon Sep 17 00:00:00 2001 From: Brett Date: Fri, 11 Oct 2024 13:51:51 -0400 Subject: [PATCH 08/11] modify warning message --- src/stpipe/step.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/stpipe/step.py b/src/stpipe/step.py index 2a12a313..d2483d17 100644 --- a/src/stpipe/step.py +++ b/src/stpipe/step.py @@ -54,7 +54,7 @@ def _warn_missing_crds_pars(step): "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 in one method.", + "use Step.call which will create and use the instance.", NoCRDSParametersWarning, ) From e73522b4036b9e546c9034889d2e81d276851de3 Mon Sep 17 00:00:00 2001 From: Brett Date: Fri, 11 Oct 2024 14:16:22 -0400 Subject: [PATCH 09/11] add test that config merging preserves _from_crds --- tests/test_config_parser.py | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) 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 From 8bbc5194707d83267f364b8c1987ac3f0177e8fb Mon Sep 17 00:00:00 2001 From: Brett Date: Fri, 11 Oct 2024 14:56:00 -0400 Subject: [PATCH 10/11] change mocking to cover more code --- tests/test_step.py | 92 +++++++++++++++++++++++++--------------------- 1 file changed, 51 insertions(+), 41 deletions(-) diff --git a/tests/test_step.py b/tests/test_step.py index 13dde12a..0fa152c6 100644 --- a/tests/test_step.py +++ b/tests/test_step.py @@ -9,7 +9,7 @@ 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 NoCRDSParametersWarning, Step @@ -152,47 +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): - cfg = 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", + }, }, - }, - } - ) - cfg._from_crds = True - return cfg - - def mock_get_config_from_reference_step(dataset, disable=None): - cfg = cp.config_from_dict( - {"str1": "from crds", "str2": "from crds", "str3": "from crds"} - ) - cfg._from_crds = True - return cfg - - def mock_get_config_from_reference_list_arg_step(dataset, disable=None): - cfg = cp.config_from_dict({"rotation": "15", "pixel_scale": "0.85"}) - cfg._from_crds = True - return cfg - - 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) # ##### From 891234a30cd6397c862ee81939ed192bd45a60a6 Mon Sep 17 00:00:00 2001 From: Brett Date: Fri, 11 Oct 2024 14:57:49 -0400 Subject: [PATCH 11/11] fix fragment type --- changes/{198.general.rst => 198.feature.rst} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename changes/{198.general.rst => 198.feature.rst} (100%) diff --git a/changes/198.general.rst b/changes/198.feature.rst similarity index 100% rename from changes/198.general.rst rename to changes/198.feature.rst