Impress fixes - #59
Closed
drawadiagram wants to merge 22 commits into
Closed
Conversation
…eviews Pipeline changes (examples/small_molecule_binding/): - Rewrite all tasks from capture_stdio to local_task=True using asyncio.create_subprocess_shell; redirect stdout+stderr to per-task .log files (only surface on failure) - Add _gpu_env() helper to propagate Dragon Policy gpu_affinity via CUDA_VISIBLE_DEVICES - Add CIF.GZ→PDB gemmi conversion before MPNN (LigandMPNN only reads PDB format) - Make scripts_path overridable via kwargs - Add mpnn_run.py: numpy deprecated alias shim for LigandMPNN's bundled openfold af2.sh: - Detect colabfold_batch from active venv or pixi env; error if not found - Use COLABFOLD_CACHE_DIR env var for model weights; fallback to ~/.cache/colabfold - Pass --data flag (not --data-dir) and --num-models 1 for integration runs rfd3.sh: - Add --writable-tmpfs --bind /scratch:/scratch to apptainer exec (required on Delta) - Set PYTHONNOUSERSITE=1 to prevent host .local packages contaminating container impress_manager.py (bug fix): - kill_parent path appended bare pipeline instead of (pipeline, future) tuple → ValueError on unpack in cleanup loop; fixed to append (pipeline, pipeline_future) - Surface pipeline exceptions via future.exception() and log via pipeline_failed() logger.py: - Add pipeline_failed() method for error-level pipeline failure reporting Code reviews: - src/CODE_REVIEW.md: bugs and quality issues in impress src/ - examples/small_molecule_binding/CODE_REVIEW.md: pipeline + scripts review - examples/protien_binding_usecase/CODE_REVIEW.md: protein binding example review Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DbNxkFCwkCEEkHnN8xGH7Q
File was present in origin/main but absent from the ipdps_pdz_usecase base branch. Added with the same DragonExecutionBackendV3 → DragonExecutionBackend update applied to run_small_molecule_binding.py. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DbNxkFCwkCEEkHnN8xGH7Q
Conflict resolutions: - examples/protein_binding/protein_binding.py: took origin/main version (standalone Boltz-capable pipeline; moved out of src/impress/pipelines/) - examples/small_molecule_binding/run_nonadaptive.py: kept HEAD version (DragonExecutionBackend, not V3) - examples/protien_binding_usecase/run_protein_binding.py: accepted deletion (file lives in examples/protein_binding/ in main) - AU files (CODE_REVIEW.md, SKILL.md, delta_env_setup.sh, delta_gpu_run.sh): staged at their new location in examples/protein_binding/ per main's rename src/impress/pipelines/protein_binding.py deleted: concrete pipeline moved to examples/protein_binding/protein_binding.py in main branch. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DbNxkFCwkCEEkHnN8xGH7Q
- src/: remove all protein_binding.py findings (file moved to examples/protein_binding/ in main merge); add note at header - examples/small_molecule_binding/: add run_nonadaptive.py to scope; extend debug-logging finding to cover both runner files; add findings for commented-out backend and hardcoded pipeline index list - examples/protein_binding/: full rewrite — new header (directory renamed protein_binding/), expanded scope to cover protein_binding.py (Boltz), protein_binding_run.py, run_nonadaptive.py; add findings for hardcoded Anvil MPNN_PATH, hardcoded peptide sequence, concurrent Boltz gather, unguarded os.unlink, DragonExecutionBackendV3 rename in all three runner files, stale delta_gpu_run.sh paths; remove resolved typo-directory finding and fixed log-message typo finding Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DbNxkFCwkCEEkHnN8xGH7Q
…nding/, protein_binding/ Bugs fixed: - filter_shape.py: undefined sfxn → sfxn_clean in RosettaScripts XML; add argparse - packmin.py: rosetta.* → pyrosetta.rosetta.* imports; remove commented dead code - mpnn_wrapper.py (pb): #!/bin/sh shebang → #!/usr/bin/env python3 - mpnn_wrapper.py (pb): temperature type=int → type=float - impress_manager.py: WorkflowEngine shutdown in try/finally; flow=None in __init__; guard submit_new_pipelines - impress_pipeline.py: remove @AbstractMethod from finalize; add no-op base impl Potential issues: - protein_binding.py, small_molecule_binding.py: Anvil hardcoded paths → None + ValueError - protein_binding.py: os.unlink → Path.unlink(missing_ok=True) - protein_binding.py: Boltz gather gated with Semaphore(2) via _guarded_s4 - protein_binding_run.py, run_protein_binding.py, run_nonadaptive.py (pb): DragonExecutionBackendV3 → DragonExecutionBackend - run_protein_binding.py: adaptive_criteria async→def; CSV path uses pipeline.base_path - delta_gpu_run.sh: path typo protien→protein; LD_LIBRARY_PATH guarded; tasks-per-node=1; eval→source Code quality: - run_small_molecule_binding.py: DEBUG→INFO logging; remove unused ThreadPoolExecutor imports - run_nonadaptive.py (smb): DEBUG→INFO; remove commented-out LocalExecutionBackend import - protein_binding_run.py, run_nonadaptive.py (pb): DEBUG→INFO logging - filter_shape.py: remove redundant .close() in with blocks - filter_energy.py: remove unconditional print to stdout - fastrelax.sh: quote $0 in dirname - af2.sh: remove diagnostic echo lines - af2_multimer_reduced.sh: remove unused /tmp/work /tmp/upper - rfd3.sh: fix comment arg order ($4=diffusion_batch_size $5=scaffold_arg) - small_molecule_binding.py: remove dead fixed_residues_file comment - mpnn_wrapper.py (pb): chains==None → chains is None - impress_manager.py: log buffered count before clear (fix stale-0 bug) - logger.py: add min_level filtering; fix error()/critical() to use output_stream Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DbNxkFCwkCEEkHnN8xGH7Q
- delta_env_setup.sh (protein_binding): source lmod in non-interactive bash,
use `module load python` for Python detection, fix version formula
(major*100+minor), add matplotlib, export VIRTUAL_ENV before PyRosetta
installer, pin dragonhpc==0.14.1 (0.14.2 breaks on Delta runtime),
remove hardcoded bblj allocation from MINIFORGE and _scratch fallbacks
- delta_gpu_run.sh (protein_binding): point default IMPRESS_VENV to ve/impress_A
- small_molecule_binding/small_molecule_binding.py: prefix scaffold_arg
with + for Hydra append semantics (scaffoldguided.target_pdb)
AymenFJA
added
documentation
AymenFJA
requested changes
Sep 4, 2026
AymenFJA
left a comment
AymenFJA
left a comment
Collaborator
There was a problem hiding this comment.
Hey @mgoliyad thanks for the suggested works. I added many comments trying to understand why some core changes were made and aslo some might be just based on wrong Claude assumption or not enough context. Happy to meet and talk if needed.
| @@ -14,12 +14,6 @@ jobs: | |||
| fail-fast: false | |||
| matrix: | |||
| include: | |||
Collaborator
There was a problem hiding this comment.
Why did you remove the py39-310? Failing tests due to different Python versions requires ensuring that other packages that Impress uses in the pipeline also do not support or require the removed Python version. This also means you are no longer allowing Impress to support py39-310
There was a problem hiding this comment.
let me add it back - I made this update because of Dragon but you are right we have other backends too.
|
|
||
| source /anvil/projects/x-nairr240405/mason/LigandMPNN/.venv/bin/activate | ||
| #source /ocean/projects/dmr170002p/hooten/LigandMPNN/.venv/bin/activate | ||
| # Re-activate the IMPRESS venv inside Dragon tasks (VIRTUAL_ENV is exported by sbatch). |
Collaborator
There was a problem hiding this comment.
Please remove any Dragon reference inside IMPRESS code base
| module load gcc/11.2.0 | ||
| #source /anvil/scratch/x-mason/IMPRESS/.venv/bin/activate | ||
| source /ocean/projects/dmr170002p/hooten/IMPRESS/.venv/bin/activate | ||
| # Re-activate the IMPRESS venv inside Dragon tasks (VIRTUAL_ENV is exported by sbatch). |
Collaborator
There was a problem hiding this comment.
Please remove any Dragon reference inside IMPRESS code base
| #source /ocean/projects/dmr170002p/hooten/IMPRESS/.venv/bin/activate | ||
| source /anvil/projects/x-nairr240405/mason/IMPRESS/.venv/bin/activate | ||
| module load modtree/gpu | ||
| # Boltz requires Python <=3.12 (numpy<2.0 etc.) so it lives in its own env. |
Collaborator
There was a problem hiding this comment.
This is the case I talked about above: if a user with boltz py39-310 needs to use this package, they won't be able to and will need to bump the version.
| echo "── Step 4: rhapsody-py[dragon] (PyPI) ──" | ||
| "${PIP}" install -q "rhapsody-py[dragon,telemetry]" | ||
| # Pin dragonhpc to 0.14.1 — 0.14.2 added waitForKeys to DDRegisterClientResponse | ||
| # but the Delta system Dragon runtime has not been updated to match; 0.14.2 fails |
Collaborator
There was a problem hiding this comment.
I do not understand this. In general, the comment is wrong. DragonHPC does not exist on the Delta system. Consider removing this comment, as it might be misleading and directly reflect whatever changes are made in the code.
| # Pin dragonhpc to 0.14.1 — 0.14.2 added waitForKeys to DDRegisterClientResponse | ||
| # but the Delta system Dragon runtime has not been updated to match; 0.14.2 fails | ||
| # with AttributeError on every DDict operation on this cluster. | ||
| "${PIP}" install -q "dragonhpc==0.14.1" |
Collaborator
There was a problem hiding this comment.
What is the reason for installing Dragon HPC twice? Doing: pip install rhapsody-pyt[dragon] already installs and pins dragon to the env.
| #SBATCH --mem=220G | ||
| #SBATCH --time=02:00:00 | ||
| #SBATCH --job-name=impress_protein | ||
| #SBATCH --mail-user=mg2347@soe.rutgers.edu |
Collaborator
There was a problem hiding this comment.
leaked email, replace it with a placeholder, or remove the email entry from the script please.
|
|
||
| @self.auto_register_task(capture_stdio=True) | ||
| async def s4(target_fasta, task_description={"gpus_per_rank": 1}): # noqa: B006 | ||
| @self.auto_register_task(local_task=True) |
Collaborator
There was a problem hiding this comment.
Why did you switch this task to local_task please?
This task is computationally intensive and is expected to run on HPC resources. Setting local_task=True causes it to execute in the local/main execution context rather than being dispatched to the resource-aware execution environment, so it will not have access to the HPC resources allocated for the task.
Also, capture_stdio=True is already enabled to capture the task's stdout/stderr logs, so there is no need to launch a separate subprocess from within the decorated function just to capture its output.
| ) | ||
| self.logger.pipeline_log(f"s4 command for {target_fasta}: {cmd}") | ||
| return cmd | ||
| # s4_boltz.sh tees its own output to boltz_run.log in the output dir |
Collaborator
There was a problem hiding this comment.
This is the consequence of switching it to local; the code below does the same capture_stdio=True and not it assumes that the task is local.
| print(f"[TELEMETRY] mean task time: {dur['mean_seconds'] * 1000:.1f} ms") | ||
| await manager.telemetry.stop() | ||
|
|
||
| await manager.flow.shutdown() |
There was a problem hiding this comment.
it's moved here:
IMPRESS/src/impress/impress_manager.py
Lines 280 to 281 in 67e4a14
… and fastrelax/interface retries - Replace AlphaFold2/ColabFold fold validation with Boltz-2 protein+ligand co-folding, adding a real ligand-binding-confidence signal (ligand_iptm) AF2 never provided. - Replace RFD3's broken scaffoldguided.target_pdb scaffold feedback (a leftover from an older RFDiffusion version) with real RFD3 partial-diffusion guidance via the InputSpecification JSON. - Fix analysis_sequence() silently never comparing MPNN candidates: it only ever read a .fa file's first line (an undesigned template record), since real LigandMPNN writes every candidate into one file, not one per candidate. - Add a metric-agnostic short-circuit to the fastrelax/interface retry loops, escalating to a new backbone as soon as a retry stops improving instead of always exhausting a flat 5x cap on backbones that don't recover. - New scripts/derive_ligand_smiles.py (RDKit SMILES derivation from a ligand's .params + reference structure) and scripts/validate_run.py (post-run validation of a completed HPC run's output). - Update delta_env_setup.sh/delta_gpu_run.sh for Boltz-2 and other Delta HPC fixes; sync CLAUDE.md and README.md with the current pipeline behavior; add a scratch-archive .gitignore pattern. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V6G3TnSqr7sUDGeHxoqk7X
- protein_binding.py: _boltz_sem was scoped per-pipeline instance, allowing
4 pipelines × 2 = 8 concurrent Boltz per GPU; replace with module-level
_boltz_sem_per_gpu dict keyed by gpu_id, shared across all instances on
the same GPU
- protein_binding.py: _guarded_s4 checks for output PDB before propagating
a backend exception; same pattern already applied to s1 and s5
- s4_boltz.sh: remove tee pipe and PIPESTATUS check; capture_stdio=True
already captures all output, and the tee was masking Boltz exit code
- Remove backend-specific references from Python comments
- pull_foundry.sh, rfd3.sh: replace hardcoded personal scratch paths with
SCRATCH/${USER} env vars; rfd3.sh --bind made conditional on $SCRATCH
- fastrelax.sh, filter_shape.sh, packmin.sh: align venv activation to
VIRTUAL_ENV idiom (was ENV_DIR with a Delta-specific hardcoded default)
- delta_gpu_run.sh (both pipelines): document that IMPRESS logs go to
.out not .err
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DbNxkFCwkCEEkHnN8xGH7Q
Remote updates primarily pdz wf, local updates small molecule.
Job 21916521 (4 PROD pipelines, ~3h) crashed 4/4 on the first guided-backbone-feedback attempt: _normalize_ligand_id() only rewrote the Boltz co-folded PDB's ligand *residue* name, never its Boltz-assigned *atom* names, so select_exposed/select_buried (copied verbatim from the base RFD3 spec, keyed by canonical .params atom names) never matched and RFD3's validator rejected every guided run. Adds _infer_ligand_atom_mapping()/_normalize_ligand_atom_names(), which establish atom correspondence via element+connectivity graph isomorphism (rdkit) with a Kabsch-RMSD tie-break for symmetric atoms, plus a coverage check in _write_guided_rfd3_json() that fails safe (falls back to unguided diffusion) instead of reproducing the crash in a new form. Validated end-to-end against real job-21916521 crash artifacts and real Boltz output (scripts/check_ligand_atom_mapping.py, validate_run.py check 8) -- not synthetic fixtures. Adds rdkit as a new runtime dependency (delta_env_setup.sh Step 10). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V6G3TnSqr7sUDGeHxoqk7X
Real production run (job 21928556) showed 3/3 pipelines that reached guided backbone feedback crash on their very first attempt with "ValidationError: ... Length argument must not be provided during partial diffusion." _write_guided_rfd3_json() was copying the base spec's partial.length verbatim, but RFD3's validator rejects it whenever partial.input/partial_t are set (length is inferred from the input structure in that mode). Drop it when writing the guided spec, and update validate_run.py's regression check accordingly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ACJqhndYcV4FT2Fu1aLsTC
analysis_backbone() required n_clashing.ligand_clashes and a real helix/sheet fraction, but RFD3's partial-diffusion (guided-backbone- feedback) output never populates the former and returns NaN for the latter -- making backbone QC structurally unsatisfiable for every guided call. Since only a successful fold ever clears rfd3_input_pdb, this permanently deadlocked any pipeline that triggered guided mode. Confirmed against real fixture data from job 21933600: all 4 pipelines locked into this state, burning 2h24m-3h16m of their 4h run for zero progress. analysis_backbone() now detects guided-mode output (missing n_clashing.ligand_clashes) and substitutes the interresidue clash counts it does report, skipping the SS-fraction check there. A consecutive-failure counter in adaptive_decision() also falls back to unguided regeneration after 3 guided-mode backbone failures, so a future unforeseen RFD3 schema gap degrades to wasted attempts rather than a permanent deadlock. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WbmHky5fVnraBHjc1ZYHav
boltz's download_boltz2() only checks that mols/ exists, not that extraction finished, so a second concurrent task calling it while a first is still mid-extract sees the directory already there, skips extraction, and later fails with "CCD component <resname> not found!" on whatever wasn't extracted yet. This race previously killed 13/16 pipelines in a production run. Hold an flock for the whole check-and-repair, verify mols/ actually contains every file mols.tar lists (not just that the directory exists), and re-extract inside the lock via boltz's own download_boltz2() if incomplete. A .mols_complete marker avoids the O(45k) file-count recheck once warmed. Validated against the current production run: the first wave of 8 concurrent boltz predictions (pipelines p1-p16, pass 1) completed within a 17s window with zero CCD errors, zero failed examples, and zero tracebacks across all 40 boltz invocations so far. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RwqGRtxXcfNmHjK5qaFoTq
…line input_dir; add mpnn_ensemble_size config
Every STEP_RFD3 escalation path (sequence-retry exhaustion, packmin
bad-pack, fastrelax/interface non-improvement) previously left
rfd3_input_pdb pointing at the same guided seed, since only backbone-QC
failures and the fold decision cleared it. A guided backbone failing
downstream of backbone QC would loop RFD3 on a near-duplicate doomed
backbone indefinitely (confirmed via job 21945304: 17 consecutive
byte-identical guided_scaffold.pdb outputs). All escalation branches now
clear it, with pipeline_log() calls explaining each decision.
Also fixes impress_smallmol_bind() computing input_dir once from p1_in
and reusing it for every pipeline instance (p1-p4) instead of each
reading its own p{i}_in/.
Adds mpnn_ensemble_size as a RunConfig field (PROD=10, TEST=2) instead
of relying on the pipeline's internal default.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FdARFBtkwaq6Tf4VEHF1nF
Contributor
Author
|
This PR was too large and mixed three independent components, which made your review harder than it needed to be. I've split it into four stacked PRs and I'm closing this one. Nothing is lost — every change here is carried forward, dropped deliberately, or listed below as still open.
Merge order: #60 → (#61 ‖ #62) → #63. GitHub will retarget the bases automatically as each lands. @AymenFJA — your review, point by point
Three things in the split that aren't in this PRFound while splitting, all in the new PRs:
Also dropped as stale rather than carried forward: 🤖 Generated with Claude Code |
Contributor
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
remove dead files and hardcodings, add delta setup, misc errs