diff --git a/README.md b/README.md index 109bc76..40d0b6a 100644 --- a/README.md +++ b/README.md @@ -205,12 +205,14 @@ result = run_zeopp( analyses=["res", "sa"], radii_file="UFF.rad", num_samples=100000, + output_dir="zeopp_runs", ) print(result["results"]["res"]) # {'Di': 18.5, 'Df': 8.0, 'Dif': 10.9, ...} print(result["results"]["sa"]) # {'ASA': 4004.7, 'ASA_m2_g': 3918.3, ...} +print(result["output_dir"]) # unique persistent directory for this run -# Parse existing Zeo++ output files -result = get_output_data("output_dir/") +# Parse output files for one structure/run +result = get_output_data(result["output_dir"]) # Agent-free, runtime-selectable MLIP execution from matkit.mlip import ( diff --git a/docs/plans/code-review-followups.md b/docs/plans/code-review-followups.md new file mode 100644 index 0000000..cfb32eb --- /dev/null +++ b/docs/plans/code-review-followups.md @@ -0,0 +1,118 @@ +# Code review follow-ups + +Review date: 2026-09-05. Baseline revision: `5cd7a03` on +`docs/calculation-examples-review`. + +This file records findings 2-7 from the repository review. They are deferred: +none of the issues below are fixed by the accompanying Zeo++ stale-output +change. Each item should receive a focused implementation and regression test +before it is marked complete. + +## 2. gRASPA CLI uses the wrong default output columns + +**Severity:** High. **Status:** Deferred. + +The gRASPA setup templates and unified `AdsorptionRequest` default to PR-EOS, +but `matkit graspa analyze` calls `get_output_data()` without `eos=True`. The +legacy parser therefore reads the non-EOS column positions by default. In the +review fixture, the same result is interpreted as uptake 12 by the unified API +and uptake 7 by the CLI. + +**Impact:** A successful CLI command can report a valid-looking but incorrect +scientific result. + +**Recommended remediation:** Persist the chosen fugacity treatment with each +prepared simulation and make analysis consume it. Add an explicit CLI override +for manually produced output, rather than relying on a silent Boolean default. +Test default PR-EOS setup/analyze parity, an explicit non-EOS result, and +missing or contradictory metadata. + +## 3. Legacy CLI failures return exit status zero + +**Severity:** High. **Status:** Deferred. + +Many legacy commands in `src/matkit/cli.py` catch broad exceptions, print an +error to stderr, and return normally. Click consequently exits with status 0. +This affects gRASPA, pygRASPA, RASPA2, plotting, legacy MLIP, PACMOF2, and +Zeo++ command paths. + +**Impact:** Shell scripts, schedulers, and CI can treat failed work as +successful and continue with missing or invalid artifacts. + +**Recommended remediation:** Convert operational failures to `ClickException` +or an equivalent nonzero exit while retaining Click's exit 2 for invalid +arguments. Add parameterized CLI tests asserting exit 1 and stderr for each +command family, plus exit 0 for successful invocations. + +## 4. UMA multiprocessing can deadlock or lose results + +**Severity:** High. **Status:** Deferred. + +`run_opt_uma_batch()` joins every worker before draining `result_queue`, then +uses `Queue.empty()` to decide whether results remain. A worker can block while +flushing a full queue, preventing `join()` from completing, and +`multiprocessing.Queue.empty()` is not reliable for synchronization. + +**Impact:** Large batches can hang indefinitely or produce an incomplete +`results.jsonl` while appearing to have processed all jobs. + +**Recommended remediation:** Collect exactly one terminal result per submitted +job while workers are active, with explicit worker-exit and timeout handling; +then join and close queue resources. Synthesize failure records for jobs whose +workers terminate without reporting. Test queue backpressure, abrupt worker +exit, delayed delivery, and complete accounting for every input/model/run-type +combination. + +## 5. Batch interruption overwrites preparation failures + +**Severity:** Medium. **Status:** Deferred. + +`_interrupt_batch()` marks every run with `run.json` but no `result.json` as a +new batch interruption. That includes items which already reached a terminal +preparation failure, so their original stage, exception, and diagnostic are +replaced by the later batch-level error. + +**Impact:** Manifests lose the root cause of individual failures and make +recovery decisions less reliable. + +**Recommended remediation:** Treat terminal item failures as immutable. +Interruption should update only pending or actively executing items, while the +batch stores its own interruption failure separately. Test a preparation +failure followed by interruption and verify both item and batch diagnostics +survive inspection and restart. + +## 6. Sanitized CIF stems can collide + +**Severity:** Medium. **Status:** Deferred. + +gRASPA and pygRASPA replace dots with underscores when constructing batch +directories and copied CIF names. Distinct inputs such as `a.b.cif` and +`a_b.cif` therefore map to the same output tree and overwrite one another. + +**Impact:** Batch manifests can point multiple structures at one simulation, +silently attributing inputs or results to the wrong framework. + +**Recommended remediation:** Assign each input a collision-resistant identity, +such as a readable sanitized stem plus a stable short hash of the original +name/path, and reject any remaining duplicate destination before writing. +Preserve the original name in the manifest. Test colliding names in both +gRASPA and pygRASPA single-condition and sweep setup. + +## 7. RASPA3 conversion accepts malformed interaction sections + +**Severity:** Medium. **Status:** Deferred. + +`parse_raspa2_force_field()` slices at most the declared interaction count but +silently skips records with fewer than four fields. It does not verify that the +file contains the declared number of valid interactions before processing the +remaining section. + +**Impact:** Conversion can emit an incomplete RASPA3 force field while +reporting success, shifting later lines into the wrong logical section or +omitting atom interactions. + +**Recommended remediation:** Require exactly the declared number of interaction +lines and validate every record's name, type, and numeric parameters with a +line-specific error. Test truncated sections, malformed records, invalid +numbers, zero interactions, and a valid file whose parsed count exactly matches +the declaration. diff --git a/src/matkit/cli.py b/src/matkit/cli.py index c29e33b..1c66f89 100644 --- a/src/matkit/cli.py +++ b/src/matkit/cli.py @@ -1998,7 +1998,7 @@ def zeopp_cli(): "--outdir", default=None, type=click.Path(), - help="Output directory for result files.", + help="Parent for a unique Zeo++ result directory.", ) def zeopp_run( cif, diff --git a/src/matkit/types.py b/src/matkit/types.py index d72fb07..e1e5bd8 100644 --- a/src/matkit/types.py +++ b/src/matkit/types.py @@ -49,6 +49,7 @@ class ZeoppResult(TypedDict): success: bool results: dict error: Optional[str] + output_dir: Optional[str] class PACMOF2Result(TypedDict): diff --git a/src/matkit/zeopp/zeopp.py b/src/matkit/zeopp/zeopp.py index 7b3681b..3ff1908 100644 --- a/src/matkit/zeopp/zeopp.py +++ b/src/matkit/zeopp/zeopp.py @@ -73,6 +73,20 @@ def _output_path(directory: Path, stem: str, analysis: str) -> Path: return next((path for path in candidates if path.is_file()), candidates[0]) +def _output_inventory(directory: Path) -> dict[str, dict[str, Path]]: + """Group recognized Zeo++ outputs by their structure stem.""" + inventory: dict[str, dict[str, Path]] = {} + for analysis, suffixes in _OUTPUT_SUFFIXES.items(): + for suffix in suffixes: + for path in sorted(directory.glob(f"*{suffix}")): + if not path.is_file(): + continue + stem = path.name.removesuffix(suffix) + # Suffix order is significant for PSD: prefer .psd_histo. + inventory.setdefault(stem, {}).setdefault(analysis, path) + return inventory + + def _find_network_binary(network_path: str | None = None) -> str: """Locate the Zeo++ network binary. @@ -368,7 +382,8 @@ def get_output_data( """Parse pre-existing Zeo++ output files. Args: - output_path: Path to directory containing Zeo++ output files. + output_path: Path to one Zeo++ output file or a directory containing + output files for exactly one structure stem. analyses: Which analyses to parse. If None, auto-detects from available files (.res, .sa, .vol, .psd_histo, .psd, .chan). @@ -377,7 +392,8 @@ def get_output_data( Raises: FileNotFoundError: If output_path does not exist. - ValueError: If specified analyses are invalid or parsing fails. + ValueError: If specified analyses are invalid, directory outputs have + multiple structure stems, or parsing fails. """ outdir = Path(output_path) if not outdir.exists(): @@ -403,16 +419,21 @@ def get_output_data( results["success"] = True return results - # Directory mode: find output files - detect = analyses if analyses is not None else list(VALID_ANALYSES) + # Directory mode: all analyses must belong to one structure. Selecting + # matches independently can silently combine stale, unrelated results. + inventory = _output_inventory(outdir) + if len(inventory) > 1: + stems = ", ".join(sorted(inventory)) + raise ValueError( + "Multiple Zeo++ structure stems found in output directory: " + f"{stems}. Use a single-structure directory or a direct file path." + ) + + available = next(iter(inventory.values()), {}) + detect = analyses if analyses is not None else sorted(available) for analysis in detect: - matches = [ - path - for suffix in _OUTPUT_SUFFIXES[analysis] - for path in sorted(outdir.glob(f"*{suffix}")) - ] - if matches: - results[analysis] = _parse_output(matches[0], analysis) + if analysis in available: + results[analysis] = _parse_output(available[analysis], analysis) elif analyses is not None: raise ValueError(f"Missing requested Zeo++ analysis: {analysis}") @@ -450,12 +471,12 @@ def run_zeopp( Passed via -r flag. Defaults to the bundled UFF.rad if None. network_path: Explicit path to the network binary. - output_dir: Directory for output files. Uses a temp directory - if None. + output_dir: Parent directory for a unique, persistent run directory. + Uses an automatically deleted temporary directory if None. Returns: - Dict with 'success', 'results' (per-analysis sub-dicts), and - 'error' keys. + Dict with 'success', 'results' (per-analysis sub-dicts), 'error', + and 'output_dir'. The output directory is None for temporary runs. Raises: FileNotFoundError: If the CIF file, radii file, or network @@ -488,8 +509,11 @@ def run_zeopp( if use_temp: workdir = Path(tempfile.mkdtemp(prefix="zeopp_")) else: - workdir = Path(output_dir) - workdir.mkdir(parents=True, exist_ok=True) + output_parent = Path(output_dir).expanduser().resolve() + output_parent.mkdir(parents=True, exist_ok=True) + workdir = Path( + tempfile.mkdtemp(prefix=f"{cifpath.stem}-zeopp-", dir=output_parent) + ) try: # Copy CIF to working directory @@ -521,7 +545,12 @@ def run_zeopp( ) # Parse output files - result = {"success": False, "results": {}, "error": None} + result = { + "success": False, + "results": {}, + "error": None, + "output_dir": None if use_temp else str(workdir), + } stem = cif_dest.stem for analysis in analyses: out_file = _output_path(workdir, stem, analysis) @@ -578,7 +607,8 @@ def run_batch( Args: cif_dir: Directory containing CIF files. - output_dir: Directory for output files and results.jsonl. + output_dir: Parent directory for unique per-structure run directories + and results.jsonl. analyses: Analysis types to run. Defaults to ['res']. probe_radius: Probe molecule radius in Angstrom. chan_radius: Channel radius in Angstrom. @@ -628,6 +658,7 @@ def _process_one(cif_file: Path) -> dict: record = { "structure": stem, "status": "success", + "output_dir": result["output_dir"], } record.update(_flatten_results(result["results"])) return record diff --git a/tests/test_zeopp.py b/tests/test_zeopp.py index 25f412f..6f30bfc 100644 --- a/tests/test_zeopp.py +++ b/tests/test_zeopp.py @@ -5,6 +5,7 @@ from pathlib import Path from unittest.mock import patch, MagicMock import shutil +import tempfile from matkit.zeopp.zeopp import ( _find_network_binary, @@ -153,9 +154,7 @@ def test_parse_specific_analysis(self, zeopp_data_dir): def test_single_file_mode(self, zeopp_data_dir): """Should parse a single output file directly.""" - result = get_output_data( - str(zeopp_data_dir / "test_structure.res") - ) + result = get_output_data(str(zeopp_data_dir / "test_structure.res")) assert result["success"] is True assert "res" in result @@ -169,6 +168,21 @@ def test_invalid_analysis_raises(self, zeopp_data_dir): with pytest.raises(ValueError, match="Invalid analysis"): get_output_data(str(zeopp_data_dir), analyses=["invalid"]) + @pytest.mark.parametrize("analyses", [None, ["res"]]) + def test_mixed_structure_stems_raise( + self, analyses, zeopp_data_dir, tmp_path + ): + """Should never combine analyses from different structures.""" + shutil.copyfile( + zeopp_data_dir / "test_structure.res", tmp_path / "a.res" + ) + shutil.copyfile(zeopp_data_dir / "test_structure.sa", tmp_path / "b.sa") + + with pytest.raises( + ValueError, match=r"Multiple Zeo\+\+ structure stems" + ): + get_output_data(str(tmp_path), analyses=analyses) + class TestRunZeopp: """Tests for running the Zeo++ network binary.""" @@ -185,8 +199,9 @@ def test_invalid_analysis_raises(self, sample_cif): @patch("matkit.zeopp.zeopp._find_network_binary") @patch("matkit.zeopp.zeopp.subprocess.run") - def test_run_res_analysis(self, mock_run, mock_find, sample_cif, - zeopp_data_dir, tmp_path): + def test_run_res_analysis( + self, mock_run, mock_find, sample_cif, zeopp_data_dir, tmp_path + ): """Should run network binary and parse .res output.""" mock_find.return_value = "/usr/bin/network" @@ -210,37 +225,43 @@ def side_effect(cmd, **kwargs): output_dir=str(tmp_path / "out"), ) assert result["success"] is True + assert Path(result["output_dir"]).parent == tmp_path / "out" assert "res" in result["results"] assert result["results"]["res"]["Di"] == pytest.approx(18.569) @patch("matkit.zeopp.zeopp._find_network_binary") @patch("matkit.zeopp.zeopp.subprocess.run") - def test_run_includes_ha_flag(self, mock_run, mock_find, - sample_cif, tmp_path): + def test_run_includes_ha_flag( + self, mock_run, mock_find, sample_cif, tmp_path + ): """Should include -ha flag by default.""" mock_find.return_value = "/usr/bin/network" mock_run.side_effect = _successful_res - run_zeopp(sample_cif, analyses=["res"], - output_dir=str(tmp_path / "out")) + run_zeopp( + sample_cif, analyses=["res"], output_dir=str(tmp_path / "out") + ) cmd = mock_run.call_args[0][0] assert "-ha" in cmd @patch("matkit.zeopp.zeopp._find_network_binary") @patch("matkit.zeopp.zeopp.subprocess.run") def test_run_uses_bundled_radii_by_default( - self, mock_run, mock_find, sample_cif, tmp_path, + self, + mock_run, + mock_find, + sample_cif, + tmp_path, ): """Should use bundled UFF.rad when no radii file given.""" mock_find.return_value = "/usr/bin/network" mock_run.side_effect = _successful_res outdir = tmp_path / "out" - run_zeopp(sample_cif, analyses=["res"], - output_dir=str(outdir)) + result = run_zeopp(sample_cif, analyses=["res"], output_dir=str(outdir)) cmd = mock_run.call_args[0][0] assert "-r" in cmd - assert (outdir / "UFF.rad").exists() + assert (Path(result["output_dir"]) / "UFF.rad").exists() @patch("matkit.zeopp.zeopp._find_network_binary") @patch("matkit.zeopp.zeopp.subprocess.run") @@ -249,15 +270,20 @@ def test_run_no_ha_flag(self, mock_run, mock_find, sample_cif, tmp_path): mock_find.return_value = "/usr/bin/network" mock_run.side_effect = _successful_res - run_zeopp(sample_cif, analyses=["res"], ha=False, - output_dir=str(tmp_path / "out")) + run_zeopp( + sample_cif, + analyses=["res"], + ha=False, + output_dir=str(tmp_path / "out"), + ) cmd = mock_run.call_args[0][0] assert "-ha" not in cmd @patch("matkit.zeopp.zeopp._find_network_binary") @patch("matkit.zeopp.zeopp.subprocess.run") - def test_run_with_radii_file(self, mock_run, mock_find, sample_cif, - tmp_path): + def test_run_with_radii_file( + self, mock_run, mock_find, sample_cif, tmp_path + ): """Should include -r flag and copy radii file to workdir.""" mock_find.return_value = "/usr/bin/network" mock_run.side_effect = _successful_res @@ -267,14 +293,78 @@ def test_run_with_radii_file(self, mock_run, mock_find, sample_cif, rad_file.write_text("H 1.0\nC 1.7\n") outdir = tmp_path / "out" - run_zeopp(sample_cif, analyses=["res"], - radii_file=str(rad_file), - output_dir=str(outdir)) + result = run_zeopp( + sample_cif, + analyses=["res"], + radii_file=str(rad_file), + output_dir=str(outdir), + ) cmd = mock_run.call_args[0][0] assert "-r" in cmd # Radii file should be copied to workdir - assert (outdir / "UFF.rad").exists() + assert (Path(result["output_dir"]) / "UFF.rad").exists() + + @patch("matkit.zeopp.zeopp._find_network_binary") + @patch("matkit.zeopp.zeopp.subprocess.run") + def test_repeated_runs_use_unique_directories( + self, mock_run, mock_find, sample_cif, tmp_path + ): + """Should preserve each persistent run in a separate directory.""" + mock_find.return_value = "/usr/bin/network" + mock_run.side_effect = _successful_res + parent = tmp_path / "out" + + first = run_zeopp(sample_cif, output_dir=str(parent)) + second = run_zeopp(sample_cif, output_dir=str(parent)) + + first_dir = Path(first["output_dir"]) + second_dir = Path(second["output_dir"]) + assert first_dir != second_dir + assert first_dir.parent == second_dir.parent == parent + assert (first_dir / f"{Path(sample_cif).stem}.res").is_file() + assert (second_dir / f"{Path(sample_cif).stem}.res").is_file() + + @patch("matkit.zeopp.zeopp._find_network_binary") + @patch("matkit.zeopp.zeopp.subprocess.run") + def test_temporary_run_is_removed( + self, mock_run, mock_find, sample_cif, tmp_path, monkeypatch + ): + """Should not advertise an automatically deleted run directory.""" + mock_find.return_value = "/usr/bin/network" + mock_run.side_effect = _successful_res + original_mkdtemp = tempfile.mkdtemp + + def local_mkdtemp(*, prefix, dir=None): + return original_mkdtemp(prefix=prefix, dir=tmp_path) + + monkeypatch.setattr(tempfile, "mkdtemp", local_mkdtemp) + result = run_zeopp(sample_cif) + + assert result["success"] is True + assert result["output_dir"] is None + assert list(tmp_path.iterdir()) == [] + + @patch("matkit.zeopp.zeopp._find_network_binary") + @patch("matkit.zeopp.zeopp.subprocess.run") + def test_stale_parent_output_is_not_reused( + self, mock_run, mock_find, sample_cif, zeopp_data_dir, tmp_path + ): + """A zero-exit process must produce outputs in its own run directory.""" + mock_find.return_value = "/usr/bin/network" + mock_run.return_value = MagicMock(returncode=0, stderr="") + parent = tmp_path / "out" + parent.mkdir() + stale = parent / f"{Path(sample_cif).stem}.res" + shutil.copyfile(zeopp_data_dir / "test_structure.res", stale) + + with pytest.raises(ValueError, match="Missing requested"): + run_zeopp(sample_cif, output_dir=str(parent)) + + assert stale.is_file() + run_dirs = list(parent.glob(f"{Path(sample_cif).stem}-zeopp-*")) + assert len(run_dirs) == 1 + assert not (run_dirs[0] / stale.name).exists() def test_run_missing_radii_file_raises(self, sample_cif): """Should raise FileNotFoundError for missing radii file.""" @@ -283,14 +373,19 @@ def test_run_missing_radii_file_raises(self, sample_cif): @patch("matkit.zeopp.zeopp._find_network_binary") @patch("matkit.zeopp.zeopp.subprocess.run") - def test_run_failure_raises(self, mock_run, mock_find, sample_cif): + def test_run_failure_raises( + self, mock_run, mock_find, sample_cif, tmp_path + ): """Should raise ValueError when network binary fails.""" mock_find.return_value = "/usr/bin/network" mock_run.return_value = MagicMock( returncode=1, stderr="Error: bad input" ) with pytest.raises(ValueError, match="network failed"): - run_zeopp(sample_cif, output_dir="/tmp/zeopp_test_fail") + run_zeopp(sample_cif, output_dir=str(tmp_path / "failed")) + run_dirs = list((tmp_path / "failed").iterdir()) + assert len(run_dirs) == 1 + assert (run_dirs[0] / Path(sample_cif).name).is_file() class TestRunBatch: @@ -309,11 +404,14 @@ def test_writes_results_jsonl(self, mock_run, tmp_path): "success": True, "results": { "res": { - "Di": 10.0, "Df": 5.0, - "Dif": 7.0, "unit": "Angstrom", + "Di": 10.0, + "Df": 5.0, + "Dif": 7.0, + "unit": "Angstrom", }, }, "error": None, + "output_dir": str(tmp_path / "artifact"), } outdir = tmp_path / "out" @@ -334,6 +432,7 @@ def test_writes_results_jsonl(self, mock_run, tmp_path): for rec in records: assert rec["status"] == "success" + assert rec["output_dir"] == str(tmp_path / "artifact") assert rec["Di"] == 10.0 assert rec["Df"] == 5.0 @@ -352,11 +451,14 @@ def side_effect(cif, **kwargs): "success": True, "results": { "res": { - "Di": 10.0, "Df": 5.0, - "Dif": 7.0, "unit": "Angstrom", + "Di": 10.0, + "Df": 5.0, + "Dif": 7.0, + "unit": "Angstrom", }, }, "error": None, + "output_dir": str(tmp_path / "artifact"), } mock_run.side_effect = side_effect