From fa8576383df7b0d59fc205ed9d766aa22df24d12 Mon Sep 17 00:00:00 2001 From: James Davies Date: Thu, 2 Nov 2023 13:07:34 +0100 Subject: [PATCH 01/11] Filter out CRDS error logging for pars- reffiles --- src/stpipe/crds_client.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/stpipe/crds_client.py b/src/stpipe/crds_client.py index 2e48b517..f86f9995 100644 --- a/src/stpipe/crds_client.py +++ b/src/stpipe/crds_client.py @@ -8,6 +8,7 @@ general integration can be managed here. """ import re +import logging import crds from crds.core import config, crds_cache_locking, heavy_client, log @@ -49,6 +50,13 @@ def get_multiple_reference_paths(parameters, reference_file_types, observatory): raise TypeError("First argument must be a dict of parameters") log.set_log_time(True) + + def filter_pars_errors(record): + return not record.getMessage().startswith( + "Error determining best reference for 'pars-" + ) + log.prepend_crds_filter(filter_pars_errors) + return _get_refpaths(parameters, tuple(reference_file_types), observatory) From c64cc9e3cdcaf94f8fd4658720be441c9ee50cdc Mon Sep 17 00:00:00 2001 From: James Davies Date: Fri, 19 Jan 2024 16:26:18 +0100 Subject: [PATCH 02/11] Add unit test, but skip because of pytest bug or crds logging bug --- src/stpipe/crds_client.py | 6 ++---- tests/test_crds_client.py | 38 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 4 deletions(-) create mode 100644 tests/test_crds_client.py diff --git a/src/stpipe/crds_client.py b/src/stpipe/crds_client.py index f86f9995..7b1ab6c7 100644 --- a/src/stpipe/crds_client.py +++ b/src/stpipe/crds_client.py @@ -8,7 +8,6 @@ general integration can be managed here. """ import re -import logging import crds from crds.core import config, crds_cache_locking, heavy_client, log @@ -52,9 +51,8 @@ def get_multiple_reference_paths(parameters, reference_file_types, observatory): log.set_log_time(True) def filter_pars_errors(record): - return not record.getMessage().startswith( - "Error determining best reference for 'pars-" - ) + return not record.startswith("Error determining best reference for 'pars-") + log.prepend_crds_filter(filter_pars_errors) return _get_refpaths(parameters, tuple(reference_file_types), observatory) diff --git a/tests/test_crds_client.py b/tests/test_crds_client.py new file mode 100644 index 00000000..5bbebc56 --- /dev/null +++ b/tests/test_crds_client.py @@ -0,0 +1,38 @@ +import pytest + +from stpipe import crds_client + + +@pytest.mark.skip( + "CRDS logs via stderr and pytest can't capture it. " + "See https://github.com/pytest-dev/pytest/issues/5997" +) +def test_pars_log_filtering(caplog): + # A bogus pars- reffile will raise an exception in CRDS + with pytest.raises(Exception, match="Error determining best reference"): + crds_client.get_multiple_reference_paths( + parameters={ + "meta.instrument.detector": "NRCA1", + "meta.instrument.filter": "F140M", + "meta.instrument.name": "NIRCAM", + "meta.instrument.pupil": "CLEAR", + "meta.observation.date": "2012-04-22", + "meta.subarray.name": "FULL", + "meta.subarray.xsize": 2048, + "meta.subarray.xstart": 1, + "meta.subarray.ysize": 2048, + "meta.subarray.ystart": 1, + "meta.telescope": "JWST", + }, + reference_file_types=["pars-crunchyfrogstep"], + observatory="jwst", + ) + + # The following will always be true because of a bug in how pytest handles + # oddball logging setups, as used by crds. See issue + # https://github.com/pytest-dev/pytest/issues/5997 + # So don't rely on this test passing (currently) to be actually testing what + # you think it is. + assert ( + "Error determining best reference for 'pars-snowblindstep'" not in caplog.text + ) From d632ca469a8c3b0e7bdc639ff76cb642e7fb7365 Mon Sep 17 00:00:00 2001 From: James Davies Date: Mon, 22 Jan 2024 12:39:21 +0100 Subject: [PATCH 03/11] Fix filtering message for leading space --- src/stpipe/crds_client.py | 10 +++++++--- tests/test_crds_client.py | 5 +++-- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/src/stpipe/crds_client.py b/src/stpipe/crds_client.py index 7b1ab6c7..dad336b4 100644 --- a/src/stpipe/crds_client.py +++ b/src/stpipe/crds_client.py @@ -7,6 +7,7 @@ directly in modules other than this crds_client so that dependency order and general integration can be managed here. """ +import logging import re import crds @@ -50,10 +51,13 @@ def get_multiple_reference_paths(parameters, reference_file_types, observatory): log.set_log_time(True) - def filter_pars_errors(record): - return not record.startswith("Error determining best reference for 'pars-") + def parsfilter(record): + if not record.getMessage().strip().startswith( + "Error determining best reference for 'pars-" + ): + return True - log.prepend_crds_filter(filter_pars_errors) + logging.getLogger("CRDS").addFilter(parsfilter) return _get_refpaths(parameters, tuple(reference_file_types), observatory) diff --git a/tests/test_crds_client.py b/tests/test_crds_client.py index 5bbebc56..3bb3af6d 100644 --- a/tests/test_crds_client.py +++ b/tests/test_crds_client.py @@ -7,7 +7,7 @@ "CRDS logs via stderr and pytest can't capture it. " "See https://github.com/pytest-dev/pytest/issues/5997" ) -def test_pars_log_filtering(caplog): +def test_pars_log_filtering(capfd): # A bogus pars- reffile will raise an exception in CRDS with pytest.raises(Exception, match="Error determining best reference"): crds_client.get_multiple_reference_paths( @@ -33,6 +33,7 @@ def test_pars_log_filtering(caplog): # https://github.com/pytest-dev/pytest/issues/5997 # So don't rely on this test passing (currently) to be actually testing what # you think it is. + captured = capfd.readouterr() assert ( - "Error determining best reference for 'pars-snowblindstep'" not in caplog.text + "Error determining best reference for 'pars-crunchyfrogstep'" not in captured.err ) From d8aca7a0ce0ec4685ce058b1eb6007f087458326 Mon Sep 17 00:00:00 2001 From: James Davies Date: Mon, 22 Jan 2024 13:57:53 +0100 Subject: [PATCH 04/11] Make filter more lenient on formating --- src/stpipe/crds_client.py | 7 +++---- tests/test_crds_client.py | 7 +++---- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/src/stpipe/crds_client.py b/src/stpipe/crds_client.py index dad336b4..1f6e626e 100644 --- a/src/stpipe/crds_client.py +++ b/src/stpipe/crds_client.py @@ -52,10 +52,9 @@ def get_multiple_reference_paths(parameters, reference_file_types, observatory): log.set_log_time(True) def parsfilter(record): - if not record.getMessage().strip().startswith( - "Error determining best reference for 'pars-" - ): - return True + if "Error determining best reference for 'pars-" in record.getMessage(): + return False + return True logging.getLogger("CRDS").addFilter(parsfilter) diff --git a/tests/test_crds_client.py b/tests/test_crds_client.py index 3bb3af6d..67bd4ae5 100644 --- a/tests/test_crds_client.py +++ b/tests/test_crds_client.py @@ -4,10 +4,10 @@ @pytest.mark.skip( - "CRDS logs via stderr and pytest can't capture it. " + "CRDS logs in a non-standard way, and pytest can't capture it. " "See https://github.com/pytest-dev/pytest/issues/5997" ) -def test_pars_log_filtering(capfd): +def test_pars_log_filtering(caplog): # A bogus pars- reffile will raise an exception in CRDS with pytest.raises(Exception, match="Error determining best reference"): crds_client.get_multiple_reference_paths( @@ -33,7 +33,6 @@ def test_pars_log_filtering(capfd): # https://github.com/pytest-dev/pytest/issues/5997 # So don't rely on this test passing (currently) to be actually testing what # you think it is. - captured = capfd.readouterr() assert ( - "Error determining best reference for 'pars-crunchyfrogstep'" not in captured.err + "Error determining best reference for 'pars-crunchyfrogstep'" not in caplog.text ) From 95c967a14b10e210c9bfc946ba96186bc65244c9 Mon Sep 17 00:00:00 2001 From: James Davies Date: Mon, 22 Jan 2024 14:54:30 +0100 Subject: [PATCH 05/11] Update CHANGES.rst --- CHANGES.rst | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGES.rst b/CHANGES.rst index 9dcc074e..2cec0bfd 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -6,6 +6,7 @@ - Remove bundled ``configobj`` package in favor of the ``configobj`` package bundled into ``astropy``. [#122] - Fix ``datetime.utcnow()`` DeprecationWarning [#134] +- Filter out log messages about non-existing pars- reffiles [#135] 0.5.1 (2023-10-02) ================== From 11606cadc68b31ba5e556de7adc219b46608601b Mon Sep 17 00:00:00 2001 From: James Davies Date: Tue, 23 Jan 2024 00:02:05 +0100 Subject: [PATCH 06/11] Unskip unit test --- tests/test_crds_client.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/tests/test_crds_client.py b/tests/test_crds_client.py index 67bd4ae5..0fb46849 100644 --- a/tests/test_crds_client.py +++ b/tests/test_crds_client.py @@ -3,10 +3,6 @@ from stpipe import crds_client -@pytest.mark.skip( - "CRDS logs in a non-standard way, and pytest can't capture it. " - "See https://github.com/pytest-dev/pytest/issues/5997" -) def test_pars_log_filtering(caplog): # A bogus pars- reffile will raise an exception in CRDS with pytest.raises(Exception, match="Error determining best reference"): From 0b4caac40635ab808359a81a84adee27d3660469 Mon Sep 17 00:00:00 2001 From: James Davies Date: Tue, 23 Jan 2024 10:50:06 +0100 Subject: [PATCH 07/11] Mock patch in CRDS_SERVER_URL for the unit test --- tests/test_crds_client.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/test_crds_client.py b/tests/test_crds_client.py index 0fb46849..4abc053a 100644 --- a/tests/test_crds_client.py +++ b/tests/test_crds_client.py @@ -1,8 +1,12 @@ +import os +from unittest import mock + import pytest from stpipe import crds_client +@mock.patch.dict(os.environ, {"CRDS_SERVER_URL": "https://jwst-crds.stsci.edu"}) def test_pars_log_filtering(caplog): # A bogus pars- reffile will raise an exception in CRDS with pytest.raises(Exception, match="Error determining best reference"): From 7f5d65ccfefe86e89c6b11f6e8fab1b302a29208 Mon Sep 17 00:00:00 2001 From: James Davies Date: Tue, 23 Jan 2024 11:01:44 +0100 Subject: [PATCH 08/11] Use monkeypatch fixture; add env vars to workflows --- .github/workflows/ci.yml | 4 ++++ .github/workflows/ci_cron.yml | 4 ++++ tests/test_crds_client.py | 7 ++----- 3 files changed, 10 insertions(+), 5 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index c1584118..b0c5e5c0 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -21,6 +21,10 @@ jobs: test: uses: OpenAstronomy/github-actions-workflows/.github/workflows/tox.yml@v1 with: + setenv: | + CRDS_PATH: /tmp/data/crds_cache + CRDS_CLIENT_RETRY_COUNT: 3 + CRDS_CLIENT_RETRY_DELAY_SECONDS: 20 envs: | - linux: py39-oldestdeps-cov-xdist - linux: py310-xdist diff --git a/.github/workflows/ci_cron.yml b/.github/workflows/ci_cron.yml index 08ba3f68..a43291a9 100644 --- a/.github/workflows/ci_cron.yml +++ b/.github/workflows/ci_cron.yml @@ -23,6 +23,10 @@ jobs: uses: OpenAstronomy/github-actions-workflows/.github/workflows/tox.yml@v1 if: (github.repository == 'spacetelescope/stpipe' && (github.event_name == 'schedule' || github.event_name == 'push' || github.event_name == 'workflow_dispatch' || contains(github.event.pull_request.labels.*.name, 'Weekly CI'))) with: + setenv: | + CRDS_PATH: /tmp/data/crds_cache + CRDS_CLIENT_RETRY_COUNT: 3 + CRDS_CLIENT_RETRY_DELAY_SECONDS: 20 envs: | - macos: py39-xdist - macos: py310-xdist diff --git a/tests/test_crds_client.py b/tests/test_crds_client.py index 4abc053a..c2132ac7 100644 --- a/tests/test_crds_client.py +++ b/tests/test_crds_client.py @@ -1,13 +1,10 @@ -import os -from unittest import mock - import pytest from stpipe import crds_client -@mock.patch.dict(os.environ, {"CRDS_SERVER_URL": "https://jwst-crds.stsci.edu"}) -def test_pars_log_filtering(caplog): +def test_pars_log_filtering(caplog, monkeypatch): + monkeypatch.setenv("CRDS_SERVER_URL", "https://jwst-crds.stsci.edu") # A bogus pars- reffile will raise an exception in CRDS with pytest.raises(Exception, match="Error determining best reference"): crds_client.get_multiple_reference_paths( From d7ecab3a930dcfe9ad416a118a96b408612fdda5 Mon Sep 17 00:00:00 2001 From: James Davies Date: Tue, 23 Jan 2024 11:14:51 +0100 Subject: [PATCH 09/11] Set the CRDS server in tox --- tests/test_crds_client.py | 5 ++--- tox.ini | 2 +- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/tests/test_crds_client.py b/tests/test_crds_client.py index c2132ac7..6212b716 100644 --- a/tests/test_crds_client.py +++ b/tests/test_crds_client.py @@ -3,9 +3,8 @@ from stpipe import crds_client -def test_pars_log_filtering(caplog, monkeypatch): - monkeypatch.setenv("CRDS_SERVER_URL", "https://jwst-crds.stsci.edu") - # A bogus pars- reffile will raise an exception in CRDS +def test_missing_pars_log_filtering(caplog): + # A made-up pars- reffile will raise an exception in CRDS with pytest.raises(Exception, match="Error determining best reference"): crds_client.get_multiple_reference_paths( parameters={ diff --git a/tox.ini b/tox.ini index 02769f1d..a15bb1bd 100644 --- a/tox.ini +++ b/tox.ini @@ -54,7 +54,7 @@ pass_env = CI set_env = devdeps: PIP_EXTRA_INDEX_URL = https://pypi.anaconda.org/astropy/simple https://pypi.anaconda.org/scientific-python-nightly-wheels/simple - jwst: CRDS_SERVER_URL=https://jwst-crds.stsci.edu + !romancal: CRDS_SERVER_URL=https://jwst-crds.stsci.edu romancal: CRDS_SERVER_URL=https://roman-crds.stsci.edu package = !cov: wheel From 736c535e5c45b917a9ad6e6a3e29f1e6b5120ba6 Mon Sep 17 00:00:00 2001 From: James Davies Date: Tue, 23 Jan 2024 12:13:02 +0100 Subject: [PATCH 10/11] Fix test dependency on stdatamodels --- tests/test_crds_client.py | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/tests/test_crds_client.py b/tests/test_crds_client.py index 6212b716..3dbeddef 100644 --- a/tests/test_crds_client.py +++ b/tests/test_crds_client.py @@ -1,23 +1,18 @@ +from crds.core.exceptions import CrdsLookupError import pytest from stpipe import crds_client -def test_missing_pars_log_filtering(caplog): +def test_missing_pars_log_filtering(capfd): + # CRDS needs stdatamodels to handle JWST lookups + pytest.importorskip("stdatamodels.jwst") + # A made-up pars- reffile will raise an exception in CRDS - with pytest.raises(Exception, match="Error determining best reference"): + with pytest.raises(CrdsLookupError): crds_client.get_multiple_reference_paths( parameters={ - "meta.instrument.detector": "NRCA1", - "meta.instrument.filter": "F140M", "meta.instrument.name": "NIRCAM", - "meta.instrument.pupil": "CLEAR", - "meta.observation.date": "2012-04-22", - "meta.subarray.name": "FULL", - "meta.subarray.xsize": 2048, - "meta.subarray.xstart": 1, - "meta.subarray.ysize": 2048, - "meta.subarray.ystart": 1, "meta.telescope": "JWST", }, reference_file_types=["pars-crunchyfrogstep"], @@ -29,6 +24,7 @@ def test_missing_pars_log_filtering(caplog): # https://github.com/pytest-dev/pytest/issues/5997 # So don't rely on this test passing (currently) to be actually testing what # you think it is. + capture = capfd.readouterr() assert ( - "Error determining best reference for 'pars-crunchyfrogstep'" not in caplog.text + "Error determining best reference for 'pars-crunchyfrogstep'" not in capture.err ) From f495c20dc68db0260b13af74d624a1860dbe0d5b Mon Sep 17 00:00:00 2001 From: James Davies Date: Tue, 23 Jan 2024 12:17:19 +0100 Subject: [PATCH 11/11] Add downstreamdeps to tox and CI --- .github/workflows/ci.yml | 2 ++ tests/test_crds_client.py | 2 +- tox.ini | 4 +++- 3 files changed, 6 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index b0c5e5c0..09955c2c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -31,6 +31,8 @@ jobs: - macos: py311-xdist - linux: py311-cov-xdist coverage: 'codecov' + - linux: py311-downstreamdeps-cov-xdist + coverage: 'codecov' - linux: py312-xdist test_downstream: uses: OpenAstronomy/github-actions-workflows/.github/workflows/tox.yml@v1 diff --git a/tests/test_crds_client.py b/tests/test_crds_client.py index 3dbeddef..24d8aeba 100644 --- a/tests/test_crds_client.py +++ b/tests/test_crds_client.py @@ -1,5 +1,5 @@ -from crds.core.exceptions import CrdsLookupError import pytest +from crds.core.exceptions import CrdsLookupError from stpipe import crds_client diff --git a/tox.ini b/tox.ini index a15bb1bd..0d828bda 100644 --- a/tox.ini +++ b/tox.ini @@ -1,7 +1,7 @@ [tox] envlist = check-{style,security,build} - test{,-warnings,-cov,-xdist,-oldestdeps,-devdeps} + test{,-warnings,-cov,-xdist,-oldestdeps,-devdeps,-downstreamdeps} test-{jwst,romancal}-xdist build-{docs,dist} @@ -37,6 +37,7 @@ description = jwst: of JWST pipeline romancal: of Romancal pipeline oldestdeps: with the oldest supported version of key dependencies + downstreamdeps: with the downstream packages that depend on stpipe warnings: treating warnings as errors cov: with coverage xdist: using parallel processing @@ -49,6 +50,7 @@ deps = romancal: romancal[test] @ git+https://github.com/spacetelescope/romancal.git oldestdeps: minimum_dependencies devdeps: astropy>=0.0.dev0 + downstreamdeps: stdatamodels pass_env = CRDS_* CI