From 9e95cc07ca8efbae24374444d82c000f16182de9 Mon Sep 17 00:00:00 2001 From: Dima Molodenskiy Date: Wed, 14 Oct 2026 10:22:25 +0200 Subject: [PATCH] fix: run AF2's ModelCIF conversion without a shell and report it truthfully AF2's post-processing built a shell command holding the output path, ran it with "python3" and took any stderr as failure. A path with spaces or shell metacharacters broke it (or ran parts of it as commands), python3 could be another interpreter than the one running AlphaPulldown, and the converter's ordinary logging was reported as an error while its real exit code was ignored. A private helper now runs the converter with sys.executable and an argument list, passes on its stdout and stderr as diagnostics, and reports a nonzero exit or a failure to start as a warning. Conversion stays best effort and in its own process, after which storage cleanup and fold completion run as before. The standalone converter is unchanged. Co-Authored-By: Claude Opus 5.5 --- .../folding_backend/alphafold2_backend.py | 53 +++++--- test/integration/test_modelcif.py | 38 ++++++ test/unit/test_alphafold2_backend_helpers.py | 113 +++++++++++++++++- 3 files changed, 184 insertions(+), 20 deletions(-) diff --git a/alphapulldown/folding_backend/alphafold2_backend.py b/alphapulldown/folding_backend/alphafold2_backend.py index e4b6c498..d20f9598 100644 --- a/alphapulldown/folding_backend/alphafold2_backend.py +++ b/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, diff --git a/test/integration/test_modelcif.py b/test/integration/test_modelcif.py index 5e1a55d8..5e57d4ab 100644 --- a/test/integration/test_modelcif.py +++ b/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 + ) diff --git a/test/unit/test_alphafold2_backend_helpers.py b/test/unit/test_alphafold2_backend_helpers.py index 892b2015..0a73e907 100644 --- a/test/unit/test_alphafold2_backend_helpers.py +++ b/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)