fix: run AF2's ModelCIF conversion without a shell and report it truthfully

alphapulldown/folding_backend/alphafold2_backend.py

@@ -10,6 +10,7 @@
 import json
 import pickle
 import subprocess
+import sys
 import enum
 import typing
 from collections.abc import Mapping
@@ -602,6 +603,41 @@ def _record_inference_kernels(output_dir, model_name, model_runner) -> None:
         json.dump(existing, f, indent=2, sort_keys=True)
 
 
+# The converter runs in its own process: it exits on errors and keeps module-level state.
+_MODELCIF_CONVERTER = os.path.join(
+    os.path.dirname(os.path.dirname(os.path.abspath(__file__))),
+    "scripts",
+    "convert_to_modelcif.py",
+)
+
+
+def _convert_to_modelcif(output_dir) -> None:
+    """Convert the fold's models to ModelCIF. Best effort: a failure is reported, never
+    raised, so the fold is still cleaned up and recorded complete.
+
+    The converter runs with this interpreter and an argument list, not a shell, so any
+    output path works; what it prints is passed on as diagnostics, and only its exit
+    code says whether it failed.
+    """
+    command = [sys.executable, _MODELCIF_CONVERTER, "--ap_output", os.fspath(output_dir)]
+    logging.info(f"Converting {output_dir} to ModelCIF format...")
+    try:
+        result = subprocess.run(command, check=False, capture_output=True, text=True)
+    except OSError as exc:
+        logging.warning(f"Could not start the ModelCIF conversion of {output_dir}: {exc}")
+        return
+    for stream, text in (("stdout", result.stdout), ("stderr", result.stderr)):
+        if text and text.strip():
+            logging.info(f"ModelCIF converter {stream}:\n{text.rstrip()}")
+    if result.returncode != 0:
+        logging.warning(
+            f"ModelCIF conversion of {output_dir} failed with exit code "
+            f"{result.returncode}; its output is logged above."
+        )
+    else:
+        logging.info(f"ModelCIF converter finished for {output_dir}.")
+
+
 class AlphaFold2Backend(FoldingBackend):
     """
     A backend to perform structure prediction using AlphaFold.
@@ -1302,23 +1338,8 @@ def postprocess(
         #             template_file_path, ranked_output_path, temp_dir
         #         )
 
-        # Call convert_to_modelcif script
         if convert_to_modelcif:
-            parent_dir = os.path.dirname(os.path.dirname((os.path.abspath(__file__))))
-            logging.info(f"Converting {output_dir} to ModelCIF format...")
-            command = f"python3 {parent_dir}/scripts/convert_to_modelcif.py " \
-                      f"--ap_output {output_dir} "
-
-            result = subprocess.run(command,
-                                   check=False,
-                                   shell=True,
-                                   capture_output=True,
-                                   text=True)
-
-            if result.stderr:
-                logging.error(f"Error: {result.stderr}")
-            else:
-                logging.info("All PDBs converted to ModelCIF format.")
+            _convert_to_modelcif(output_dir)
         post_prediction_process(
            output_dir,
            compress_pickles=compress_pickles,

test/integration/test_modelcif.py

@@ -159,3 +159,41 @@ def test_missing_fasta_falls_back_to_structure_sequence(self):
             with open(out_cif, "r", encoding="ascii") as fh:
                 cif_txt = fh.read()
             self.assertIn("MESAIA", cif_txt)
+
+    def test_backend_conversion_matches_the_script_in_any_output_path(self):
+        """The AF2 backend's conversion, run in a directory full of shell metacharacters,
+        writes what the standalone script writes in a plain one."""
+        try:
+            from alphapulldown.folding_backend.alphafold2_backend import _convert_to_modelcif
+        except ImportError as exc:  # the AF2 backend needs AlphaFold 2
+            self.skipTest(f"AlphaFold 2 backend unavailable: {exc}")
+        with tempfile.TemporaryDirectory() as temp_dir:
+            # The fold directory's name becomes the ModelCIF entry ID, so both are TEST.
+            plain = Path(temp_dir, "plain", "TEST")
+            odd = Path(temp_dir, "runs A+B; touch pwned $(x) 'q' & `y`", "TEST")
+            for fold_dir in (plain, odd):
+                shutil.copytree(join(self.input_dir, "TEST"), fold_dir)
+
+            subprocess.run(
+                [sys.executable, self.script_path, "--ap_output", str(plain)],
+                check=True, capture_output=True, text=True,
+            )
+            _convert_to_modelcif(odd)
+
+            files = sorted(path.relative_to(plain) for path in plain.rglob("*") if path.is_file())
+            self.assertEqual(
+                files, sorted(path.relative_to(odd) for path in odd.rglob("*") if path.is_file())
+            )
+            self.assertIn(Path("ranked_0.cif"), files)
+            for relative in files:
+                if relative.suffix == ".zip":  # archives differ in their timestamps only
+                    with zipfile.ZipFile(plain / relative) as expected, zipfile.ZipFile(
+                        odd / relative
+                    ) as actual:
+                        self.assertEqual(expected.namelist(), actual.namelist())
+                        for name in expected.namelist():
+                            self.assertEqual(expected.read(name), actual.read(name), name)
+                else:
+                    self.assertEqual(
+                        (plain / relative).read_bytes(), (odd / relative).read_bytes(), relative
+                    )

test/unit/test_alphafold2_backend_helpers.py

@@ -1248,7 +1248,15 @@ def test_postprocess_ranks_models_relaxes_best_and_runs_cleanup(
     assert (tmp_path / "ranked_1.pdb").read_text(encoding="utf-8") == "PDB:low"
     assert (tmp_path / "relax_metrics.json").is_file()
     assert cleanup_calls
-    assert subprocess_calls
+    (command,), kwargs = subprocess_calls[0]
+    assert command == [
+        af2_backend_module.sys.executable,
+        af2_backend_module._MODELCIF_CONVERTER,
+        "--ap_output",
+        str(tmp_path),
+    ]
+    assert command[1].endswith("scripts/convert_to_modelcif.py")
+    assert "shell" not in kwargs
 
 
 def test_postprocess_relaxes_all_using_saved_unrelaxed_pdbs(
@@ -1340,8 +1348,8 @@ def test_postprocess_handles_monomers_without_relaxation_and_logs_modelcif_error
     )
     monkeypatch.setattr(
         af2_backend_module.logging,
-        "error",
-        lambda message: modelcif_errors.append(message),
+        "warning",
+        lambda message, *args: modelcif_errors.append(message % args if args else message),
     )
     monkeypatch.setattr(
         af2_backend_module.subprocess,
@@ -1383,7 +1391,9 @@ def test_postprocess_handles_monomers_without_relaxation_and_logs_modelcif_error
     assert not (tmp_path / "relaxed_modelA.pdb").exists()
     assert plot_calls == [0]
     assert cleanup_calls
-    assert modelcif_errors == ["Error: convert failed"]
+    assert [message for message in modelcif_errors if "ModelCIF" in message] == [
+        f"ModelCIF conversion of {tmp_path} failed with exit code 1; its output is logged above."
+    ]
 
 
 def test_postprocess_skips_best_relaxation_below_score_threshold(
@@ -2116,3 +2126,98 @@ def test_an_unreadable_json_left_by_an_older_version_is_ignored(
 
     assert af2_backend_module._read_from_json_if_exists(str(path)) == {}
     assert "Ignoring unreadable" in caplog.text
+
+
+_FAKE_MODELCIF_CONVERTER = """
+import argparse, pathlib, sys
+parser = argparse.ArgumentParser()
+parser.add_argument("--ap_output")
+output = pathlib.Path(parser.parse_args().ap_output)
+(output / "ranked_0.cif").write_text("data_converted\\n")
+print("INFO converting ranked_0.pdb", file=sys.stderr)
+"""
+
+
+def test_modelcif_conversion_works_in_any_output_path(
+    af2_backend_module, monkeypatch, tmp_path, caplog
+):
+    converter = tmp_path / "convert_to_modelcif.py"
+    converter.write_text(_FAKE_MODELCIF_CONVERTER)
+    monkeypatch.setattr(af2_backend_module, "_MODELCIF_CONVERTER", str(converter))
+    monkeypatch.chdir(tmp_path)
+    output_dir = tmp_path / "fold A+B; touch pwned $(touch pwned2) 'q' & `x`"
+    output_dir.mkdir()
+    caplog.set_level("INFO")
+
+    af2_backend_module._convert_to_modelcif(output_dir)
+
+    assert (output_dir / "ranked_0.cif").read_text() == "data_converted\n"
+    # No shell ran the command, so nothing else appeared.
+    assert sorted(path.name for path in tmp_path.iterdir()) == sorted(
+        [converter.name, output_dir.name]
+    )
+    # Informational stderr is passed on, not taken for a failure.
+    assert "INFO converting ranked_0.pdb" in caplog.text
+    assert "failed" not in caplog.text
+
+
+def _postprocess_monomer(af2_backend_module, monkeypatch, tmp_path, *, convert_to_modelcif):
+    cleanup_calls = []
+    monkeypatch.setattr(af2_backend_module, "plot_pae_from_matrix", lambda **kwargs: None)
+    monkeypatch.setattr(
+        af2_backend_module,
+        "post_prediction_process",
+        lambda *args, **kwargs: cleanup_calls.append(args),
+    )
+    af2_backend_module.start_fold_record(tmp_path, {"backend": "alphafold2"})
+    af2_backend_module.AlphaFold2Backend.postprocess(
+        prediction_results={
+            "modelA": {
+                "plddt": np.array([81.0, 82.0], dtype=np.float32),
+                "predicted_aligned_error": np.zeros((2, 2), dtype=np.float32),
+                "max_predicted_aligned_error": 31.0,
+                "ranking_confidence": 81.5,
+                "ptm": 0.4,
+                "seqs": ["AB"],
+                "unrelaxed_protein": SimpleNamespace(name="mono"),
+            }
+        },
+        multimeric_object=af2_backend_module.MonomericObject("single", "AB"),
+        output_dir=tmp_path,
+        features_directory="/features",
+        models_to_relax=af2_backend_module.ModelsToRelax.NONE,
+        convert_to_modelcif=convert_to_modelcif,
+    )
+    return cleanup_calls
+
+
+@pytest.mark.parametrize("failure", ["nonzero exit", "launch error"])
+def test_a_failed_modelcif_conversion_still_cleans_up_and_completes_the_fold(
+    af2_backend_module, monkeypatch, tmp_path, caplog, failure
+):
+    def run(command, **kwargs):
+        if failure == "launch error":
+            raise FileNotFoundError(command[0])
+        return SimpleNamespace(returncode=2, stdout="", stderr="Traceback: ValueError")
+
+    monkeypatch.setattr(af2_backend_module.subprocess, "run", run)
+
+    cleanup_calls = _postprocess_monomer(
+        af2_backend_module, monkeypatch, tmp_path, convert_to_modelcif=True
+    )
+
+    assert cleanup_calls
+    assert af2_backend_module.read_fold_record(tmp_path)["complete"] is True
+    expected = "failed with exit code 2" if failure == "nonzero exit" else "Could not start"
+    assert expected in caplog.text
+
+
+def test_no_modelcif_conversion_starts_when_it_is_off(
+    af2_backend_module, monkeypatch, tmp_path
+):
+    def run(*args, **kwargs):
+        raise AssertionError("the ModelCIF converter started")
+
+    monkeypatch.setattr(af2_backend_module.subprocess, "run", run)
+
+    assert _postprocess_monomer(af2_backend_module, monkeypatch, tmp_path, convert_to_modelcif=False)