Skip to content

Updated the memory size estimate in contactmap - #1700

Open
amjjbonvin wants to merge 8 commits into
mainfrom
contactmap-size
Open

amjjbonvin wants to merge 8 commits into
mainfrom
contactmap-size

Conversation

@amjjbonvin

Copy link
Copy Markdown
Member

What does this PR do and why?

Fixes #1699. The [contactmap] memory gate was skipping the module on hosts
with plenty of memory — the antibody-antigen tutorial reported
needs 37.25Gb has 16.83Gb on a 32 GB laptop.

That 37.25 Gb figure never looked at the input files at all. Three separate
problems compounded in get_necessary_memory() / get_available_memory()
(src/haddock/libs/libutil.py):

  1. The file lookup always failed. The estimate called
    os.path.getsize(models[0].file_name), but PDBFile.file_name is a bare
    basename (libontology.py:49) while _run() executes with the cwd set to
    the contactmap step folder (modules/__init__.py:251) and the models live
    in the previous step folder. getsize() therefore raised on every real
    run, and a bare except Exception silently substituted a hard-coded 10000
    atom guess. That is where the number came from:
    10000² × 8 / 1024³ = 0.745 Gb, times ncores = 50, is exactly 37.25 Gb.

  2. The size→atoms heuristic was off by an order of magnitude.
    file_size // 10 assumes 10 bytes per atom. A PDB ATOM record is 81 bytes,
    and extract_pdb_dt() skips hydrogens, so the matrix only ever covers heavy
    atoms. Measured on tests/golden_data, the real figure is 99–158 bytes per
    heavy atom — a 10–16× overestimate of the atom count, which because memory
    is quadratic becomes a 100–250× overestimate of memory.

  3. The scaling assumed more parallelism than exists. The per-model cost was
    multiplied by ncores, but only min(ncores, n_jobs) distance matrices are
    ever live at once, and n_jobs is the number of clusters (or topX models
    when unclustered) — typically far below ncores.

What it does now

  • Models are addressed through rel_path instead of file_name, so the files
    are actually found.
  • The largest model is located by stat (cheap) — matching what the docstring
    always claimed but the code never did — and only that one file is parsed, by
    a new count_heavy_atoms() helper that mirrors extract_pdb_dt()'s filter
    exactly. Reading one file costs a few ms and removes the guesswork entirely.
  • The peak accounts for compute_distance_matrix() calling
    squareform(pdist(coords)): the condensed N(N-1)/2 array is still alive
    while squareform allocates the full N² matrix, so the peak is ~1.5× the
    final matrix. This was previously unaccounted for.
  • The requirement is scaled by min(ncores, len(contact_jobs)). The check
    moved to just after the job list is built, which is the first point where
    the job count is known.
  • Lookup failures are logged instead of being swallowed by a bare
    except Exception. That silence is why this went unnoticed.
  • get_available_memory() now reports total physical memory rather than
    psutil's available. On macOS available excludes the reclaimable page
    cache and compressed pages, so an idle 32 GB host reports 14–17 GB — meaning
    the old gate could flip between runs on identical input.

Effect

model before after
protprot (1780 heavy atoms) 37.25 Gb 36 MB
protlig (3041 heavy atoms) 37.25 Gb 106 MB
protdna (961 heavy atoms) 37.25 Gb 11 MB

(Every "before" is identical because the estimate never read the files.)
The denominator goes from a fluctuating 14–17 Gb to a stable 32 Gb.

The gate is kept rather than removed — the NxN matrix is still quadratic and
can still exhaust memory on a large enough system. It should now trip only when
that is genuinely about to happen.

How was this tested?

  • pytest tests/ — 1796 passed, 5 skipped
  • pytest integration_tests/ (contactmap and util subset) — 14 passed, 1 skipped
  • ruff check and ruff format --check clean on all changed files
    (the 5 pre-existing E721/F401 warnings in tests/test_module_contmap.py
    are unchanged from main)

The existing memory tests only asserted memory > 0, so they passed straight
through the broken fallback path and caught none of this. They now assert the
exact expected value against a reference formula. New tests:

  • test_count_heavy_atoms — the count must equal what extract_pdb_dt()
    actually puts into the matrix, so the two cannot drift apart.
  • test_get_necessary_memory — exact match for the largest of several
    models, plus a < 0.1 Gb bound that would have failed on the old code.
  • test_get_necessary_memory_uses_rel_path — reproduces the real two-step run
    directory layout, asserts that the bare file_name does not resolve from
    the step folder, and that the estimate is correct anyway. This is the
    regression test for the actual bug.

Verified manually against a simulated run directory as well: with cwd at
2_contactmap/ and the model in 1_prev/, the old lookup fails and the new
one returns 36.3 MB.

AI assistance

Diagnosis and implementation were done with Claude Code.

Checklist

  • Tests cover the new and/or changed code
  • Documentation updated if needed (also in the haddock3 user-manual
  • CHANGELOG.md updated for user-facing changes

Related issues

Closes #1699

Notes for reviewers

amjjbonvin and others added 3 commits September 23, 2026 14:15
The `contactmap` memory gate skipped the module on machines with ample
memory. Three problems compounded:

- `get_necessary_memory()` looked up `models[0].file_name`, a bare
  basename, while `_run()` executes with cwd set to the contactmap step
  folder and the models live in the previous one. `os.path.getsize()`
  therefore raised on every real run and a bare `except Exception`
  silently fell back to a 10000 atom guess -- 0.745 Gb, multiplied by
  `ncores`. The estimate never looked at the input files at all.
- The `file_size // 10` heuristic assumed 10 bytes per atom. A PDB ATOM
  record is 81 bytes and `extract_pdb_dt()` skips hydrogens, so the real
  figure is ~100-160 bytes per heavy atom. Memory is quadratic in the
  atom count, making this a 100-250x overestimate.
- The requirement was scaled by `ncores` even though only
  `min(ncores, n_jobs)` distance matrices are ever live at once.

Now the largest model is located by `stat` (via `rel_path`) and only that
one is parsed, by a new `count_heavy_atoms()` helper that mirrors the
filtering of `extract_pdb_dt()`. The peak accounts for `squareform()`
allocating the full matrix while the condensed `pdist()` array is still
alive. Lookup failures are logged instead of being swallowed.

`get_available_memory()` now reports total physical memory: psutil's
`available` excludes the reclaimable page cache, so an idle 32 Gb macOS
host reports 14-17 Gb and the gate flips between runs on identical input.

The existing tests only asserted `memory > 0` and so passed straight
through the broken fallback path. They now assert the exact expected
value, and a new test reproduces the two-step run directory layout that
triggered the bug.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@amjjbonvin amjjbonvin self-assigned this Sep 23, 2026
@amjjbonvin amjjbonvin added m|contactmap AI Changes authored or assisted by AI labels Sep 23, 2026
@amjjbonvin
amjjbonvin requested a review from VGPReys September 23, 2026 13:02
Comment thread src/haddock/libs/libutil.py Outdated
@amjjbonvin

amjjbonvin commented Sep 23, 2026 via email

Copy link
Copy Markdown
Member Author

Updated docstring and implementation of get_available_memory to return available memory instead of total memory.
Comment thread src/haddock/modules/analysis/contactmap/__init__.py Outdated
Comment thread src/haddock/libs/libutil.py
amjjbonvin and others added 2 commits September 25, 2026 13:24
When the memory estimate exceeded what the host had available, the module
skipped itself entirely and produced no contact maps at all. But the
shortfall is usually one of parallelism, not of feasibility: each job
holds one NxN distance matrix, so running fewer of them side by side
lowers the requirement proportionally.

The gate now computes how many jobs the available memory can feed and
hands that number to the execution engine as `ncores`. Only when a single
job does not fit is there no way to proceed, and that is the one case
left that still skips the module.

The reduction goes down to what actually fits rather than straight to 1,
so affordable parallelism is not thrown away: with 8 cores requested,
0.5Gb per job and 1.6Gb free, 3 jobs run in parallel.

The reduced count is passed to `get_engine()` through a copy of the
params rather than by mutating `self.params`. `params.cfg` is written
before `run()` so mutating would not have corrupted it, but keeping
`self.params` as the record of what the user asked for is preferable --
the reduction is a local scheduling decision, not a change of intent.

Tests cover the four outcomes (untouched, reduced to an intermediate
count, reduced to 1, skipped) plus the params-not-mutated guarantee.
Verified that the reduction reaches the `Scheduler` and that a real run
on the degraded path still produces its heatmaps, chordcharts and TSVs.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@amjjbonvin
amjjbonvin requested a review from VGPReys September 25, 2026 12:05

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI Changes authored or assisted by AI m|contactmap

Projects

None yet

Development

Successfully merging this pull request may close these issues.

The matrix size check in the contactmap module might overestimate the required memory

3 participants