From 4d17c8b5cf19226695f006a01b248bca8dad4aaa Mon Sep 17 00:00:00 2001 From: Ninjani Date: Fri, 9 Oct 2026 19:41:10 +0200 Subject: [PATCH] fix: resolve unavailable PDB minor revisions within the pinned major --- docs/dataset.md | 5 +- src/plinder/core/utils/io.py | 85 ++++++++++++++++++------ tests/core/test_source_mmcif.py | 114 +++++++++++++++++++++++++++++++- 3 files changed, 181 insertions(+), 23 deletions(-) diff --git a/docs/dataset.md b/docs/dataset.md index d38441e4..84f5d7e6 100644 --- a/docs/dataset.md +++ b/docs/dataset.md @@ -204,7 +204,10 @@ revision recorded in `entry_sources.parquet` is fetched from the [wwPDB versioned archive](https://www.wwpdb.org/ftp/pdb-versioned-ftp-site) and cached separately from the bulk release. Source PDB mmCIFs are therefore not included by `plinder_download`; `download_pdb_mmcifs()` can prefetch selected -source revisions for offline reconstruction. +source revisions for offline reconstruction. If the exact minor revision is +unavailable, PLINDER uses the latest minor revision within the same major +version. wwPDB increments the major version when coordinates, polymer sequences, +or chemical identities change. The reconstructed mmCIFs contain the atom, sequence, component, assembly, and bond information needed to read them as self-contained PDBx/mmCIF files. diff --git a/src/plinder/core/utils/io.py b/src/plinder/core/utils/io.py index 2c7c8b88..cd4fb667 100644 --- a/src/plinder/core/utils/io.py +++ b/src/plinder/core/utils/io.py @@ -175,6 +175,7 @@ def _validate_gzipped_mmcif( path: Path, *, expected_revision: PDBRevision | None = None, + allow_newer_minor: bool = False, ) -> None: """Reject corrupt/non-mmCIF data and revision mismatches.""" try: @@ -186,7 +187,12 @@ def _validate_gzipped_mmcif( raise ValueError(f"downloaded file is not an mmCIF: {path}") if expected_revision is not None: actual_revision = _mmcif_revision(path) - if actual_revision != expected_revision: + compatible = ( + allow_newer_minor + and actual_revision[0] == expected_revision[0] + and actual_revision[1] >= expected_revision[1] + ) + if actual_revision != expected_revision and not compatible: raise ValueError( f"source mmCIF revision mismatch for {path}: expected " f"{expected_revision[0]}.{expected_revision[1]}, found " @@ -200,6 +206,7 @@ def _download_versioned_mmcif( destination: Path, pdb_id: str, revision: PDBRevision, + allow_newer_minor: bool = False, ) -> None: """Download one mmCIF atomically, retrying only transient failures.""" major, minor = revision @@ -241,6 +248,7 @@ def _download_versioned_mmcif( _validate_gzipped_mmcif( temporary, expected_revision=revision, + allow_newer_minor=allow_newer_minor, ) temporary.replace(destination) return @@ -272,7 +280,12 @@ def get_pdb_mmcif( manifest_path: Path | str | None = None, base_url: str = WWPDB_VERSIONED_MMCIF_URL, ) -> Path: - """Return the release-pinned source mmCIF, downloading it when allowed. + """Return a source mmCIF from the release-pinned major revision. + + Prefer the exact pinned revision. If wwPDB no longer retains that minor + revision, use its latest minor revision within the same major version. + wwPDB requires a major increment for coordinate, sequence, and chemical + identity changes. Exact and same-major downloads have separate cache paths. Online calls populate the release-local cache atomically. In offline mode the function performs no network or filesystem writes: a valid cached file @@ -286,18 +299,23 @@ def get_pdb_mmcif( cache_dir=cache_dir, revision=revision, ) - cached = False - if destination.is_file(): - try: - _validate_gzipped_mmcif( - destination, - expected_revision=revision, - ) - cached = True - except ValueError: - cached = False - if cached and (not force_update or is_offline()): - return destination + major, minor = revision + major_destination = destination.with_name(f"{pdb_id}_v{major}.cif.gz") + for candidate, allow_newer_minor in ( + (destination, False), + (major_destination, True), + ): + if candidate.is_file(): + try: + _validate_gzipped_mmcif( + candidate, + expected_revision=revision, + allow_newer_minor=allow_newer_minor, + ) + except ValueError: + continue + if not force_update or is_offline(): + return candidate if is_offline(): raise FileNotFoundError( f"No valid cached source mmCIF for {pdb_id} at {destination}. " @@ -306,16 +324,41 @@ def get_pdb_mmcif( ) destination.parent.mkdir(parents=True, exist_ok=True) - major, minor = revision extended_id = _extended_pdb_id(pdb_id) filename = f"{extended_id}_xyz_v{major}-{minor}.cif.gz" url = f"{base_url.rstrip('/')}/{pdb_id[-3:-1]}/{extended_id}/{filename}" - _download_versioned_mmcif( - url=url, - destination=destination, - pdb_id=pdb_id, - revision=revision, - ) + try: + _download_versioned_mmcif( + url=url, + destination=destination, + pdb_id=pdb_id, + revision=revision, + ) + except FileNotFoundError: + LOG.warning( + "source mmCIF %s revision %d.%d is unavailable; trying the latest " + "minor revision of major version %d", + pdb_id, + major, + minor, + major, + ) + major_base_url = base_url.rstrip("/").replace( + "/data/entries", "/views/all/coordinates/mmcif" + ) + url = ( + f"{major_base_url}/{pdb_id[-3:-1]}/{extended_id}/" + f"{extended_id}_xyz_v{major}.cif.gz" + ) + _download_versioned_mmcif( + url=url, + destination=major_destination, + pdb_id=pdb_id, + revision=revision, + allow_newer_minor=True, + ) + destination = major_destination + major, minor = _mmcif_revision(destination) LOG.info( f"cached wwPDB source mmCIF {pdb_id} revision {major}.{minor} at {destination}" ) diff --git a/tests/core/test_source_mmcif.py b/tests/core/test_source_mmcif.py index 973423b6..0697ff29 100644 --- a/tests/core/test_source_mmcif.py +++ b/tests/core/test_source_mmcif.py @@ -2,6 +2,7 @@ # Distributed under the terms of the Apache License 2.0 from __future__ import annotations +import gzip from pathlib import Path import pandas as pd @@ -212,7 +213,12 @@ def get(*args, **kwargs): with pytest.raises(FileNotFoundError, match="revision 1.0"): core_io.get_pdb_mmcif("9zzz", manifest_path=source_manifest) - assert len(calls) == 1 + assert calls == [ + "https://files-versioned.wwpdb.org/pdb_versioned/data/entries/" + "zz/pdb_00009zzz/pdb_00009zzz_xyz_v1-0.cif.gz", + "https://files-versioned.wwpdb.org/pdb_versioned/views/all/coordinates/mmcif/" + "zz/pdb_00009zzz/pdb_00009zzz_xyz_v1.cif.gz", + ] assert not core_io.pdb_mmcif_cache_path( "9zzz", manifest_path=source_manifest, @@ -412,6 +418,112 @@ def test_source_mmcif_rejects_wrong_version(release_cache, cif_2y4i, monkeypatch ).exists() +def _revision_mmcif(major: int, minor: int) -> bytes: + return gzip.compress( + ( + "data_test\nloop_\n" + "_pdbx_audit_revision_history.data_content_type\n" + "_pdbx_audit_revision_history.major_revision\n" + "_pdbx_audit_revision_history.minor_revision\n" + f"'Structure model' {major} {minor}\n" + ).encode() + ) + + +@pytest.mark.parametrize("offline", [False, True]) +def test_source_mmcif_falls_back_to_same_major_and_reuses_cache( + release_cache, source_manifest, monkeypatch, caplog, offline +): + calls = [] + monkeypatch.setattr(core_io.LOG, "propagate", True) + + def get(url, **kwargs): + calls.append(url) + if len(calls) == 1: + return _Response(b"", status_code=404) + return _Response(_revision_mmcif(1, 6)) + + monkeypatch.setattr(core_io.requests, "get", get) + path = core_io.get_pdb_mmcif("2y4i", manifest_path=source_manifest) + + assert path == release_cache / "source_mmcifs" / "y4" / "2y4i_v1.cif.gz" + assert core_io._mmcif_revision(path) == (1, 6) + assert not core_io.pdb_mmcif_cache_path( + "2y4i", manifest_path=source_manifest + ).exists() + assert calls == [ + "https://files-versioned.wwpdb.org/pdb_versioned/data/entries/" + "y4/pdb_00002y4i/pdb_00002y4i_xyz_v1-5.cif.gz", + "https://files-versioned.wwpdb.org/pdb_versioned/views/all/coordinates/mmcif/" + "y4/pdb_00002y4i/pdb_00002y4i_xyz_v1.cif.gz", + ] + assert "trying the latest minor revision" in caplog.text + + monkeypatch.setattr( + core_io.requests, "get", lambda *args, **kwargs: pytest.fail("cache missed") + ) + monkeypatch.setenv("PLINDER_OFFLINE", str(offline)) + assert core_io.get_pdb_mmcif("2y4i", manifest_path=source_manifest) == path + if offline: + assert ( + core_io.get_pdb_mmcif( + "2y4i", manifest_path=source_manifest, force_update=True + ) + == path + ) + + +@pytest.mark.parametrize("actual_revision", [(2, 0), (1, 4)]) +def test_source_mmcif_rejects_incompatible_major_fallback( + release_cache, source_manifest, monkeypatch, actual_revision +): + responses = [ + _Response(b"", status_code=404), + _Response(_revision_mmcif(*actual_revision)), + ] + monkeypatch.setattr( + core_io.requests, "get", lambda *args, **kwargs: responses.pop(0) + ) + with pytest.raises(ValueError, match="revision mismatch"): + core_io.get_pdb_mmcif("2y4i", manifest_path=source_manifest) + assert not list((release_cache / "source_mmcifs").rglob("*.cif.gz")) + assert not list((release_cache / "source_mmcifs").rglob("*.tmp")) + + +def test_source_mmcif_force_update_refreshes_major_cache( + release_cache, source_manifest, monkeypatch +): + cache = release_cache / "source_mmcifs" / "y4" / "2y4i_v1.cif.gz" + cache.parent.mkdir(parents=True) + cache.write_bytes(_revision_mmcif(1, 6)) + responses = [ + _Response(b"", status_code=404), + _Response(_revision_mmcif(1, 7)), + ] + monkeypatch.setattr( + core_io.requests, "get", lambda *args, **kwargs: responses.pop(0) + ) + assert ( + core_io.get_pdb_mmcif("2y4i", manifest_path=source_manifest, force_update=True) + == cache + ) + assert core_io._mmcif_revision(cache) == (1, 7) + assert responses == [] + + +def test_source_mmcif_prefers_exact_cache_over_major_cache( + release_cache, source_manifest, monkeypatch +): + exact = core_io.pdb_mmcif_cache_path("2y4i", manifest_path=source_manifest) + exact.parent.mkdir(parents=True) + exact.write_bytes(_revision_mmcif(1, 5)) + exact.with_name("2y4i_v1.cif.gz").write_bytes(_revision_mmcif(1, 6)) + monkeypatch.setattr( + core_io.requests, "get", lambda *args, **kwargs: pytest.fail("cache missed") + ) + assert core_io.get_pdb_mmcif("2y4i", manifest_path=source_manifest) == exact + + @pytest.mark.parametrize("value", ["", "../../etc/passwd", "not-a-pdb"]) def test_source_mmcif_rejects_invalid_ids(value): with pytest.raises(ValueError, match="invalid PDB ID"):