Skip to content

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

Merged
DimaMolod merged 1 commit into
mainfrom
fix/af2-modelcif-subprocess
Oct 14, 2026
Merged

DimaMolod merged 1 commit into
mainfrom
fix/af2-modelcif-subprocess

Conversation

@DimaMolod

Copy link
Copy Markdown
Collaborator

Plan item 1 of 3: make AF2's ModelCIF subprocess reliable. ModelCIF stays best effort.

Problem

AF2 post-processing built a shell command containing the output path, ran it with python3, and treated any stderr output as failure. As a result:

  • a path with spaces or shell metacharacters broke the command, or ran parts of it as commands;
  • python3 could be a different interpreter from the one running AlphaPulldown;
  • the converter's ordinary logging was reported as an error, while its real exit code was ignored.

Change

A private helper, _convert_to_modelcif, runs the converter as [sys.executable, <converter path>, "--ap_output", <dir>] without a shell.

  • The converter's stdout and stderr are passed on as diagnostics.
  • A nonzero exit or a failure to start is reported as a warning.
  • Success is logged as "converter finished", not "all PDBs converted".
  • The order is unchanged: conversion, then storage cleanup, then fold completion. Conversion stays in its own process.
  • The standalone converter, its flags and its exit behaviour are unchanged.

Tests

  • Real subprocess, unusual path: a stand-in converter run in fold A+B; touch pwned $(touch pwned2) 'q' & \x``. It writes into exactly that directory, nothing else appears, and informational stderr does not count as failure.
  • Failures: a nonzero exit and a launch failure each log a warning, and cleanup and fold completion still run.
  • Disabled: with conversion off, no subprocess starts.
  • Same output as before: a new integration test converts the TEST fixture through the backend helper inside a directory full of shell metacharacters, and with the standalone script in a plain directory. Every .cif is byte-identical, and the zip archives have identical members; only their timestamps differ. Both images run this test.
  • Unit + integration, both release images: identical to main, plus the new tests. AF2 image: 979 passed. AF3 image: 1050 passed.
  • GPU: runs on main after this round is merged.

🤖 Generated with Claude Code

…hfully

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 <noreply@anthropic.com>
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@DimaMolod
DimaMolod merged commit 1aed0ae into main Oct 14, 2026
6 checks passed
@DimaMolod
DimaMolod deleted the fix/af2-modelcif-subprocess branch October 14, 2026 09:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant