refactor: simplify histogram processing by consolidating file handling and enhancing error reporting

Author: ElouenGinat

src/khisto/core/backend.py

@@ -7,8 +7,6 @@
 from __future__ import annotations
 
 import json
-import pathlib
-import shutil
 import subprocess
 import tempfile
 from dataclasses import dataclass, field
@@ -161,19 +159,22 @@ def _to_result(h: _HistogramPayload, **kwargs: Any) -> HistogramResult:
     )
 
 
-def _parse_file_type(file_path: pathlib.Path) -> None:
-    """Validate the exploratory JSON output file name."""
-    if file_path.suffix != ".json" or ".series" not in file_path.stem:
-        raise ValueError(f"Unrecognized histogram file name: {file_path.name}")
+def _format_runtime_error(
+    summary: str, cmd: list[str], details: str | None = None
+) -> str:
+    """Build a consistent runtime error message for khisto failures."""
+    message = f"{summary} while running: {' '.join(cmd)}"
+    if details:
+        message = f"{message}\n{details}"
+    return message
 
 
-def _process_histogram_files(temp_dir: str, base_name: str) -> list[HistogramResult]:
+def _process_histogram_file(
+    temp_output_file: tempfile._TemporaryFileWrapper[str],
+) -> list[HistogramResult]:
     """Process exploratory JSON generated by khisto CLI."""
-    output_file = pathlib.Path(temp_dir) / f"{base_name}.json"
-    _parse_file_type(output_file)
-
-    with output_file.open("r", encoding="utf-8") as f:
-        khisto_output: _KhistoOutput = _KhistoOutput.from_dict(json.load(f))
+    temp_output_file.seek(0)
+    khisto_output: _KhistoOutput = _KhistoOutput.from_dict(json.load(temp_output_file))
 
     histogram_series = khisto_output.histogramSeries
     best_idx = histogram_series.interpretableHistogramNumber - 1
@@ -210,7 +211,7 @@ def compute_histograms(x: np.ndarray) -> list[HistogramResult]:
     Raises
     ------
     RuntimeError
-        If khisto CLI execution fails.
+        If khisto CLI execution fails or returns invalid output.
     ValueError
         If input array is empty after filtering.
     """
@@ -220,37 +221,54 @@ def compute_histograms(x: np.ndarray) -> list[HistogramResult]:
     if len(x) == 0:
         raise ValueError("Input array is empty after filtering")
 
-    with tempfile.NamedTemporaryFile(
-        mode="wb", suffix=".bin", delete=False
-    ) as temp_input:
-        x.tofile(temp_input)
-        temp_input_path = temp_input.name
-
-    temp_dir = tempfile.mkdtemp(prefix="khisto_output_")
-    output_file = pathlib.Path(temp_dir) / "histogram.series.json"
-    cmd: list[str] = []
-
-    try:
-        cmd = [
-            str(KHISTO_BIN_DIR),
-            "-b",
-            "-e",
-            "-j",
-            temp_input_path,
-            str(output_file),
-        ]
-        subprocess.run(cmd, capture_output=True, text=True, check=True)
-        return _process_histogram_files(temp_dir, output_file.stem)
-
-    except subprocess.CalledProcessError as e:
-        stdout = e.stdout.strip()
-        stderr = e.stderr.strip()
-        details = "\n".join(part for part in (stdout, stderr) if part)
-        message = f"khisto failed with exit code {e.returncode} while running: {' '.join(cmd)}"
-        if details:
-            message = f"{message}\n{details}"
-        logger.error(message)
-        raise RuntimeError(message) from e
-    finally:
-        pathlib.Path(temp_input_path).unlink(missing_ok=True)
-        shutil.rmtree(temp_dir, ignore_errors=True)
+    with tempfile.NamedTemporaryFile(mode="wb", suffix=".bin") as temp_input_file:
+        x.tofile(temp_input_file)
+        with tempfile.NamedTemporaryFile(mode="r", suffix=".json") as temp_output_file:
+            cmd = [
+                str(KHISTO_BIN_DIR),
+                "-b",
+                "-e",
+                "-j",
+                temp_input_file.name,
+                temp_output_file.name,
+            ]
+            try:
+                subprocess.run(cmd, capture_output=True, text=True, check=True)
+            except subprocess.CalledProcessError as e:
+                stdout = e.stdout.strip()
+                stderr = e.stderr.strip()
+                details = "\n".join(part for part in (stdout, stderr) if part)
+                message = _format_runtime_error(
+                    f"khisto failed with exit code {e.returncode}",
+                    cmd,
+                    details or None,
+                )
+                logger.error(message)
+                raise RuntimeError(message) from e
+            except OSError as e:
+                message = _format_runtime_error(
+                    "khisto could not be started",
+                    cmd,
+                    str(e),
+                )
+                logger.error(message)
+                raise RuntimeError(message) from e
+
+            try:
+                return _process_histogram_file(temp_output_file)
+            except json.JSONDecodeError as e:
+                message = _format_runtime_error(
+                    "khisto produced invalid JSON output",
+                    cmd,
+                    str(e),
+                )
+                logger.error(message)
+                raise RuntimeError(message) from e
+            except (AttributeError, IndexError, TypeError, ValueError) as e:
+                message = _format_runtime_error(
+                    "khisto produced an invalid histogram payload",
+                    cmd,
+                    str(e),
+                )
+                logger.error(message)
+                raise RuntimeError(message) from e

tests/core/test_backend.py

@@ -13,8 +13,7 @@
 
 from khisto.core import compute_histograms
 from khisto.core.backend import (
-    _parse_file_type,
-    _process_histogram_files,
+    _process_histogram_file,
     HistogramResult,
 )
 
@@ -40,29 +39,6 @@ def _histogram_payload(
     }
 
 
-class TestParseFileType:
-    """Tests for _parse_file_type function."""
-
-    def test_series_json_file(self):
-        """Test validation of histogram.series.json file."""
-        _parse_file_type(pathlib.Path("histogram.series.json"))
-
-    def test_binary_series_json_file(self):
-        """Test validation of histogram.series.bin.json file."""
-        _parse_file_type(pathlib.Path("gaussian_histogram.series.bin.json"))
-
-    def test_invalid_file_name(self):
-        """Test that invalid file names raise ValueError."""
-        with pytest.raises(ValueError, match="Unrecognized histogram file name"):
-            _parse_file_type(pathlib.Path("histogram.json"))
-
-        with pytest.raises(ValueError, match="Unrecognized histogram file name"):
-            _parse_file_type(pathlib.Path("histogram.csv"))
-
-        with pytest.raises(ValueError, match="Unrecognized histogram file name"):
-            _parse_file_type(pathlib.Path("histogram.series.csv"))
-
-
 class TestHistogramResult:
     """Tests for HistogramResult dataclass."""
 
@@ -161,6 +137,37 @@ def fail_run(*args, **kwargs):
         assert "khisto stderr message" in message
         assert "-j" in message
 
+    def test_compute_histograms_reports_cli_launch_failure(self, monkeypatch):
+        """Test that khisto launch failures raise a descriptive runtime error."""
+
+        def fail_run(*args, **kwargs):
+            raise FileNotFoundError("[Errno 2] No such file or directory: 'khisto'")
+
+        monkeypatch.setattr("khisto.core.backend.subprocess.run", fail_run)
+
+        with pytest.raises(RuntimeError, match="could not be started") as excinfo:
+            compute_histograms(np.array([0.0, 1.0]))
+
+        message = str(excinfo.value)
+        assert "No such file or directory" in message
+        assert "-j" in message
+
+    def test_compute_histograms_reports_invalid_json_output(self, monkeypatch):
+        """Test that malformed JSON output raises a descriptive runtime error."""
+
+        def fake_run(cmd, **kwargs):
+            output_path = pathlib.Path(cmd[-1])
+            output_path.write_text("{not json", encoding="utf-8")
+
+        monkeypatch.setattr("khisto.core.backend.subprocess.run", fake_run)
+
+        with pytest.raises(RuntimeError, match="invalid JSON output") as excinfo:
+            compute_histograms(np.array([0.0, 1.0]))
+
+        message = str(excinfo.value)
+        assert "Expecting property name enclosed in double quotes" in message
+        assert "-j" in message
+
     def test_compute_histograms_writes_binary_input(self, monkeypatch):
         """Test that compute_histograms writes binary input and requests JSON output."""
 
@@ -178,15 +185,15 @@ def test_compute_histograms_writes_binary_input(self, monkeypatch):
         captured_values = None
 
         monkeypatch.setattr(
-            "khisto.core.backend._process_histogram_files",
-            lambda temp_dir, base_name: expected_result,
+            "khisto.core.backend._process_histogram_file",
+            lambda temp_output_file: expected_result,
         )
 
         def fake_run(cmd, **kwargs):
             nonlocal captured_values
             assert "-b" in cmd
             assert "-j" in cmd
-            assert cmd[-1].endswith("histogram.series.json")
+            assert cmd[-1].endswith(".json")
             input_path = pathlib.Path(cmd[-2])
             captured_values = np.fromfile(input_path, dtype=np.float64)
 
@@ -233,7 +240,8 @@ def test_series_selects_finest_interpretable_histogram(self, tmp_path):
         }
         self._write_json(tmp_path / "histogram.series.json", payload)
 
-        results = _process_histogram_files(str(tmp_path), "histogram.series")
+        with (tmp_path / "histogram.series.json").open("r", encoding="utf-8") as stream:
+            results = _process_histogram_file(stream)
 
         assert len(results) == 3
         assert [result.granularity for result in results] == [0, 2, 3]
@@ -270,7 +278,8 @@ def test_series_keeps_finest_histogram_when_all_interpretable(self, tmp_path):
         }
         self._write_json(tmp_path / "histogram.series.json", payload)
 
-        results = _process_histogram_files(str(tmp_path), "histogram.series")
+        with (tmp_path / "histogram.series.json").open("r", encoding="utf-8") as stream:
+            results = _process_histogram_file(stream)
 
         assert len(results) == 2
         assert [result.is_best for result in results] == [False, True]