From c3fe0043bcb7610faa22cb49db99e45ecf40d117 Mon Sep 17 00:00:00 2001 From: Franklyn Dunbar Date: Fri, 31 Jul 2026 09:11:18 -0800 Subject: [PATCH 1/2] Fix DataFrame truthiness crash in _validate_kinfile; surface pdp3 failures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _validate_kinfile used `if kin_df and not kin_df.empty`, which raises `ValueError: The truth value of a DataFrame is ambiguous` whenever an existing kin file parses into a DataFrame — so the cached-result check crashed instead of skipping re-processing. earthscope-sfg-workflows currently monkeypatches the method to work around this; with this fix that workaround can be dropped. _run_pdp3 now also logs an error when pdp3 exits non-zero (returncode + stderr tail) and when a run produces no kin output, so failed runs are visible in logs instead of only in the ProcessingResult fields. Regression tests cover the valid-kinfile path (raised before the fix), missing path, override=True, and unparseable files. Co-Authored-By: Claude Fable 5 --- .../src/pride_ppp/factories/processor.py | 17 +++++++++- packages/pride-ppp/tests/test_processor.py | 33 +++++++++++++++++++ 2 files changed, 49 insertions(+), 1 deletion(-) diff --git a/packages/pride-ppp/src/pride_ppp/factories/processor.py b/packages/pride-ppp/src/pride_ppp/factories/processor.py index 10a65b9..8b3ec0b 100644 --- a/packages/pride-ppp/src/pride_ppp/factories/processor.py +++ b/packages/pride-ppp/src/pride_ppp/factories/processor.py @@ -604,6 +604,15 @@ def _run_pdp3( for line in result.stderr.strip().splitlines(): logger.warning(line) + if result.returncode != 0: + stderr_tail = "\n".join(result.stderr.strip().splitlines()[-5:]) + logger.error( + "pdp3 exited with code %d for site %s: %s", + result.returncode, + site, + stderr_tail or "(no stderr)", + ) + # pdp3 writes outputs as e.g. "kin_2025254_ncc1" (no extension). # Search recursively in the working dir to find them. kin_files = list(Path(tmpdir).rglob(f"kin_*_{site.lower()}")) @@ -621,6 +630,12 @@ def _run_pdp3( shutil.move(str(src), str(dst)) kin_out = dst logger.info("Generated kin file %s", dst) + else: + logger.error( + "pdp3 produced no kin output for site %s (returncode %d)", + site, + result.returncode, + ) if res_files: src = res_files[0] @@ -679,7 +694,7 @@ def _validate_kinfile(self, kin_path: Path, override: bool = False) -> bool: return False # Attempt to parse the kinfile — only accept it if it yields data kin_df: pd.DataFrame | None = kin_to_kin_position_df(kin_path) - if kin_df and not kin_df.empty: + if kin_df is not None and not kin_df.empty: return True return False diff --git a/packages/pride-ppp/tests/test_processor.py b/packages/pride-ppp/tests/test_processor.py index d5d4688..f09eabb 100644 --- a/packages/pride-ppp/tests/test_processor.py +++ b/packages/pride-ppp/tests/test_processor.py @@ -21,6 +21,7 @@ pytest.skip(f"gnss-product-management not installed: {e}", allow_module_level=True) from pride_ppp.factories.processor import ( + PrideProcessor, _resolution_to_satellite_products, _resolution_to_table_dir, ) @@ -173,3 +174,35 @@ def test_resolution_to_table_dir_with_none_local_path() -> None: table_dir = _resolution_to_table_dir(resolution) assert table_dir is None + + +@pytest.fixture +def processor() -> PrideProcessor: + """A PrideProcessor without running __init__ — _validate_kinfile is self-free.""" + return object.__new__(PrideProcessor) + + +class TestValidateKinfile: + """Regression tests for PrideProcessor._validate_kinfile. + + The original implementation used `if kin_df and not kin_df.empty`, which + raises `ValueError: The truth value of a DataFrame is ambiguous` for any + kin file that parses into a DataFrame. + """ + + def test_valid_kinfile_returns_true(self, processor: PrideProcessor, kin_file: Path) -> None: + # Raised ValueError before the truthiness fix + assert processor._validate_kinfile(kin_file) is True + + def test_missing_path_returns_false(self, processor: PrideProcessor, tmp_path: Path) -> None: + assert processor._validate_kinfile(tmp_path / "kin_missing.kin") is False + + def test_override_skips_cache_check(self, processor: PrideProcessor, kin_file: Path) -> None: + assert processor._validate_kinfile(kin_file, override=True) is False + + def test_unparseable_kinfile_returns_false( + self, processor: PrideProcessor, tmp_path: Path + ) -> None: + garbage = tmp_path / "kin_2021220_bako.kin" + garbage.write_text("not a kin file\nno header here\n") + assert processor._validate_kinfile(garbage) is False From 21ff3b592910dd5edef0e5d2a869b982841cd638 Mon Sep 17 00:00:00 2001 From: Franklyn Dunbar Date: Fri, 31 Jul 2026 09:49:35 -0800 Subject: [PATCH 2/2] Halt processing when required products are missing; test _run_pdp3 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit process() logged 'Missing required products' and then ran pdp3 anyway with an incomplete config — a run guaranteed to fail downstream, and invisibly so for callers that only checked the log. It now raises MissingProductsError (exported from pride_ppp) unless a valid cached kin file short-circuits the run first. process_batch() stays batch-robust: files whose date is missing required products yield a failed ProcessingResult (returncode -1, stderr lists the missing specs) without dispatching pdp3, and other dates in the batch process normally. New tests cover _run_pdp3 via a fake pdp3 script on PATH (output moving/renaming, non-zero exit + no-kin-output error logging) and both missing-products paths, with a guard that pdp3 is never invoked for an unfulfilled date. Co-Authored-By: Claude Fable 5 --- packages/pride-ppp/src/pride_ppp/__init__.py | 8 +- .../src/pride_ppp/factories/processor.py | 47 +++++++- packages/pride-ppp/tests/test_processor.py | 104 ++++++++++++++++++ 3 files changed, 157 insertions(+), 2 deletions(-) diff --git a/packages/pride-ppp/src/pride_ppp/__init__.py b/packages/pride-ppp/src/pride_ppp/__init__.py index 6bd962f..9ddf87c 100644 --- a/packages/pride-ppp/src/pride_ppp/__init__.py +++ b/packages/pride-ppp/src/pride_ppp/__init__.py @@ -29,7 +29,12 @@ read_kin_data, validate_kin_file, ) -from .factories.processor import PrideProcessor, ProcessingMode, ProcessingResult +from .factories.processor import ( + MissingProductsError, + PrideProcessor, + ProcessingMode, + ProcessingResult, +) from .factories.rinex import merge_broadcast_files, rinex_get_time_range from .specifications.cli import PrideCLIConfig from .specifications.config import PRIDEPPPFileConfig, SatelliteProducts @@ -40,6 +45,7 @@ "PrideProcessor", "ProcessingMode", "ProcessingResult", + "MissingProductsError", # CLI / config "PrideCLIConfig", "PRIDEPPPFileConfig", diff --git a/packages/pride-ppp/src/pride_ppp/factories/processor.py b/packages/pride-ppp/src/pride_ppp/factories/processor.py index 8b3ec0b..1ae8c74 100644 --- a/packages/pride-ppp/src/pride_ppp/factories/processor.py +++ b/packages/pride-ppp/src/pride_ppp/factories/processor.py @@ -108,6 +108,20 @@ class ProcessingMode(enum.Enum): # --------------------------------------------------------------------------- +class MissingProductsError(RuntimeError): + """Raised when required GNSS products could not be resolved. + + pdp3 cannot produce a usable solution without the required products, + so processing halts instead of launching a run that is guaranteed to + fail downstream. + """ + + def __init__(self, date: datetime.date, missing: list[str]): + self.date = date + self.missing = missing + super().__init__(f"Missing required products for {date}: {missing}") + + @dataclass(frozen=True) class ProcessingResult: """Immutable result from a single RINEX → kinematic processing run. @@ -744,6 +758,9 @@ def process( Raises: FileNotFoundError: If *rinex* does not exist. + MissingProductsError: If required products could not be + resolved (unless a valid cached output short-circuits + the run first). """ rinex = Path(rinex) if not rinex.exists(): @@ -796,6 +813,7 @@ def process( if not resolution.all_required_fulfilled: missing = [r.spec for r in resolution.missing if r.required] logger.error("Missing required products: %s", missing) + raise MissingProductsError(start_date, missing) # --- 4. Write config -------------------------------------------------- # Config is persisted at pride_dir/{year}/{doy}/config_file so it can @@ -865,7 +883,10 @@ def process_batch( A ``ProcessingResult`` for each file as it completes. Cached results are yielded first, then pdp3 results in completion order. Wrap in ``list()`` if you need all - results at once. + results at once. Files whose date is missing required + products yield a failed result (``returncode == -1``, + ``success == False``) without running pdp3; other dates in + the batch are unaffected. Raises: ValueError: If *sites* length does not match *rinex_files*. @@ -936,6 +957,30 @@ def process_batch( ) continue + # Required products missing for this date: pdp3 would fail + # downstream, so yield a failed result instead of dispatching. + # Other dates in the batch still process normally. + if not resolutions[d].all_required_fulfilled: + missing = [r.spec for r in resolutions[d].missing if r.required] + logger.error( + "Missing required products for %s on %s: %s — skipping pdp3 run", + site, + d, + missing, + ) + yield ProcessingResult( + rinex_path=rinex, + site=site, + date=d, + kin_path=None, + res_path=None, + config_path=config_paths[d], + resolution=resolutions[d], + returncode=-1, + stderr=f"Missing required products: {missing}", + ) + continue + command = self._build_pdp_command( rinex=rinex, site=site, diff --git a/packages/pride-ppp/tests/test_processor.py b/packages/pride-ppp/tests/test_processor.py index f09eabb..ce24b33 100644 --- a/packages/pride-ppp/tests/test_processor.py +++ b/packages/pride-ppp/tests/test_processor.py @@ -7,6 +7,9 @@ from __future__ import annotations +import datetime +import logging +import os from pathlib import Path from tempfile import TemporaryDirectory @@ -20,7 +23,9 @@ except ImportError as e: pytest.skip(f"gnss-product-management not installed: {e}", allow_module_level=True) +from pride_ppp.factories import processor as processor_module from pride_ppp.factories.processor import ( + MissingProductsError, PrideProcessor, _resolution_to_satellite_products, _resolution_to_table_dir, @@ -206,3 +211,102 @@ def test_unparseable_kinfile_returns_false( garbage = tmp_path / "kin_2021220_bako.kin" garbage.write_text("not a kin file\nno header here\n") assert processor._validate_kinfile(garbage) is False + + +class TestRunPdp3: + """Subprocess handling in _run_pdp3, exercised via a fake pdp3 on PATH.""" + + @pytest.fixture + def fake_pdp3(self, tmp_path, monkeypatch): + """Return a factory that installs a fake pdp3 shell script on PATH.""" + bin_dir = tmp_path / "bin" + bin_dir.mkdir() + + def install(script_body: str) -> None: + pdp3 = bin_dir / "pdp3" + pdp3.write_text(f"#!/bin/sh\n{script_body}\n") + pdp3.chmod(0o755) + monkeypatch.setenv("PATH", f"{bin_dir}{os.pathsep}{os.environ['PATH']}") + + return install + + def test_outputs_moved_with_extensions(self, fake_pdp3, tmp_path: Path) -> None: + fake_pdp3("touch kin_2025254_ncc1 res_2025254_ncc1") + out = tmp_path / "out" + + kin, res, rc, _ = PrideProcessor._run_pdp3(command=["pdp3"], site="NCC1", output_dir=out) + + assert rc == 0 + assert kin == out / "kin_2025254_ncc1.kin" and kin.exists() + assert res == out / "res_2025254_ncc1.res" and res.exists() + + def test_nonzero_exit_and_missing_output_are_logged( + self, fake_pdp3, tmp_path: Path, caplog + ) -> None: + fake_pdp3("echo boom >&2; exit 2") + out = tmp_path / "out" + + with caplog.at_level(logging.ERROR, logger="pride_ppp.factories.processor"): + kin, res, rc, stderr = PrideProcessor._run_pdp3( + command=["pdp3"], site="NCC1", output_dir=out + ) + + assert rc == 2 + assert kin is None and res is None + assert "boom" in stderr + assert any("pdp3 exited with code 2" in m for m in caplog.messages) + assert any("produced no kin output" in m for m in caplog.messages) + + +def _unfulfilled_resolution() -> DependencyResolution: + return DependencyResolution( + spec_name="test", + resolved=[ + ResolvedDependency(spec="ORBIT", required=True, status="missing", local_path=None) + ], + ) + + +class TestMissingRequiredProducts: + """Processing must not launch pdp3 when required products are missing.""" + + def test_process_raises_missing_products_error(self, tmp_path: Path, monkeypatch) -> None: + proc = object.__new__(PrideProcessor) + proc._output_dir = tmp_path / "out" + monkeypatch.setattr(proc, "_resolve", lambda dt: _unfulfilled_resolution()) + + rinex = tmp_path / "test.rnx" + rinex.write_text("") + + with pytest.raises(MissingProductsError, match="ORBIT"): + proc.process(rinex, site="ncc1", date=datetime.date(2025, 9, 11)) + + def test_process_batch_yields_failed_result_without_running_pdp3( + self, tmp_path: Path, monkeypatch + ) -> None: + proc = object.__new__(PrideProcessor) + proc._output_dir = tmp_path / "out" + proc._pride_dir = tmp_path / "pride" + monkeypatch.setattr(proc, "_resolve", lambda dt: _unfulfilled_resolution()) + # Keep the orchestration test hermetic: no real RINEX headers, no + # installed PRIDE config template, and pdp3 must never be invoked. + start = datetime.datetime(2025, 9, 11, tzinfo=datetime.timezone.utc) + monkeypatch.setattr(processor_module, "rinex_get_time_range", lambda p: (start, start)) + monkeypatch.setattr(processor_module, "_write_config", lambda sp, td, dest: dest) + + def _fail(*args, **kwargs): + raise AssertionError("pdp3 must not run for unfulfilled dates") + + monkeypatch.setattr(proc, "_run_pdp3", _fail) + + rinex = tmp_path / "test.rnx" + rinex.write_text("") + + results = list(proc.process_batch([rinex], sites=["ncc1"])) + + assert len(results) == 1 + result = results[0] + assert result.returncode == -1 + assert result.success is False + assert "Missing required products" in result.stderr + assert "ORBIT" in result.stderr