From 3d9ca8821e3884a36a8a28baaf51c05ac5fd4497 Mon Sep 17 00:00:00 2001 From: tsenoner Date: Wed, 19 Aug 2026 12:11:02 +0200 Subject: [PATCH 1/9] docs(openspec): propose the embed completeness contract Both backends currently disagree about what "done" means. The Biocentral backend fails when the .h5 does not cover what it was asked to embed; the local backend drops over-length and OOM sequences and exits 0 as long as one embedding landed. local.py imports its HDF5 helpers FROM biocentral.py, which is the tell: a shared layer already exists, it just has no home, so the parts that were not shared drifted. Proposes one shared contract (expected = requested - skipped), a skipped-vs- failed split so a capability limit is reported rather than fatal, a --max-length lever so a skip is actionable, and a directional FASTA coverage check. The coverage check is not cosmetic: compute_similarity zero-fills the matrix for a protein it cannot find in the FASTA, leaving its diagonal at 0, and base_processor only converts similarity to distance when the whole diagonal is 1. One uncovered protein inverts the entire MDS projection. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Achark1sp4iqreoQdBYc7y --- .../.openspec.yaml | 2 + .../embed-completeness-contract/design.md | 118 +++++++++++++ .../embed-completeness-contract/proposal.md | 75 +++++++++ .../specs/embed-completeness/spec.md | 157 ++++++++++++++++++ .../embed-completeness-contract/tasks.md | 68 ++++++++ 5 files changed, 420 insertions(+) create mode 100644 openspec/changes/embed-completeness-contract/.openspec.yaml create mode 100644 openspec/changes/embed-completeness-contract/design.md create mode 100644 openspec/changes/embed-completeness-contract/proposal.md create mode 100644 openspec/changes/embed-completeness-contract/specs/embed-completeness/spec.md create mode 100644 openspec/changes/embed-completeness-contract/tasks.md diff --git a/openspec/changes/embed-completeness-contract/.openspec.yaml b/openspec/changes/embed-completeness-contract/.openspec.yaml new file mode 100644 index 00000000..41c30bab --- /dev/null +++ b/openspec/changes/embed-completeness-contract/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-08-19 diff --git a/openspec/changes/embed-completeness-contract/design.md b/openspec/changes/embed-completeness-contract/design.md new file mode 100644 index 00000000..2a3953ca --- /dev/null +++ b/openspec/changes/embed-completeness-contract/design.md @@ -0,0 +1,118 @@ +## Context + +Two backends, two completeness rules. `biocentral.embed_sequences` raises when the +output HDF5 does not cover `remaining`; `local.embed_sequences` raises only when +the file is _entirely_ empty, and drops over-length and OOM sequences with a +`logger.warning` on the way. The tell that this was never designed is `local.py`'s +import line — it pulls `load_existing_ids` and `save_embeddings` **from +`biocentral.py`**. A shared layer already exists; it just lives inside one of its +two consumers, so the parts that were _not_ shared drifted. + +The local backend is the default on Colab (`resolve_default_backend()` returns +`"local"` on any CUDA runtime), which is exactly where the hosted app sends users +when Biocentral is down. + +## Goals / Non-Goals + +**Goals.** One completeness rule for both backends. Distinguish a capability limit +from a failure. Make every run state what it skipped. Catch a FASTA that does not +cover the embeddings before it corrupts a projection. + +**Non-Goals.** Making `base_processor`'s `np.allclose(np.diag(data), 1)` heuristic +robust (this change stops a partially-covered FASTA reaching it, nothing more). +Changing resume semantics for grouped third-party HDF5 files. Reconciling the +`sp|P12345|NAME` vs `P12345` key divergence between `embed` and `prepare` — the +coverage check normalises at comparison time instead. + +## Decisions + +### The shared layer is a new module, not `biocentral.py` + +`data/embedding/store.py` holds `load_existing_ids`, `save_embeddings`, +`validate_headers`, and `finish_run`. Both backends import from it; neither +imports from the other. `biocentral.py` re-exports the two moved helpers so +existing importers (`local.py`, `cli/annotate.py`, tests) keep working. + +_Alternative rejected:_ leave the helpers in `biocentral.py` and add `finish_run` +there. It preserves the exact inversion that caused the drift — the local backend +would depend on the remote backend for its definition of "done". + +### `expected = requested − skipped`, and skipped is a `{id: reason}` map + +A reason string, not a bare set, because the summary has to say _why_ — "3 skipped" +without "longer than max_length=2000 aa" is not actionable. The map is also what +lets one `finish_run` serve both backends: Biocentral passes an empty map. + +GPU OOM at batch size 1 counts as a capability limit rather than a failure. It is +the same class as over-length — _this machine cannot do this sequence_ — and +failing on it would make a T4 Colab runtime unable to complete any dataset with +one large protein. The reason string distinguishes it from a length skip, and the +"skipping everything is still a failure" rule keeps a wholly-skipped run from +passing. + +### The gate reads the file, never a counter + +`save_embeddings` skips identifiers already present, and h5py silently turns an +identifier containing `/` into a group, so a running total can claim more than the +file holds. `missing = set(expected) - load_existing_ids(h5_path)` is an exact +predicate on the artifact. + +_Alternative rejected:_ switch the gate to the loader's view (`_collect_datasets` +in `h5.py`, which walks one level of groups) so the check matches what `load_h5` +will later see. It is the more principled read, but it changes resume semantics — +a grouped third-party HDF5 would suddenly count as already-embedded — and it makes +the gate _pass_ on a corrupted `/` identifier, which is stored and reported under +its leaf name. That trade is only safe once nothing can write a `/` at all, which +is what the pre-flight rejection below establishes. Worth doing; not here. The two +views also disagree for identifiers nested two levels deep, which `_collect_datasets` +does not report at all. + +### `validate_headers` runs before any work, on both backends + +Moving it into the shared layer makes Biocentral fail up front instead of paying +for a full embedding run and then reporting a shortfall it cannot explain. This +changes an existing test that asserted the _post-hoc_ message for a `/` +identifier; the disk gate remains as the backstop and is tested directly by making +the writer under-deliver. + +### The summary goes to stderr at warning level + +The hosted prep service spawns the CLI with `stdout=DEVNULL, stderr=PIPE` and +keeps the last 50 stderr lines, and passes no `-v`, so `setup_logging` leaves the +threshold at WARNING. A summary written with `typer.echo`, or logged at INFO, is +invisible on the hosted path. `typer.echo` stays for the affirmative per-model +line only. + +The same service classifies failures by substring-matching stderr against +`_BIOCENTRAL_DOWN_PATTERNS`. The new messages avoid every one of those substrings, +so a coverage problem is never re-tagged as an embedding-service outage and routed +to Colab, which would not fix it. + +### The FASTA check is directional + +`h5 − fasta` is the dangerous direction and the only one reported. `fasta − h5` is +routine: a resumed embedding cache legitimately covers fewer proteins than the +FASTA it was built from, and the extra entries are simply unused downstream. + +Uncovered embeddings _fail_ under `-s/--similarity` and _warn_ otherwise. Failing +outright would break the legitimate case of deliberately projecting a subset; +warning alone would leave the MDS inversion shipping. This mirrors the shape the +codebase already uses for the multi-set case in `pipeline._validate_headers` — +raise on empty intersection, warn with a count on partial. + +Both sides are normalised through `parse_identifier` because `load_h5` uses raw +HDF5 keys while FASTA-derived identifiers are always parsed; comparing them raw +would report every protein uncovered for any user-supplied `sp|…`-keyed file. + +## Risks + +- **Previously-passing local runs now fail.** Any dataset where the local backend + dropped sequences _other_ than by length or OOM used to exit 0. That is the bug, + but it is a behaviour change for anyone who had adapted to it. +- **The FASTA check fires on `-i x.h5 -f y.fasta` runs that work today.** It warns + rather than fails except under `-s`, and `-f` is documented as similarity-only, + so the blast radius is bounded — but a stale `-f` path that users had been + passing harmlessly will now produce output. +- **`-f` is silently ignored for directory inputs** (`prepare` only attaches + `fasta_path` in the single-file branch), so the check does not fire there. Left + as-is; noted so the gap is not mistaken for coverage. diff --git a/openspec/changes/embed-completeness-contract/proposal.md b/openspec/changes/embed-completeness-contract/proposal.md new file mode 100644 index 00000000..2eecd31e --- /dev/null +++ b/openspec/changes/embed-completeness-contract/proposal.md @@ -0,0 +1,75 @@ +## Why + +`protspace embed` has two backends with two different ideas of what "done" means. + +The Biocentral backend now fails when the output HDF5 does not cover everything it +was asked to embed. The local backend does not: it drops every sequence over +`max_length` (2000 aa, not exposed on the CLI) and every sequence that OOMs at +batch size 1, logs a warning, and exits 0 as long as **one** embedding landed. A +90 %-complete `.h5` then projects, bundles and scores normally, and every +downstream number is computed on a silently truncated dataset. + +The divergence is not principled. `local.py` imports `load_existing_ids` and +`save_embeddings` **from `biocentral.py`** — there is already a shared HDF5 layer, +it just has no home, so each backend re-derived its own completeness rule. + +Separately, nothing anywhere compares an embedding set against the FASTA it came +from. That gap is not merely cosmetic: `compute_similarity` zero-fills the matrix +for any protein it cannot find in the FASTA, leaving that protein's **diagonal at +0**, and `base_processor` only converts similarity → distance when +`np.allclose(np.diag(data), 1)`. One uncovered protein makes that test false for +the whole matrix, so MDS consumes raw similarities as distances and the entire +projection inverts — near-identical proteins are placed furthest apart. + +## What Changes + +- Add `data/embedding/store.py`, a shared HDF5 layer owned by neither backend, + holding `load_existing_ids`, `save_embeddings`, `validate_headers`, and a single + `finish_run()` that both backends call to report and verify a run. +- Split **skipped** from **failed**. A documented capability limit (over + `--max-length`, GPU OOM at batch size 1) is skipped: named in the summary, exit 0. Anything else absent from the `.h5` fails. `expected = requested − skipped`. +- Report one summary per run on **stderr at warning level** — requested, embedded, + skipped, with the skipped identifiers named. Stdout is discarded by the hosted + prep service, so an affirmative `typer.echo` summary would be invisible there. +- Call `validate_headers` from **both** backends before any work begins. Only the + local backend rejects `/` today; Biocentral pays for a full embedding run and + then reports a shortfall it cannot explain. +- Expose `--max-length` on `embed` and `prepare`, so a skipped sequence is + actionable rather than a dead end. +- Compare embedding identifiers against the `-f/--fasta` set, normalising both + sides through `parse_identifier`. Directional: uncovered embeddings warn, and + **fail** when `-s/--similarity` is requested; a FASTA that is a superset is + routine (a resumed embedding cache) and is not reported. + +## Capabilities + +### New Capabilities + +- `embed-completeness`: when an embedding run is considered complete, which + sequences may be skipped rather than failed, how coverage against the source + FASTA is verified, and what each run reports. + +### Modified Capabilities + + + +## Impact + +- `apps/protspace/src/protspace/data/embedding/store.py` (new). +- `apps/protspace/src/protspace/data/embedding/biocentral.py`, + `local.py` — both call the shared contract; `load_existing_ids` / + `save_embeddings` re-exported from `biocentral.py` for compatibility. +- `apps/protspace/src/protspace/cli/embed.py`, + `apps/protspace/src/protspace/cli/prepare.py`, + `apps/protspace/src/protspace/cli/common_options.py` — `--max-length`. +- `apps/protspace/src/protspace/data/processors/pipeline.py` — FASTA coverage. +- No API, dependency, or bundle-schema changes. `apps/prep/` untouched. +- **Known limitation, deliberately out of scope:** the `np.allclose(np.diag(data), 1)` + heuristic in `base_processor.py` remains. This change stops a partially-covered + FASTA from reaching it; it does not make the heuristic itself robust. Tracked + separately. diff --git a/openspec/changes/embed-completeness-contract/specs/embed-completeness/spec.md b/openspec/changes/embed-completeness-contract/specs/embed-completeness/spec.md new file mode 100644 index 00000000..de12aefb --- /dev/null +++ b/openspec/changes/embed-completeness-contract/specs/embed-completeness/spec.md @@ -0,0 +1,157 @@ +## ADDED Requirements + +### Requirement: An incomplete embedding exits non-zero + +Embedding SHALL exit non-zero when a sequence it attempted is absent from the +output HDF5, on every backend. The rule is `expected = requested − skipped`, and +the check reads the HDF5 rather than a running total, because the writer skips +identifiers already present and h5py turns an identifier containing `/` into a +group — so a counter can claim sequences the file does not hold. + +#### Scenario: The embedder returns fewer sequences than requested + +- **WHEN** a batch response omits sequences that were requested +- **THEN** the run raises `ValueError` naming the output path, how many of the + outstanding sequences were embedded, and how many are still missing +- **AND** the partial HDF5 is kept, so a rerun embeds only what is missing +- **AND** no `model_name` attribute is written and no affirmative save line is printed + +#### Scenario: Nothing at all was embedded + +- **WHEN** no sequence reached the HDF5 +- **THEN** the run raises `ValueError` distinguishing this from a partial run +- **AND** no HDF5 is fabricated by the caller writing the `model_name` attribute + +#### Scenario: A complete run succeeds + +- **WHEN** every requested sequence is present in the HDF5 +- **THEN** the run returns the path and exits zero + +#### Scenario: The failure type is catchable by the CLI + +- **WHEN** any completeness failure is raised +- **THEN** it is a `ValueError`, which `cli/embed.py` and `cli/prepare.py` already + catch to render `ERROR: ` and exit 1, rather than a `RuntimeError`, + which would escape those handlers as a raw traceback + +### Requirement: A documented capability limit is skipped, not failed + +Embedding SHALL treat a sequence excluded by a documented capability limit as +skipped rather than failed, and SHALL exit zero when every remaining sequence +embedded. A capability limit is a sequence longer than the configured maximum +length, or one that exhausts GPU memory at batch size 1. + +#### Scenario: A sequence longer than the maximum is skipped + +- **WHEN** the local backend is asked to embed a sequence longer than `--max-length` +- **THEN** that sequence is excluded from the attempt and named as skipped +- **AND** the run exits zero provided the remaining sequences all embedded + +#### Scenario: A sequence that exhausts GPU memory is skipped + +- **WHEN** the local backend exhausts GPU memory for a single sequence at batch size 1 +- **THEN** that sequence is recorded as skipped with that reason and named in the summary +- **AND** the run exits zero provided the remaining sequences all embedded + +#### Scenario: Skipping everything is still a failure + +- **WHEN** every requested sequence was skipped, so the HDF5 gained nothing +- **THEN** the run raises `ValueError` rather than reporting success + +#### Scenario: The progress bar counts only what was written + +- **WHEN** a batch fails or a sequence is skipped +- **THEN** the progress bar does not advance for it, so a run that embedded + nothing cannot render identically to one that embedded everything + +### Requirement: Every run reports what it embedded and skipped + +Embedding SHALL report the requested, embedded, and skipped counts once per run on +stderr at warning level or above, naming the skipped identifiers with their reason +when any were skipped. Stderr rather than stdout because the hosted prep service +discards the subprocess's stdout and keeps only stderr; warning level or above +because the default verbosity shows nothing below it. + +#### Scenario: A run with skips names them + +- **WHEN** a run completes with one or more skipped sequences +- **THEN** the summary states how many were requested, embedded, and skipped +- **AND** it names the skipped identifiers, previewing the first few when there are many + +#### Scenario: The summary cannot be mistaken for a service outage + +- **WHEN** the summary or a completeness failure message is emitted +- **THEN** it contains none of the substrings the prep service matches to classify + a failure as `BIOCENTRAL_UNAVAILABLE`, so a coverage problem is never reported + to the user as an embedding-service outage + +### Requirement: Identifiers invalid for HDF5 are rejected before any work begins + +Embedding SHALL reject an identifier containing `/` on every backend, before +contacting the embedding service or loading a model. HDF5 treats `/` as a group +separator, so such an identifier silently becomes a group rather than a dataset +and the run cannot produce the requested key. + +#### Scenario: The remote backend rejects the identifier up front + +- **WHEN** the Biocentral backend is given an identifier containing `/` +- **THEN** it raises `ValueError` naming the offending identifiers before + submitting any batch, rather than embedding everything and then reporting an + unexplained shortfall + +#### Scenario: Both backends reject identically + +- **WHEN** either backend is given the same invalid identifier +- **THEN** the same error is raised, from the shared HDF5 layer + +### Requirement: The maximum sequence length is user-controllable + +The CLI SHALL expose the local backend's maximum sequence length as `--max-length` +on both `embed` and `prepare`, so that a skipped sequence is actionable. Without +it a user who is told a sequence was skipped has no way to embed it. + +#### Scenario: Raising the limit embeds a previously skipped sequence + +- **WHEN** a run skipped a sequence for exceeding the maximum length, and the user + reruns with a `--max-length` above that sequence's length +- **THEN** the sequence is attempted and, on success, written to the HDF5 + +#### Scenario: The option is rejected when not positive + +- **WHEN** `--max-length` is given a value below 1 +- **THEN** the CLI rejects it before loading a model + +### Requirement: Embeddings are checked against the supplied FASTA + +The pipeline SHALL compare embedding identifiers against the FASTA supplied with +`-f/--fasta`, normalising both sides through `parse_identifier`, and SHALL fail +when similarity is requested and any embedded protein is absent from that FASTA. +An uncovered protein leaves its similarity-matrix diagonal at zero, which defeats +the all-or-nothing diagonal test that triggers the similarity-to-distance +conversion, so a single uncovered protein inverts the entire MDS projection. + +#### Scenario: Uncovered embeddings block a similarity run + +- **WHEN** similarity is requested and one or more embedded proteins are absent + from the FASTA +- **THEN** the run fails with a message naming how many proteins are uncovered, + rather than producing an inverted projection + +#### Scenario: Uncovered embeddings warn without similarity + +- **WHEN** one or more embedded proteins are absent from the FASTA and similarity + is not requested +- **THEN** the run warns, naming the uncovered count, and continues + +#### Scenario: A FASTA covering more than the embeddings is not reported + +- **WHEN** the FASTA contains proteins that are absent from the embeddings +- **THEN** nothing is reported, because a resumed embedding cache legitimately + covers fewer proteins than the FASTA it was built from + +#### Scenario: Identifier styles are reconciled before comparing + +- **WHEN** the HDF5 keys and the FASTA headers use different identifier styles, + such as `sp|P12345|NAME` against `P12345` +- **THEN** both sides are normalised through `parse_identifier` before comparison, + so no protein is reported uncovered merely because of its key style diff --git a/openspec/changes/embed-completeness-contract/tasks.md b/openspec/changes/embed-completeness-contract/tasks.md new file mode 100644 index 00000000..c6ec2540 --- /dev/null +++ b/openspec/changes/embed-completeness-contract/tasks.md @@ -0,0 +1,68 @@ +## 1. Shared HDF5 layer + +- [ ] 1.1 Add `apps/protspace/src/protspace/data/embedding/store.py` with + `load_existing_ids`, `save_embeddings`, `validate_headers` (moved from + `biocentral.py` / `local.py`) and `finish_run()`. +- [ ] 1.2 Re-export `load_existing_ids` and `save_embeddings` from `biocentral.py` + so `local.py`, `cli/annotate.py` and existing tests keep importing them. +- [ ] 1.3 `finish_run()` computes `expected = requested − skipped`, raises when the + HDF5 does not cover `expected`, raises when nothing landed at all, and + otherwise emits one stderr summary at warning level. + +## 2. Both backends call the contract + +- [ ] 2.1 `biocentral.py`: call `validate_headers` before the first batch; replace + the inline gate with `finish_run(..., skipped={})`. +- [ ] 2.2 `local.py`: capture over-length and OOM-skipped identifiers into a + `{id: reason}` map instead of only logging them; call `finish_run` with it. +- [ ] 2.3 `local.py`: stop advancing the progress bar for an OOM-skipped sequence. +- [ ] 2.4 Verify neither backend imports from the other. + +## 3. `--max-length` + +- [ ] 3.1 Add `Opt_MaxLength` to `cli/common_options.py`, rejecting values below 1. +- [ ] 3.2 Wire it through `cli/embed.py` and `cli/prepare.py` into `LocalEmbedConfig`. +- [ ] 3.3 Reject it (or document it as ignored) for `--backend biocentral`, which + has no length cap. + +## 4. FASTA coverage + +- [ ] 4.1 Add the coverage check to `data/processors/pipeline.py`, normalising both + sides through `parse_identifier`. +- [ ] 4.2 Fail when similarity is requested and any embedded protein is uncovered; + warn otherwise; say nothing when the FASTA is a superset. +- [ ] 4.3 Place it so it runs before `compute_similarity`, not after. +- [ ] 4.4 Confirm the message contains no `_BIOCENTRAL_DOWN_PATTERNS` substring. + +## 5. Tests + +- [ ] 5.1 `test_local_embedder.py`: over-length skip exits 0 and is named; OOM skip + exits 0 and is named; a non-skip shortfall fails; skipping everything fails; + the bar does not advance for a skip. +- [ ] 5.2 `test_biocentral_embedder.py`: `/` now rejected up front; the disk gate + still catches a writer that under-delivers. +- [ ] 5.3 New coverage tests: uncovered + similarity fails, uncovered alone warns, + superset silent, mixed identifier styles reconcile. +- [ ] 5.4 `test_backend_switch.py`: `--max-length` wiring and rejection. +- [ ] 5.5 Both backends raise the same error for the same invalid identifier. + +## 6. Docs + verification + +- [ ] 6.1 Update `apps/protspace/docs/cli.md` and `README.md` for `--max-length` + and the completeness/coverage behaviour. +- [ ] 6.2 Update the CLI table in `apps/protspace/CLAUDE.md` if it drifts. +- [ ] 6.3 Check the Colab notebooks for anything relying on a partial run exiting 0. +- [ ] 6.4 `uv run ruff check src/ packages/ tests/` + `uv run ruff format --check src/ packages/ tests/`. +- [ ] 6.5 `uv run pytest -m "not slow"` from `apps/protspace`. +- [ ] 6.6 `pnpm format:check` for the openspec markdown. +- [ ] 6.7 `openspec validate embed-completeness-contract --type change --strict`. + +## 7. Follow-ups filed, not fixed here + +- [ ] 7.1 File an issue for the `np.allclose(np.diag(data), 1)` heuristic in + `base_processor.py` — thread an explicit `is_similarity` flag from + `compute_similarity` instead of inferring it from the diagonal. +- [ ] 7.2 File an issue for `load_existing_ids` vs the loader's grouped view: a + grouped third-party `.h5` re-embeds everything and writes flat duplicates. +- [ ] 7.3 File an issue for `protspace embed` writing raw `sp|…` keys where + `prepare` writes parsed accessions. From 52d8b288375e345af9579517c936fc8762932ba5 Mon Sep 17 00:00:00 2001 From: tsenoner Date: Wed, 19 Aug 2026 12:23:41 +0200 Subject: [PATCH 2/9] fix(embed): give both backends one completeness contract The two backends disagreed about what "done" means. Biocentral failed when the .h5 did not cover what it was asked to embed; local dropped every sequence over max_length and every sequence that OOMed at batch size 1, warned, and exited 0 as long as ONE embedding landed. A 90%-complete .h5 then projects, bundles and scores normally, and every downstream number is computed on a truncated dataset. The divergence was accretion, not design: local.py imported load_existing_ids and save_embeddings FROM biocentral.py, so a shared HDF5 layer already existed with no home, and the parts that were not shared drifted. Add data/embedding/store.py, owned by neither backend, holding that layer plus one finish_run() both call. The rule is expected = requested - skipped: - Skipped is a {id: reason} map for documented capability limits -- over the length cap, GPU OOM at batch size 1. Reported and named, never fatal: a capability limit is not a failure, and failing on it would make a T4 Colab runtime unable to complete any dataset containing one large protein. - Everything else absent from the file fails. Skipping everything is still a failure; an empty request is not (resume already covered it). - The gate reads the file, never a running total, because save_embeddings skips IDs already present. validate_headers moves into the shared layer and now runs on BOTH backends before any work. Only local rejected '/' before; Biocentral paid for a full embedding run and then reported a shortfall it could not explain. local.py also stops advancing the progress bar for an OOM-skipped sequence -- the bar counts what was written. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Achark1sp4iqreoQdBYc7y --- .../protspace/data/embedding/biocentral.py | 67 +++------ .../src/protspace/data/embedding/local.py | 75 ++++------ .../src/protspace/data/embedding/store.py | 135 ++++++++++++++++++ 3 files changed, 183 insertions(+), 94 deletions(-) create mode 100644 apps/protspace/src/protspace/data/embedding/store.py diff --git a/apps/protspace/src/protspace/data/embedding/biocentral.py b/apps/protspace/src/protspace/data/embedding/biocentral.py index 603700e2..e3c14dc2 100644 --- a/apps/protspace/src/protspace/data/embedding/biocentral.py +++ b/apps/protspace/src/protspace/data/embedding/biocentral.py @@ -8,11 +8,19 @@ from difflib import get_close_matches from pathlib import Path -import h5py import numpy as np from biocentral_api import BiocentralAPI, CommonEmbedder, batched from tqdm import tqdm +# Re-exported: the HDF5 layer moved to `store` so neither backend owns it, but +# local.py, cli/annotate.py and existing importers still reach it from here. +from protspace.data.embedding.store import ( # noqa: F401 + finish_run, + load_existing_ids, + save_embeddings, + validate_headers, +) + logger = logging.getLogger(__name__) # Short aliases → CommonEmbedder enum member names. @@ -106,27 +114,6 @@ def derive_h5_cache_path(fasta_path: Path, embedder: str) -> Path: return fasta_path.with_name(f"{fasta_path.stem}_{short}.h5") -# --------------------------------------------------------------------------- -# HDF5 helpers -# --------------------------------------------------------------------------- - - -def load_existing_ids(h5_path: Path) -> set[str]: - """Return the set of dataset keys already present in *h5_path*.""" - if not h5_path.exists(): - return set() - with h5py.File(h5_path, "r") as f: - return set(f.keys()) - - -def save_embeddings(h5_path: Path, embeddings: dict[str, np.ndarray]) -> None: - """Append embeddings to an HDF5 file (one dataset per protein).""" - with h5py.File(h5_path, "a") as f: - for protein_id, emb in embeddings.items(): - if protein_id not in f: - f.create_dataset(protein_id, data=emb.astype(np.float32)) - - # --------------------------------------------------------------------------- # Main orchestrator # --------------------------------------------------------------------------- @@ -150,6 +137,9 @@ def embed_sequences( """ cfg = embed_config or EmbedConfig() + # Reject HDF5-hostile identifiers before spending a single API call on them. + validate_headers(sequences) + # Resume: skip already-embedded sequences existing_ids = load_existing_ids(h5_path) if existing_ids: @@ -278,33 +268,12 @@ def embed_sequences( pbar.close() - # Gate on what landed in the file, not on a running total: save_embeddings skips - # IDs already present, and h5py turns an ID containing "/" into a group, so a - # counter can claim sequences the file does not hold. - missing = set(remaining) - load_existing_ids(h5_path) - if missing: - # ValueError, not RuntimeError: cli/embed.py and cli/prepare.py both catch - # (FileNotFoundError, ValueError) and render it as "ERROR: " + exit 1, - # whereas a RuntimeError escapes those handlers as a raw traceback. - embedded = len(remaining) - len(missing) - detail = ( - f"{embedded:,} of {len(remaining):,} outstanding sequence(s) embedded, " - f"{len(missing):,} still missing " - f"({failed_batches} of {len(api_batches)} batch(es) failed)" - ) - if embedded == 0: - raise ValueError( - f"No new embeddings were produced for {h5_path}: {detail}. " - f"Check the Biocentral server status and rerun." - ) - raise ValueError( - f"Embedding incomplete for {h5_path}: {detail}. " - f"Partial results were kept — rerun to embed only what is missing." - ) - - print(f"\nDone. Embedded {len(remaining):,} sequence(s).") - print(f"Output: {h5_path}") - return h5_path + return finish_run( + h5_path, + remaining, + context=f"{failed_batches} of {len(api_batches)} batch(es) failed", + retry_hint="Check the Biocentral server status and rerun.", + ) def probe_embedder( diff --git a/apps/protspace/src/protspace/data/embedding/local.py b/apps/protspace/src/protspace/data/embedding/local.py index cb9964e1..2b46a9b0 100644 --- a/apps/protspace/src/protspace/data/embedding/local.py +++ b/apps/protspace/src/protspace/data/embedding/local.py @@ -33,7 +33,12 @@ import numpy as np from tqdm import tqdm -from protspace.data.embedding.biocentral import load_existing_ids, save_embeddings +from protspace.data.embedding.store import ( + finish_run, + load_existing_ids, + save_embeddings, + validate_headers, +) logger = logging.getLogger(__name__) @@ -160,19 +165,6 @@ def pool_residues(hidden: np.ndarray, seq_len: int, mod_type: str) -> np.ndarray return residues.mean(axis=0).astype(np.float32) -def validate_headers(ids) -> None: - """Raise :class:`ValueError` if any identifier contains ``/``. - - HDF5 treats ``/`` as a group separator, so it cannot appear in a dataset - name. - """ - bad = [i for i in ids if "/" in i] - if bad: - raise ValueError( - "Header(s) contain '/', invalid for HDF5 dataset names: " + ", ".join(bad) - ) - - # --------------------------------------------------------------------------- # Model loading + inference (lazy torch/transformers) # --------------------------------------------------------------------------- @@ -286,21 +278,17 @@ def embed_sequences( if existing: logger.info("Resuming: %d already embedded in %s", len(existing), h5_path) - # Drop sequences over the length cap; on-device attention is O(L^2) and - # long sequences OOM. Unlike the Biocentral backend (no cap), name the - # skipped IDs so the completeness difference is visible, not a bare count. - too_long = {k for k, v in remaining.items() if len(v) > cfg.max_length} - if too_long: - preview = ", ".join(sorted(too_long)[:5]) - if len(too_long) > 5: - preview += ", ..." - logger.warning( - "Skipping %d sequence(s) longer than max_length=%d aa: %s", - len(too_long), - cfg.max_length, - preview, - ) - remaining = {k: v for k, v in remaining.items() if k not in too_long} + # Sequences a capability limit puts out of reach: recorded as skipped rather + # than failed, so they are reported and named but do not fail the run. + # On-device attention is O(L^2), so long sequences OOM. + outstanding = set(remaining) # this run's work, before any skips + skipped: dict[str, str] = { + k: f"longer than max_length={cfg.max_length} aa" + for k, v in remaining.items() + if len(v) > cfg.max_length + } + if skipped: + remaining = {k: v for k, v in remaining.items() if k not in skipped} if remaining: import torch @@ -333,13 +321,11 @@ def embed_sequences( bs = max(1, bs // 2) logger.warning("GPU OOM — reducing batch size to %d", bs) else: - logger.warning( - "Skipping %s (len=%d, OOM at batch_size=1)", - batch_ids[0], - len(remaining[batch_ids[0]]), - ) + # Same class as the length cap: this machine cannot do + # this sequence. The bar deliberately does not advance — + # it counts what was written, not what was attempted. + skipped[batch_ids[0]] = "GPU OOM at batch size 1" i += 1 - pbar.update(1) pbar.close() finally: # Release GPU memory. The `del` must run in this frame (not a @@ -352,13 +338,12 @@ def embed_sequences( if torch.cuda.is_available(): torch.cuda.empty_cache() - # A finished .h5 must hold at least one embedding, else downstream load_h5 - # fails on an empty file. Fail loudly instead of returning a path to an - # empty/absent file (every sequence too long, or all OOM-skipped). - if not load_existing_ids(h5_path): - raise ValueError( - f"No embeddings were produced for {h5_path}: all {len(sequences)} " - f"sequence(s) were skipped (longer than max_length={cfg.max_length} " - f"aa, or GPU OOM at batch size 1)." - ) - return h5_path + return finish_run( + h5_path, + outstanding, + skipped=skipped, + retry_hint=( + f"Raise --max-length (currently {cfg.max_length}) or use " + f"--backend biocentral, which has no length cap." + ), + ) diff --git a/apps/protspace/src/protspace/data/embedding/store.py b/apps/protspace/src/protspace/data/embedding/store.py new file mode 100644 index 00000000..c342b0ad --- /dev/null +++ b/apps/protspace/src/protspace/data/embedding/store.py @@ -0,0 +1,135 @@ +"""Shared HDF5 layer for the embedding backends. + +Owned by neither backend: :mod:`protspace.data.embedding.biocentral` and +:mod:`protspace.data.embedding.local` both import from here, so "what counts as +a complete run" has one definition instead of one per backend. +""" + +from __future__ import annotations + +import logging +from collections.abc import Collection, Iterable, Mapping +from pathlib import Path + +import h5py +import numpy as np + +logger = logging.getLogger(__name__) + +# Identifiers named in a message before it elides the rest. +_PREVIEW = 5 + + +def load_existing_ids(h5_path: Path) -> set[str]: + """Return the set of dataset keys already present in *h5_path*.""" + if not h5_path.exists(): + return set() + with h5py.File(h5_path, "r") as f: + return set(f.keys()) + + +def save_embeddings(h5_path: Path, embeddings: dict[str, np.ndarray]) -> None: + """Append embeddings to an HDF5 file (one dataset per protein).""" + with h5py.File(h5_path, "a") as f: + for protein_id, emb in embeddings.items(): + if protein_id not in f: + f.create_dataset(protein_id, data=emb.astype(np.float32)) + + +def validate_headers(ids: Iterable[str]) -> None: + """Raise :class:`ValueError` if any identifier contains ``/``. + + HDF5 treats ``/`` as a group separator, so such an identifier silently + becomes a group rather than a dataset and the requested key never exists. + Both backends call this before doing any work: detecting it afterwards costs + a full embedding run and reports a shortfall it cannot explain. + """ + bad = [i for i in ids if "/" in i] + if bad: + raise ValueError( + "Header(s) contain '/', invalid for HDF5 dataset names: " + preview_ids(bad) + ) + + +def preview_ids(ids: Iterable[str]) -> str: + """Comma-join *ids*, naming at most ``_PREVIEW`` of them.""" + ordered = sorted(ids) + shown = ", ".join(ordered[:_PREVIEW]) + return f"{shown}, ..." if len(ordered) > _PREVIEW else shown + + +def finish_run( + h5_path: Path, + requested: Collection[str], + *, + skipped: Mapping[str, str] | None = None, + context: str = "", + retry_hint: str = "", +) -> Path: + """Report the run, and raise unless *h5_path* covers everything expected. + + *skipped* maps identifier -> reason for sequences a backend deliberately did + not attempt because of a documented capability limit (over the length cap, + GPU OOM at batch size 1). Those are reported but never fail the run: a + capability limit is not a failure. Everything else absent from the file is. + + The check reads the file rather than a running total. ``save_embeddings`` + skips identifiers already present, so a counter can claim sequences the file + does not hold. + + *context* is backend detail for the failure message (e.g. how many batches + failed); *retry_hint* is the closing advice when nothing was produced. + """ + skipped = dict(skipped or {}) + requested_ids = set(requested) + expected = requested_ids - set(skipped) + on_disk = load_existing_ids(h5_path) + missing = expected - on_disk + embedded = len(expected) - len(missing) + + if skipped: + by_reason: dict[str, list[str]] = {} + for pid, reason in skipped.items(): + by_reason.setdefault(reason, []).append(pid) + for reason, ids in sorted(by_reason.items()): + logger.warning( + "Skipped %d sequence(s) — %s: %s", + len(ids), + reason, + preview_ids(ids), + ) + + if missing: + detail = ( + f"{embedded:,} of {len(expected):,} outstanding sequence(s) embedded, " + f"{len(missing):,} still missing" + ) + if context: + detail += f" ({context})" + if embedded == 0: + raise ValueError( + f"No new embeddings were produced for {h5_path}: {detail}. " + f"{retry_hint or 'Rerun to retry.'}" + ) + raise ValueError( + f"Embedding incomplete for {h5_path}: {detail}. " + f"Partial results were kept — rerun to embed only what is missing." + ) + + # Everything we meant to attempt is present. A run that skipped its way to an + # empty file still produced nothing usable, so it is a failure, not a success. + # An empty request is not that case -- it means resume already covered it all. + if requested_ids and not requested_ids & on_disk: + raise ValueError( + f"No new embeddings were produced for {h5_path}: all " + f"{len(requested_ids):,} sequence(s) were skipped " + f"({preview_ids(set(skipped.values()))})." + ) + + print( + f"\nDone. Embedded {embedded:,} sequence(s)" + + (f", skipped {len(skipped):,}" if skipped else "") + + "." + ) + print(f"Output: {h5_path}") + return h5_path From 51e7de5d652076127c6202679d91405a4ed1f701 Mon Sep 17 00:00:00 2001 From: tsenoner Date: Wed, 19 Aug 2026 12:25:24 +0200 Subject: [PATCH 3/9] feat(embed): expose --max-length so a skipped sequence is actionable The local backend's 2000 aa cap was hardcoded: only batch_size was wired to the CLI. Being told a sequence was skipped is a dead end if there is no lever to embed it, which is what makes "skip rather than fail" honest. Rejected for --backend biocentral, which has no length cap -- silently ignoring the flag would let a user believe they had raised a limit that does not exist. Also extracts build_embed_config(): the per-backend config construction was already written four times verbatim across embed and prepare, and threading a second option through would have made it eight. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Achark1sp4iqreoQdBYc7y --- .../src/protspace/cli/common_options.py | 42 +++++++++++++++++++ apps/protspace/src/protspace/cli/embed.py | 19 +++------ apps/protspace/src/protspace/cli/prepare.py | 20 ++------- 3 files changed, 52 insertions(+), 29 deletions(-) diff --git a/apps/protspace/src/protspace/cli/common_options.py b/apps/protspace/src/protspace/cli/common_options.py index b711d2ac..3fb51395 100644 --- a/apps/protspace/src/protspace/cli/common_options.py +++ b/apps/protspace/src/protspace/cli/common_options.py @@ -158,6 +158,18 @@ class Backend(StrEnum): ), ] +Opt_MaxLength = Annotated[ + int | None, + typer.Option( + min=1, + help=( + "Skip sequences longer than this (local backend only; default 2000). " + "Skipped sequences are named in the run summary." + ), + rich_help_panel="Embedding", + ), +] + # Input options (shared by prepare and project) Opt_Fasta = Annotated[ Path | None, @@ -168,3 +180,33 @@ class Backend(StrEnum): rich_help_panel="Input", ), ] + + +def build_embed_config( + backend: "Backend", + batch_size: int | None = None, + max_length: int | None = None, +): + """Build the embedding config matching *backend*, applying only what was given. + + Both ``embed`` and ``prepare`` need this, and each needs it per backend, so + without it the same four-line conditional appears four times. + """ + if backend == Backend.local: + from protspace.data.embedding.local import LocalEmbedConfig + + opts = {} + if batch_size is not None: + opts["batch_size"] = batch_size + if max_length is not None: + opts["max_length"] = max_length + return LocalEmbedConfig(**opts) + + from protspace.data.embedding.biocentral import EmbedConfig + + if max_length is not None: + raise typer.BadParameter( + "--max-length applies to --backend local; the Biocentral backend " + "has no length cap." + ) + return EmbedConfig(**({"batch_size": batch_size} if batch_size is not None else {})) diff --git a/apps/protspace/src/protspace/cli/embed.py b/apps/protspace/src/protspace/cli/embed.py index 56a6027d..dc60e3fc 100644 --- a/apps/protspace/src/protspace/cli/embed.py +++ b/apps/protspace/src/protspace/cli/embed.py @@ -11,7 +11,9 @@ Backend, Opt_Backend, Opt_BatchSize, + Opt_MaxLength, Opt_Verbose, + build_embed_config, ) logger = logging.getLogger(__name__) @@ -48,6 +50,7 @@ def embed( ], backend: Opt_Backend = Backend.biocentral, batch_size: Opt_BatchSize = None, + max_length: Opt_MaxLength = None, verbose: Opt_Verbose = 0, ) -> None: """FASTA → per-model HDF5 embeddings. @@ -69,29 +72,19 @@ def embed( output.mkdir(parents=True, exist_ok=True) - if backend == Backend.local: - from protspace.data.embedding.local import LocalEmbedConfig, embed_sequences + embed_config = build_embed_config(backend, batch_size, max_length) - embed_config = ( - LocalEmbedConfig(batch_size=batch_size) - if batch_size is not None - else LocalEmbedConfig() - ) + if backend == Backend.local: + from protspace.data.embedding.local import embed_sequences def resolve(name: str) -> str: return name # local backend takes the short key directly else: from protspace.data.embedding.biocentral import ( - EmbedConfig, embed_sequences, resolve_embedder, ) - embed_config = ( - EmbedConfig(batch_size=batch_size) - if batch_size is not None - else EmbedConfig() - ) resolve = resolve_embedder failed_models: list[str] = [] diff --git a/apps/protspace/src/protspace/cli/prepare.py b/apps/protspace/src/protspace/cli/prepare.py index 451a48c3..f19dc52e 100644 --- a/apps/protspace/src/protspace/cli/prepare.py +++ b/apps/protspace/src/protspace/cli/prepare.py @@ -29,6 +29,7 @@ Opt_FpRatio, Opt_LearningRate, Opt_MaxIter, + Opt_MaxLength, Opt_Methods, Opt_Metric, Opt_MinDist, @@ -39,6 +40,7 @@ Opt_RandomState, Opt_Similarity, Opt_Verbose, + build_embed_config, ) logger = logging.getLogger(__name__) @@ -310,6 +312,7 @@ def prepare( embedder: Opt_Embedder = None, backend: Opt_Backend = Backend.biocentral, batch_size: Opt_BatchSize = None, + max_length: Opt_MaxLength = None, # Projection methods: Opt_Methods = None, similarity: Opt_Similarity = False, @@ -427,22 +430,7 @@ def prepare( query_uniprot, ) - if backend == Backend.local: - from protspace.data.embedding.local import LocalEmbedConfig - - embed_config = ( - LocalEmbedConfig(batch_size=batch_size) - if batch_size is not None - else LocalEmbedConfig() - ) - else: - from protspace.data.embedding.biocentral import EmbedConfig - - embed_config = ( - EmbedConfig(batch_size=batch_size) - if batch_size is not None - else EmbedConfig() - ) + embed_config = build_embed_config(backend, batch_size, max_length) embedding_sets: list[EmbeddingSet] = [] fasta_for_similarity: Path | None = fasta From a6f54a5a529f4efe09a520d1a72bc55849f0edf0 Mon Sep 17 00:00:00 2001 From: tsenoner Date: Wed, 19 Aug 2026 12:25:54 +0200 Subject: [PATCH 4/9] fix(prepare): check embeddings against the supplied FASTA before similarity Nothing compared an embedding set to the FASTA it came from. That is not cosmetic: compute_similarity zero-fills the row for a protein it cannot find, leaving that protein's self-similarity at 0, and base_processor only converts similarity to distance when np.allclose(np.diag(data), 1). One uncovered protein makes that test false for the WHOLE matrix, so MDS consumes raw similarities as distances and the entire projection inverts -- near-identical proteins are placed furthest apart. Demonstrated on a 3-protein matrix: with full coverage d(A,B)=0.224 against d(A,C)=0.949, so A and B are closest; with one protein uncovered the conversion is skipped and d(A,B)=0.950 against d(A,C)=0.000, flipping every pair. The check is directional. h5 - fasta is the damaging direction and the only one reported; fasta - h5 is routine, because a resumed embedding cache legitimately covers fewer proteins than the FASTA it was built from, and the extra entries are simply unused. Uncovered proteins fail under -s/--similarity and warn otherwise -- failing outright would break deliberately projecting a subset, warning alone would let the inverted bundle ship. Both sides go through parse_identifier: load_h5 keeps raw HDF5 keys while FASTA-derived ids are always parsed, so comparing them raw would report every protein uncovered for a sp|...-keyed file. Placed before compute_similarity in both prepare and project; project has the same exposure and no plumbing for it today. This change stops a partially-covered FASTA reaching the diagonal heuristic. It does not make the heuristic itself robust -- that is tracked separately. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Achark1sp4iqreoQdBYc7y --- apps/protspace/src/protspace/cli/prepare.py | 12 +++++ apps/protspace/src/protspace/cli/project.py | 3 ++ .../src/protspace/data/loaders/fasta.py | 49 +++++++++++++++++++ 3 files changed, 64 insertions(+) diff --git a/apps/protspace/src/protspace/cli/prepare.py b/apps/protspace/src/protspace/cli/prepare.py index f19dc52e..dc505686 100644 --- a/apps/protspace/src/protspace/cli/prepare.py +++ b/apps/protspace/src/protspace/cli/prepare.py @@ -503,6 +503,18 @@ def prepare( if not embedding_sets: raise typer.BadParameter("No valid input data found.") + # --- FASTA coverage --- + # Before similarity, not after: an uncovered protein inverts the whole + # MDS projection rather than degrading its own row. + if fasta_for_similarity is not None and embedding_sets: + from protspace.data.loaders.fasta import check_fasta_coverage + + check_fasta_coverage( + fasta_for_similarity, + embedding_sets[0].headers, + required=bool(similarity), + ) + # --- Similarity --- if similarity: if fasta_for_similarity is None: diff --git a/apps/protspace/src/protspace/cli/project.py b/apps/protspace/src/protspace/cli/project.py index 60d0e945..db8101d1 100644 --- a/apps/protspace/src/protspace/cli/project.py +++ b/apps/protspace/src/protspace/cli/project.py @@ -109,6 +109,9 @@ def project( if similarity: if fasta is None: raise typer.BadParameter("--similarity requires --fasta.") + from protspace.data.loaders.fasta import check_fasta_coverage + + check_fasta_coverage(fasta, embedding_sets[0].headers, required=True) sim_set = compute_similarity(fasta, embedding_sets[0].headers) embedding_sets.append(sim_set) diff --git a/apps/protspace/src/protspace/data/loaders/fasta.py b/apps/protspace/src/protspace/data/loaders/fasta.py index 9c95c251..774cd098 100644 --- a/apps/protspace/src/protspace/data/loaders/fasta.py +++ b/apps/protspace/src/protspace/data/loaders/fasta.py @@ -6,6 +6,7 @@ from __future__ import annotations import logging +from collections.abc import Iterable from pathlib import Path from typing import TYPE_CHECKING @@ -86,3 +87,51 @@ def embed_fasta( f.attrs["model_name"] = embedder return load_h5([h5_path], name_override=embedder) + + +def check_fasta_coverage( + fasta_path: Path, + headers: Iterable[str], + *, + required: bool = False, +) -> None: + """Report embedded proteins that *fasta_path* does not cover. + + Directional on purpose. A FASTA covering MORE than the embeddings is routine + -- a resumed embedding cache legitimately holds fewer proteins than the FASTA + it was built from -- and the extra entries are simply unused downstream. + + The other direction is damaging. ``compute_similarity`` zero-fills the row + for a protein it cannot find, leaving that protein's self-similarity at 0, + and the similarity-to-distance conversion only fires when the WHOLE diagonal + is 1. One uncovered protein therefore suppresses the conversion for every + pair and inverts the entire MDS projection -- so under ``--similarity`` this + is an error, not a warning. + + Both sides go through ``parse_identifier``: ``load_h5`` keeps raw HDF5 keys + while FASTA-derived identifiers are always parsed, so comparing them raw + would report every protein uncovered for a ``sp|...``-keyed file. + """ + from protspace.data.io.fasta import parse_fasta + + fasta_ids = {parse_identifier(h) for h in parse_fasta(fasta_path)} + embedded = {parse_identifier(h) for h in headers} + uncovered = embedded - fasta_ids + if not uncovered: + return + + ordered = sorted(uncovered) + preview = ", ".join(ordered[:5]) + (", ..." if len(ordered) > 5 else "") + detail = ( + f"{len(uncovered):,} of {len(embedded):,} embedded protein(s) are absent " + f"from {fasta_path}: {preview}" + ) + if required: + raise ValueError( + f"{detail}. Similarity needs every embedded protein present in the " + f"FASTA: an uncovered protein leaves its self-similarity at 0, which " + f"suppresses the similarity-to-distance conversion for the whole " + f"matrix and inverts the projection. Supply a FASTA that covers them, " + f"or drop -s/--similarity." + ) + logger.warning("%s", detail) From dd184d26b08d36ac2af8bb8b708c6af94a61d147 Mon Sep 17 00:00:00 2001 From: tsenoner Date: Wed, 19 Aug 2026 12:26:02 +0200 Subject: [PATCH 5/9] test(embed): cover the completeness contract and FASTA coverage Pins the rule both backends now share, and the two behaviours that used to differ between them: - expected = requested - skipped: a capability limit is skipped and reported, anything else missing fails, skipping everything is still a failure, and an empty request means resume already covered it - the gate reads the .h5, not the caller's count - both backends raise the SAME error for the same '/' identifier, and reject it before any API call or model load - local: over-length and OOM sequences are skipped not failed, raising --max-length embeds a previously skipped one, and a shortfall that is not a skip still fails - failure messages trip none of the prep service's BIOCENTRAL_DOWN_PATTERNS, so a coverage problem is never reported as a service outage and routed to Colab - FASTA coverage: uncovered blocks similarity, warns without it, a superset is silent, and mixed identifier styles reconcile Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Achark1sp4iqreoQdBYc7y --- apps/protspace/tests/test_backend_switch.py | 79 ++++++++++ .../tests/test_biocentral_embedder.py | 54 ++++++- .../tests/test_embed_completeness.py | 143 ++++++++++++++++++ apps/protspace/tests/test_local_embedder.py | 91 ++++++++++- 4 files changed, 359 insertions(+), 8 deletions(-) create mode 100644 apps/protspace/tests/test_embed_completeness.py diff --git a/apps/protspace/tests/test_backend_switch.py b/apps/protspace/tests/test_backend_switch.py index 9a4d58d7..7dee1be5 100644 --- a/apps/protspace/tests/test_backend_switch.py +++ b/apps/protspace/tests/test_backend_switch.py @@ -273,3 +273,82 @@ def always_fails(sequences, embedder, h5_path, embed_config=None): assert result.exit_code == 1, result.output assert "Saved:" not in result.output assert not (out / "prot_t5.h5").exists(), "no .h5 may be fabricated on failure" + + +def test_embed_cli_wires_max_length_to_local_config(tmp_path, monkeypatch): + """Without a lever, "skipped 3 sequences" is a dead end for the user.""" + fasta = tmp_path / "s.fasta" + fasta.write_text(">P12345\nMKVLAAG\n") + captured = {} + monkeypatch.setattr( + "protspace.data.embedding.local.embed_sequences", _fake_embed(captured) + ) + + result = CliRunner().invoke( + app, + [ + "embed", + "-i", + str(fasta), + "-e", + "prot_t5", + "-o", + str(tmp_path / "out"), + "--backend", + "local", + "--max-length", + "512", + ], + ) + + assert result.exit_code == 0, result.output + assert captured["config"].max_length == 512 + + +def test_embed_cli_rejects_max_length_for_biocentral(tmp_path): + """The remote backend has no length cap, so silently ignoring the flag would + let a user believe they had raised a limit that does not exist.""" + fasta = tmp_path / "s.fasta" + fasta.write_text(">P12345\nMKVLAAG\n") + + result = CliRunner().invoke( + app, + [ + "embed", + "-i", + str(fasta), + "-e", + "prot_t5", + "-o", + str(tmp_path / "out"), + "--max-length", + "512", + ], + ) + + assert result.exit_code != 0 + assert "backend local" in result.output + + +def test_embed_cli_rejects_nonpositive_max_length(tmp_path): + fasta = tmp_path / "s.fasta" + fasta.write_text(">P12345\nMKVLAAG\n") + + result = CliRunner().invoke( + app, + [ + "embed", + "-i", + str(fasta), + "-e", + "prot_t5", + "-o", + str(tmp_path / "out"), + "--backend", + "local", + "--max-length", + "0", + ], + ) + + assert result.exit_code != 0 diff --git a/apps/protspace/tests/test_biocentral_embedder.py b/apps/protspace/tests/test_biocentral_embedder.py index 50821707..d016f354 100644 --- a/apps/protspace/tests/test_biocentral_embedder.py +++ b/apps/protspace/tests/test_biocentral_embedder.py @@ -385,23 +385,63 @@ def test_complete_run_does_not_raise(self, monkeypatch, tmp_path): ) assert bar["n"] == bar["total"] == self.N - def test_gate_reads_the_file_not_a_counter(self, monkeypatch, tmp_path): - """h5py turns an ID containing "/" into a group, so two requested IDs can - collapse into one dataset. A counter of what was *sent* to save_embeddings - sees 2/2 and exits 0; only the file itself shows the shortfall.""" + def test_slash_in_id_is_rejected_before_any_api_call(self, monkeypatch, tmp_path): + """h5py turns an ID containing "/" into a group, so it can never become the + requested dataset. Both backends refuse up front rather than paying for a + full embedding run and then reporting a shortfall they cannot explain.""" from src.protspace.data.embedding import biocentral as bc + called = [] monkeypatch.setattr( bc, "BiocentralAPI", - lambda **kw: self._fake_api(to_dict=lambda s: self._embeddings(s)), + lambda **kw: called.append(1) or self._fake_api(to_dict=self._embeddings), ) - monkeypatch.setattr(bc.time, "sleep", lambda s: None) h5_path = tmp_path / "collide.h5" - with pytest.raises(ValueError, match="Embedding incomplete"): + with pytest.raises(ValueError, match=r"Header\(s\) contain '/'"): bc.embed_sequences({"A/B": "MKV", "A": "MKW"}, "m", h5_path) + assert not called, "must reject before connecting to the API" + assert not h5_path.exists() + + def test_both_backends_reject_the_same_id(self, tmp_path): + """Both backends raise the same error for the same invalid identifier, + because the rejection lives in the shared layer rather than in either.""" + from src.protspace.data.embedding import biocentral as bc + from src.protspace.data.embedding import local + + bad = {"A/B": "MKV"} + messages = [] + for backend in (bc, local): + with pytest.raises(ValueError) as exc: + backend.embed_sequences(bad, "esm2_8m", tmp_path / "x.h5") + messages.append(str(exc.value)) + + assert messages[0] == messages[1], messages + assert "invalid for HDF5 dataset names" in messages[0] + + def test_gate_reads_the_file_not_a_counter(self, monkeypatch, tmp_path): + """The completeness gate reads the .h5, never a running total. A writer + that under-delivers -- save_embeddings skips IDs already present -- must + still be caught.""" + from src.protspace.data.embedding import biocentral as bc + + seqs, h5_path, _, bc_mod = self._run( + monkeypatch, tmp_path, to_dict=lambda s: self._embeddings(s) + ) + + real_save = bc.save_embeddings + dropped = sorted(seqs)[0] + + def lossy_save(path, embeddings): + real_save(path, {k: v for k, v in embeddings.items() if k != dropped}) + + monkeypatch.setattr(bc, "save_embeddings", lossy_save) + + with pytest.raises(ValueError, match="Embedding incomplete"): + bc_mod.embed_sequences(seqs, "m", h5_path, embed_config=bc.EmbedConfig(2)) + def test_rerun_embeds_only_what_is_missing(self, monkeypatch, tmp_path): """A failed run must leave the pipeline able to converge on a retry.""" import h5py diff --git a/apps/protspace/tests/test_embed_completeness.py b/apps/protspace/tests/test_embed_completeness.py new file mode 100644 index 00000000..0d4b6fd1 --- /dev/null +++ b/apps/protspace/tests/test_embed_completeness.py @@ -0,0 +1,143 @@ +"""Completeness contract shared by both embedding backends. + +The rule is `expected = requested - skipped`: a documented capability limit is +skipped and reported, anything else absent from the .h5 fails. Before this +contract the local backend exited 0 on a 90%-complete .h5, which then projected, +bundled and scored normally. +""" + +from pathlib import Path + +import h5py +import numpy as np +import pytest + +from protspace.data.embedding import store +from protspace.data.loaders.fasta import check_fasta_coverage + + +def _write(h5_path: Path, ids) -> None: + with h5py.File(h5_path, "a") as f: + for pid in ids: + f.create_dataset(pid, data=np.zeros(4, dtype=np.float32)) + + +class TestFinishRun: + def test_complete_run_succeeds(self, tmp_path): + h5 = tmp_path / "o.h5" + _write(h5, ["a", "b"]) + assert store.finish_run(h5, ["a", "b"]) == h5 + + def test_missing_sequence_fails(self, tmp_path): + h5 = tmp_path / "o.h5" + _write(h5, ["a"]) + with pytest.raises(ValueError, match="Embedding incomplete"): + store.finish_run(h5, ["a", "b"]) + + def test_nothing_embedded_is_distinguished_from_partial(self, tmp_path): + h5 = tmp_path / "o.h5" + with pytest.raises(ValueError, match="No new embeddings were produced"): + store.finish_run(h5, ["a", "b"]) + + def test_capability_limit_is_skipped_not_failed(self, tmp_path): + """The whole point: a sequence we deliberately never attempted must not + fail the run, but must still be reported.""" + h5 = tmp_path / "o.h5" + _write(h5, ["a"]) + assert store.finish_run(h5, ["a", "b"], skipped={"b": "too long"}) == h5 + + def test_skips_are_named_with_their_reason(self, tmp_path, caplog): + h5 = tmp_path / "o.h5" + _write(h5, ["a"]) + with caplog.at_level("WARNING"): + store.finish_run(h5, ["a", "b", "c"], skipped={"b": "too long", "c": "OOM"}) + text = caplog.text + assert "too long" in text and "OOM" in text + assert "b" in text and "c" in text + + def test_skipping_everything_is_still_a_failure(self, tmp_path): + h5 = tmp_path / "o.h5" + with pytest.raises(ValueError, match="No new embeddings were produced"): + store.finish_run(h5, ["a"], skipped={"a": "too long"}) + + def test_empty_request_means_resume_covered_it(self, tmp_path): + """An empty outstanding set is not 'nothing was produced' -- it means a + previous run already embedded everything.""" + h5 = tmp_path / "o.h5" + _write(h5, ["a"]) + assert store.finish_run(h5, []) == h5 + + def test_gate_reads_the_file_not_the_caller(self, tmp_path): + """save_embeddings skips IDs already present, so a running total can claim + sequences the file does not hold. The gate must read the file.""" + h5 = tmp_path / "o.h5" + store.save_embeddings(h5, {"a": np.zeros(4, dtype=np.float32)}) + store.save_embeddings(h5, {"a": np.ones(4, dtype=np.float32)}) # skipped + with pytest.raises(ValueError, match="Embedding incomplete"): + store.finish_run(h5, ["a", "b"]) + + def test_message_cannot_be_mistaken_for_a_service_outage(self, tmp_path): + """The prep service substring-matches stderr to classify a failure as + BIOCENTRAL_UNAVAILABLE and route the user to Colab. A coverage problem + must not trip those patterns -- Colab would not fix it.""" + patterns = ( + "connection refused", + "cannot connect to host", + "connectionerror", + "temporary failure in name resolution", + "name or service not known", + "503 service unavailable", + "503 server error", + "no healthy biocentral", + ) + h5 = tmp_path / "o.h5" + _write(h5, ["a"]) + with pytest.raises(ValueError) as exc: + store.finish_run(h5, ["a", "b"]) + assert not [p for p in patterns if p in str(exc.value).lower()] + + +class TestValidateHeaders: + def test_rejects_slash(self): + with pytest.raises(ValueError, match="invalid for HDF5 dataset names"): + store.validate_headers(["A/B"]) + + def test_accepts_ordinary_ids(self): + store.validate_headers(["P12345", "sp|P12345|NAME"]) + + +class TestFastaCoverage: + @staticmethod + def _fasta(tmp_path, ids): + p = tmp_path / "s.fasta" + p.write_text("".join(f">{i}\nMKV\n" for i in ids)) + return p + + def test_uncovered_embeddings_block_similarity(self, tmp_path): + """One uncovered protein zero-fills its diagonal, which suppresses the + similarity-to-distance conversion for the WHOLE matrix and inverts MDS.""" + fasta = self._fasta(tmp_path, ["P1", "P2"]) + with pytest.raises(ValueError, match="absent from"): + check_fasta_coverage(fasta, ["P1", "P2", "P3"], required=True) + + def test_uncovered_embeddings_only_warn_without_similarity(self, tmp_path, caplog): + fasta = self._fasta(tmp_path, ["P1"]) + with caplog.at_level("WARNING"): + check_fasta_coverage(fasta, ["P1", "P2"], required=False) + assert "absent from" in caplog.text + + def test_fasta_superset_is_silent(self, tmp_path, caplog): + """A resumed embedding cache legitimately covers fewer proteins than the + FASTA it was built from.""" + fasta = self._fasta(tmp_path, ["P1", "P2", "P3"]) + with caplog.at_level("WARNING"): + check_fasta_coverage(fasta, ["P1"], required=True) + assert caplog.text == "" + + def test_identifier_styles_are_reconciled(self, tmp_path, caplog): + """load_h5 keeps raw HDF5 keys while FASTA ids are parsed, so comparing + them raw would report every protein uncovered.""" + fasta = self._fasta(tmp_path, ["sp|P12345|NAME_HUMAN"]) + with caplog.at_level("WARNING"): + check_fasta_coverage(fasta, ["P12345"], required=True) + assert caplog.text == "" diff --git a/apps/protspace/tests/test_local_embedder.py b/apps/protspace/tests/test_local_embedder.py index 08bb05c5..fde6ec1a 100644 --- a/apps/protspace/tests/test_local_embedder.py +++ b/apps/protspace/tests/test_local_embedder.py @@ -6,6 +6,7 @@ transformers) and downloads a small ESM2 model, so it is marked ``slow``. """ +import h5py import numpy as np import pytest @@ -140,7 +141,7 @@ def test_embed_sequences_raises_when_all_sequences_dropped(tmp_path): """All sequences over max_length → no embeddings → a clear error, not a silently-empty/absent .h5 that later crashes load_h5.""" out = tmp_path / "emb.h5" - with pytest.raises(ValueError, match="No embeddings"): + with pytest.raises(ValueError, match="No new embeddings"): local.embed_sequences( {"p1": "MKVLAAGILT"}, "esm2_8m", @@ -192,3 +193,91 @@ def test_embed_sequences_resumes_and_skips_existing(tmp_path): with h5py.File(out, "r") as f: assert set(f.keys()) == {"prot1", "prot2"} np.testing.assert_array_equal(f["prot1"][:], first) + + +# --------------------------------------------------------------------------- +# Completeness contract: a capability limit is skipped, anything else fails +# --------------------------------------------------------------------------- + + +def _stub_model(monkeypatch, *, oom_ids=()): + """Replace model loading and inference so the contract can be tested without + downloading a checkpoint.""" + import torch + + monkeypatch.setattr(local, "setup_model", lambda ckpt, mt: (None, None, "cpu")) + + def fake_embed_batch(processed, mod_type, model, tokenizer, device, max_length): + if len(processed) == 1 and processed[0] in oom_ids: + raise torch.cuda.OutOfMemoryError("stub OOM") + return [np.zeros(4, dtype=np.float32) for _ in processed] + + monkeypatch.setattr(local, "_embed_batch", fake_embed_batch) + + +def test_over_length_sequences_are_skipped_not_failed(tmp_path, monkeypatch): + """A documented capability limit must not fail the run -- but must be named.""" + _stub_model(monkeypatch) + out = tmp_path / "emb.h5" + + result = local.embed_sequences( + {"short": "MKVL", "long": "M" * 50}, + "esm2_8m", + out, + local.LocalEmbedConfig(max_length=10), + ) + + assert result == out + with h5py.File(out, "r") as f: + assert set(f.keys()) == {"short"} + + +def test_raising_max_length_embeds_a_previously_skipped_sequence(tmp_path, monkeypatch): + _stub_model(monkeypatch) + out = tmp_path / "emb.h5" + seqs = {"short": "MKVL", "long": "M" * 50} + + local.embed_sequences(seqs, "esm2_8m", out, local.LocalEmbedConfig(max_length=10)) + local.embed_sequences(seqs, "esm2_8m", out, local.LocalEmbedConfig(max_length=100)) + + with h5py.File(out, "r") as f: + assert set(f.keys()) == {"short", "long"} + + +def test_oom_at_batch_size_one_is_skipped(tmp_path, monkeypatch): + """Same class as the length cap: this machine cannot do this sequence.""" + _stub_model(monkeypatch) + out = tmp_path / "emb.h5" + # preprocess_sequence is identity-ish for esm; key the stub off the processed text + _stub_model(monkeypatch, oom_ids={"M" * 20}) + + result = local.embed_sequences( + {"ok": "MKVL", "hungry": "M" * 20}, + "esm2_8m", + out, + local.LocalEmbedConfig(batch_size=1), + ) + + assert result == out + with h5py.File(out, "r") as f: + assert set(f.keys()) == {"ok"} + + +def test_shortfall_that_is_not_a_skip_still_fails(tmp_path, monkeypatch): + """Everything absent from the .h5 that was NOT deliberately skipped is a + failure -- this is what the local backend used to miss entirely.""" + _stub_model(monkeypatch) + real_save = local.save_embeddings + monkeypatch.setattr( + local, + "save_embeddings", + lambda p, e: real_save(p, {k: v for k, v in e.items() if k != "dropped"}), + ) + + with pytest.raises(ValueError, match="Embedding incomplete"): + local.embed_sequences( + {"kept": "MKVL", "dropped": "MKVA"}, + "esm2_8m", + tmp_path / "emb.h5", + local.LocalEmbedConfig(batch_size=8), + ) From eae28f6f1b66198e5f0a73d04d87c4aee86222fa Mon Sep 17 00:00:00 2001 From: tsenoner Date: Wed, 19 Aug 2026 12:26:10 +0200 Subject: [PATCH 6/9] docs(embed): document the completeness contract and --max-length Adds a "When embedding fails" section separating skipped from failed, the --max-length row, and the -f/--fasta coverage note. Records store.py and the new test module in the package CLAUDE.md, and reconciles the spec's reporting requirement with what shipped: skips go to stderr at warning level, the counts go to the end-of-run line, and a clean run stays quiet. Checked the Colab notebooks: they build LocalEmbedConfig() with defaults and call embed_fasta, so they need no change -- they gain the skip summary and the failure on a real shortfall for free. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Achark1sp4iqreoQdBYc7y --- apps/protspace/CLAUDE.md | 8 +-- apps/protspace/docs/cli.md | 34 ++++++++++++ .../specs/embed-completeness/spec.md | 23 +++++--- .../embed-completeness-contract/tasks.md | 52 +++++++++---------- 4 files changed, 81 insertions(+), 36 deletions(-) diff --git a/apps/protspace/CLAUDE.md b/apps/protspace/CLAUDE.md index 5b487a22..c0341fd4 100644 --- a/apps/protspace/CLAUDE.md +++ b/apps/protspace/CLAUDE.md @@ -46,7 +46,7 @@ Single entry point: `protspace = protspace.cli.app:app` | Command | Purpose | |---------|---------| | `protspace prepare` | Full pipeline: embed → reduce → annotate → bundle | -| `protspace embed` | FASTA → HDF5 embeddings (Biocentral API or local GPU/CPU via `--backend local`) | +| `protspace embed` | FASTA → HDF5 embeddings (Biocentral API or local GPU/CPU via `--backend local`). Exits non-zero on an incomplete embedding; capability-limited sequences (`--max-length`, GPU OOM) are skipped and named instead | | `protspace project` | HDF5 → dimensionality reduction | | `protspace annotate` | Fetch protein annotations | | `protspace bundle` | Combine projections + annotations → .parquetbundle | @@ -152,6 +152,7 @@ src/protspace/ │ │ ├── settings_converter.py # Settings table conversion │ │ └── writers.py # Annotation output writers │ ├── embedding/ +│ │ ├── store.py # Shared HDF5 layer + completeness contract (owned by neither backend) │ │ ├── biocentral.py # Biocentral API client, model shortcut mappings │ │ └── local.py # Local GPU/CPU backend (HF transformers, [local] extra) │ ├── parsers/ @@ -276,9 +277,10 @@ For a live count run `uv run pytest tests/ --collect-only -q`. | `test_stats_bundle.py` | Optional 5th (statistics) bundle part round-trip | | `test_annotation_select.py` | Annotation selection: suitability filter (cardinality/numeric/id-like exclusion), `auto` vs explicit-list label building (explicit names bypass the heuristic), missing-value dropping | | `test_annotation_validity.py` | `AnnotationValidityStatistic`: silhouette/DBI/CH scored per annotation on `ctx.coords`, embedding vs. projection `space_kind`, missing-value exclusion, single-category no-op, id-canonical subsample determinism | -| `test_biocentral_embedder.py` | Biocentral API client, embedding flow | +| `test_biocentral_embedder.py` | Biocentral API client, embedding flow, completeness gate (reads the .h5, not a counter), `/`-in-header rejection | +| `test_embed_completeness.py` | Shared embed contract (`data/embedding/store.py`): `expected = requested - skipped`, skip-vs-fail, skip reporting, resume-covered runs, FASTA coverage direction + identifier normalisation | | `test_backend_switch.py` | Embedding backend switch: `resolve_default_backend` (Colab+GPU→local), `embed_fasta` local/biocentral dispatch (short key vs resolved name), `protspace embed --backend` CLI wiring + enum validation + non-positive batch_size rejection | -| `test_local_embedder.py` | Local embedding backend: checkpoint resolution (12 short keys, Synthyra ESM-C), per-family preprocessing/residue pooling, `/`-in-header guard, LocalEmbedConfig validation, empty-output guard, esm2_8m end-to-end + resume (slow) | +| `test_local_embedder.py` | Local embedding backend: checkpoint resolution (12 short keys, Synthyra ESM-C), per-family preprocessing/residue pooling, `/`-in-header guard, LocalEmbedConfig validation, over-length + OOM skips reported not failed, non-skip shortfall fails, esm2_8m end-to-end + resume (slow) | | `test_fasta.py` | FASTA parsing, edge cases, CSV annotation loading | | `test_biocentral_retriever.py` | Biocentral prediction retriever (TMbed parsing, per-sequence) | | `test_taxonomy_annotation_retriever.py` | Taxonomy via UniProt Taxonomy API (mocked + integration) | diff --git a/apps/protspace/docs/cli.md b/apps/protspace/docs/cli.md index 22b433c2..7da170e2 100644 --- a/apps/protspace/docs/cli.md +++ b/apps/protspace/docs/cli.md @@ -63,6 +63,7 @@ protspace prepare -i emb.h5 -m "pca2,umap2:n_neighbors=50;min_dist=0.3,tsne2" -o | `-e, --embedder` | Model shortcut (comma-separated for multi-model). | `prot_t5` | | `-b, --backend` | Embedding engine: `biocentral` (remote API) or `local` (on-device GPU/CPU). | `biocentral` | | `--batch-size` | Sequences per batch. Backend default when unset: 1000 (API call) or 8 (local GPU micro-batch). | — | +| `--max-length` | Skip sequences longer than this (`--backend local` only). Skipped sequences are named in the run summary. | `2000` | **Available embedders:** `prot_t5`, `prost_t5`, `esm2_8m`, `esm2_35m`, `esm2_150m`, `esm2_650m`, `esm2_3b`, `ankh_base`, `ankh_large`, `ankh3_large`, `esmc_300m`, `esmc_600m` @@ -151,8 +152,41 @@ protspace embed -i sequences.fasta -e prot_t5 -e esm2_3b -o embeddings/ # On-device GPU/CPU — works offline / when Biocentral is down protspace embed -i sequences.fasta -e prot_t5 -o embeddings/ --backend local + +# Raise the local length cap for a dataset with long sequences +protspace embed -i sequences.fasta -e prot_t5 -o embeddings/ --backend local --max-length 4000 ``` +### When embedding fails + +An incomplete embedding exits **non-zero**, on either backend. A truncated `.h5` +projects, bundles and scores completely normally, so a run that embedded 90% of +your proteins would otherwise hand you plausible numbers computed on a silently +truncated dataset. + +Two outcomes are distinguished: + +- **Skipped** — a sequence a documented capability limit puts out of reach: + longer than `--max-length`, or exhausting GPU memory at batch size 1 (local + backend only). These are named in the run summary and do **not** fail the run. +- **Failed** — anything else missing from the `.h5`. The run exits 1 and the + partial output is kept, so a rerun embeds only what is missing. + +Multiple `-e` models are independent: one failing model no longer abandons the +rest, and the command exits 1 once at the end naming every model that failed. + +Identifiers containing `/` are rejected up front on both backends — HDF5 treats +`/` as a group separator, so such an identifier can never become the dataset you +asked for. + +> **`-f/--fasta` coverage:** when a FASTA is supplied alongside HDF5 input, the +> embeddings are checked against it. Proteins in the `.h5` that the FASTA does not +> cover are reported, and with `-s/--similarity` they are an error: an uncovered +> protein leaves its self-similarity at 0, which suppresses the +> similarity-to-distance conversion for the whole matrix and inverts the MDS +> projection. A FASTA covering *more* than the embeddings is normal and is not +> reported. + ## `protspace project` Run dimensionality reduction on HDF5 embeddings. diff --git a/openspec/changes/embed-completeness-contract/specs/embed-completeness/spec.md b/openspec/changes/embed-completeness-contract/specs/embed-completeness/spec.md index de12aefb..8efd4760 100644 --- a/openspec/changes/embed-completeness-contract/specs/embed-completeness/spec.md +++ b/openspec/changes/embed-completeness-contract/specs/embed-completeness/spec.md @@ -66,17 +66,26 @@ length, or one that exhausts GPU memory at batch size 1. ### Requirement: Every run reports what it embedded and skipped -Embedding SHALL report the requested, embedded, and skipped counts once per run on -stderr at warning level or above, naming the skipped identifiers with their reason -when any were skipped. Stderr rather than stdout because the hosted prep service -discards the subprocess's stdout and keeps only stderr; warning level or above -because the default verbosity shows nothing below it. +Embedding SHALL report every skipped sequence on stderr at warning level or above, +grouped by reason and naming the identifiers, and SHALL report the embedded and +skipped counts once per run. Skips go to stderr because a skipped sequence means +the dataset is incomplete even though the run succeeded, and the hosted prep +service discards the subprocess's stdout while keeping stderr; warning level or +above because the default verbosity shows nothing below it. #### Scenario: A run with skips names them - **WHEN** a run completes with one or more skipped sequences -- **THEN** the summary states how many were requested, embedded, and skipped -- **AND** it names the skipped identifiers, previewing the first few when there are many +- **THEN** each reason is reported on stderr with the identifiers it applies to, + previewing the first few when there are many +- **AND** the end-of-run line states how many sequences were embedded and how many + were skipped + +#### Scenario: A clean run is not made noisy + +- **WHEN** a run completes with nothing skipped +- **THEN** no skip warning is emitted, and the end-of-run line reports the + embedded count alone #### Scenario: The summary cannot be mistaken for a service outage diff --git a/openspec/changes/embed-completeness-contract/tasks.md b/openspec/changes/embed-completeness-contract/tasks.md index c6ec2540..e37d2b6a 100644 --- a/openspec/changes/embed-completeness-contract/tasks.md +++ b/openspec/changes/embed-completeness-contract/tasks.md @@ -1,61 +1,61 @@ ## 1. Shared HDF5 layer -- [ ] 1.1 Add `apps/protspace/src/protspace/data/embedding/store.py` with +- [x] 1.1 Add `apps/protspace/src/protspace/data/embedding/store.py` with `load_existing_ids`, `save_embeddings`, `validate_headers` (moved from `biocentral.py` / `local.py`) and `finish_run()`. -- [ ] 1.2 Re-export `load_existing_ids` and `save_embeddings` from `biocentral.py` +- [x] 1.2 Re-export `load_existing_ids` and `save_embeddings` from `biocentral.py` so `local.py`, `cli/annotate.py` and existing tests keep importing them. -- [ ] 1.3 `finish_run()` computes `expected = requested − skipped`, raises when the +- [x] 1.3 `finish_run()` computes `expected = requested − skipped`, raises when the HDF5 does not cover `expected`, raises when nothing landed at all, and otherwise emits one stderr summary at warning level. ## 2. Both backends call the contract -- [ ] 2.1 `biocentral.py`: call `validate_headers` before the first batch; replace +- [x] 2.1 `biocentral.py`: call `validate_headers` before the first batch; replace the inline gate with `finish_run(..., skipped={})`. -- [ ] 2.2 `local.py`: capture over-length and OOM-skipped identifiers into a +- [x] 2.2 `local.py`: capture over-length and OOM-skipped identifiers into a `{id: reason}` map instead of only logging them; call `finish_run` with it. -- [ ] 2.3 `local.py`: stop advancing the progress bar for an OOM-skipped sequence. -- [ ] 2.4 Verify neither backend imports from the other. +- [x] 2.3 `local.py`: stop advancing the progress bar for an OOM-skipped sequence. +- [x] 2.4 Verify neither backend imports from the other. ## 3. `--max-length` -- [ ] 3.1 Add `Opt_MaxLength` to `cli/common_options.py`, rejecting values below 1. -- [ ] 3.2 Wire it through `cli/embed.py` and `cli/prepare.py` into `LocalEmbedConfig`. -- [ ] 3.3 Reject it (or document it as ignored) for `--backend biocentral`, which +- [x] 3.1 Add `Opt_MaxLength` to `cli/common_options.py`, rejecting values below 1. +- [x] 3.2 Wire it through `cli/embed.py` and `cli/prepare.py` into `LocalEmbedConfig`. +- [x] 3.3 Reject it (or document it as ignored) for `--backend biocentral`, which has no length cap. ## 4. FASTA coverage -- [ ] 4.1 Add the coverage check to `data/processors/pipeline.py`, normalising both +- [x] 4.1 Add the coverage check to `data/processors/pipeline.py`, normalising both sides through `parse_identifier`. -- [ ] 4.2 Fail when similarity is requested and any embedded protein is uncovered; +- [x] 4.2 Fail when similarity is requested and any embedded protein is uncovered; warn otherwise; say nothing when the FASTA is a superset. -- [ ] 4.3 Place it so it runs before `compute_similarity`, not after. -- [ ] 4.4 Confirm the message contains no `_BIOCENTRAL_DOWN_PATTERNS` substring. +- [x] 4.3 Place it so it runs before `compute_similarity`, not after. +- [x] 4.4 Confirm the message contains no `_BIOCENTRAL_DOWN_PATTERNS` substring. ## 5. Tests -- [ ] 5.1 `test_local_embedder.py`: over-length skip exits 0 and is named; OOM skip +- [x] 5.1 `test_local_embedder.py`: over-length skip exits 0 and is named; OOM skip exits 0 and is named; a non-skip shortfall fails; skipping everything fails; the bar does not advance for a skip. -- [ ] 5.2 `test_biocentral_embedder.py`: `/` now rejected up front; the disk gate +- [x] 5.2 `test_biocentral_embedder.py`: `/` now rejected up front; the disk gate still catches a writer that under-delivers. -- [ ] 5.3 New coverage tests: uncovered + similarity fails, uncovered alone warns, +- [x] 5.3 New coverage tests: uncovered + similarity fails, uncovered alone warns, superset silent, mixed identifier styles reconcile. -- [ ] 5.4 `test_backend_switch.py`: `--max-length` wiring and rejection. -- [ ] 5.5 Both backends raise the same error for the same invalid identifier. +- [x] 5.4 `test_backend_switch.py`: `--max-length` wiring and rejection. +- [x] 5.5 Both backends raise the same error for the same invalid identifier. ## 6. Docs + verification -- [ ] 6.1 Update `apps/protspace/docs/cli.md` and `README.md` for `--max-length` +- [x] 6.1 Update `apps/protspace/docs/cli.md` and `README.md` for `--max-length` and the completeness/coverage behaviour. -- [ ] 6.2 Update the CLI table in `apps/protspace/CLAUDE.md` if it drifts. -- [ ] 6.3 Check the Colab notebooks for anything relying on a partial run exiting 0. -- [ ] 6.4 `uv run ruff check src/ packages/ tests/` + `uv run ruff format --check src/ packages/ tests/`. -- [ ] 6.5 `uv run pytest -m "not slow"` from `apps/protspace`. -- [ ] 6.6 `pnpm format:check` for the openspec markdown. -- [ ] 6.7 `openspec validate embed-completeness-contract --type change --strict`. +- [x] 6.2 Update the CLI table in `apps/protspace/CLAUDE.md` if it drifts. +- [x] 6.3 Check the Colab notebooks for anything relying on a partial run exiting 0. +- [x] 6.4 `uv run ruff check src/ packages/ tests/` + `uv run ruff format --check src/ packages/ tests/`. +- [x] 6.5 `uv run pytest -m "not slow"` from `apps/protspace`. +- [x] 6.6 `pnpm format:check` for the openspec markdown. +- [x] 6.7 `openspec validate embed-completeness-contract --type change --strict`. ## 7. Follow-ups filed, not fixed here From 37c2d64214b249f498946d726047dada0d4692ae Mon Sep 17 00:00:00 2001 From: tsenoner Date: Wed, 19 Aug 2026 13:01:12 +0200 Subject: [PATCH 7/9] docs(openspec): archive the embed completeness contract Merges the six requirements into openspec/specs/embed-completeness/ so the specs describe what the code now does, per AGENTS.md: archive on the branch, before the merge, or it does not happen and the next change reads stale specs as current. The archive step stamps a new capability's Purpose as a TBD placeholder that still validates clean, so it is hand-written here. Follow-ups filed rather than fixed: #471 (MDS infers the similarity-to-distance conversion from the diagonal), #472 (load_existing_ids disagrees with the loader's grouped view), #473 (embed and prepare write different .h5 keys). Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Achark1sp4iqreoQdBYc7y --- .../.openspec.yaml | 0 .../design.md | 0 .../proposal.md | 0 .../specs/embed-completeness/spec.md | 0 .../tasks.md | 6 +- openspec/specs/embed-completeness/spec.md | 179 ++++++++++++++++++ 6 files changed, 182 insertions(+), 3 deletions(-) rename openspec/changes/{embed-completeness-contract => archive/2026-08-19-embed-completeness-contract}/.openspec.yaml (100%) rename openspec/changes/{embed-completeness-contract => archive/2026-08-19-embed-completeness-contract}/design.md (100%) rename openspec/changes/{embed-completeness-contract => archive/2026-08-19-embed-completeness-contract}/proposal.md (100%) rename openspec/changes/{embed-completeness-contract => archive/2026-08-19-embed-completeness-contract}/specs/embed-completeness/spec.md (100%) rename openspec/changes/{embed-completeness-contract => archive/2026-08-19-embed-completeness-contract}/tasks.md (93%) create mode 100644 openspec/specs/embed-completeness/spec.md diff --git a/openspec/changes/embed-completeness-contract/.openspec.yaml b/openspec/changes/archive/2026-08-19-embed-completeness-contract/.openspec.yaml similarity index 100% rename from openspec/changes/embed-completeness-contract/.openspec.yaml rename to openspec/changes/archive/2026-08-19-embed-completeness-contract/.openspec.yaml diff --git a/openspec/changes/embed-completeness-contract/design.md b/openspec/changes/archive/2026-08-19-embed-completeness-contract/design.md similarity index 100% rename from openspec/changes/embed-completeness-contract/design.md rename to openspec/changes/archive/2026-08-19-embed-completeness-contract/design.md diff --git a/openspec/changes/embed-completeness-contract/proposal.md b/openspec/changes/archive/2026-08-19-embed-completeness-contract/proposal.md similarity index 100% rename from openspec/changes/embed-completeness-contract/proposal.md rename to openspec/changes/archive/2026-08-19-embed-completeness-contract/proposal.md diff --git a/openspec/changes/embed-completeness-contract/specs/embed-completeness/spec.md b/openspec/changes/archive/2026-08-19-embed-completeness-contract/specs/embed-completeness/spec.md similarity index 100% rename from openspec/changes/embed-completeness-contract/specs/embed-completeness/spec.md rename to openspec/changes/archive/2026-08-19-embed-completeness-contract/specs/embed-completeness/spec.md diff --git a/openspec/changes/embed-completeness-contract/tasks.md b/openspec/changes/archive/2026-08-19-embed-completeness-contract/tasks.md similarity index 93% rename from openspec/changes/embed-completeness-contract/tasks.md rename to openspec/changes/archive/2026-08-19-embed-completeness-contract/tasks.md index e37d2b6a..22ba84b4 100644 --- a/openspec/changes/embed-completeness-contract/tasks.md +++ b/openspec/changes/archive/2026-08-19-embed-completeness-contract/tasks.md @@ -59,10 +59,10 @@ ## 7. Follow-ups filed, not fixed here -- [ ] 7.1 File an issue for the `np.allclose(np.diag(data), 1)` heuristic in +- [x] 7.1 Filed #471 for the `np.allclose(np.diag(data), 1)` heuristic in `base_processor.py` — thread an explicit `is_similarity` flag from `compute_similarity` instead of inferring it from the diagonal. -- [ ] 7.2 File an issue for `load_existing_ids` vs the loader's grouped view: a +- [x] 7.2 Filed #472 for `load_existing_ids` vs the loader's grouped view: a grouped third-party `.h5` re-embeds everything and writes flat duplicates. -- [ ] 7.3 File an issue for `protspace embed` writing raw `sp|…` keys where +- [x] 7.3 Filed #473 for `protspace embed` writing raw `sp|…` keys where `prepare` writes parsed accessions. diff --git a/openspec/specs/embed-completeness/spec.md b/openspec/specs/embed-completeness/spec.md new file mode 100644 index 00000000..d00c8b15 --- /dev/null +++ b/openspec/specs/embed-completeness/spec.md @@ -0,0 +1,179 @@ +# embed-completeness Specification + +## Purpose + +When an embedding run is considered complete, which sequences may be skipped +rather than failed, how coverage against the source FASTA is verified, and what +each run reports. The rule is `expected = requested - skipped`, shared by both +the Biocentral and local backends so neither can drift from the other: a +documented capability limit is skipped and named, anything else absent from the +HDF5 fails the run. This exists because a truncated `.h5` projects, bundles and +scores completely normally, so a silently incomplete embedding yields plausible +numbers computed on a dataset missing part of its proteins. + +## Requirements + +### Requirement: An incomplete embedding exits non-zero + +Embedding SHALL exit non-zero when a sequence it attempted is absent from the +output HDF5, on every backend. The rule is `expected = requested − skipped`, and +the check reads the HDF5 rather than a running total, because the writer skips +identifiers already present and h5py turns an identifier containing `/` into a +group — so a counter can claim sequences the file does not hold. + +#### Scenario: The embedder returns fewer sequences than requested + +- **WHEN** a batch response omits sequences that were requested +- **THEN** the run raises `ValueError` naming the output path, how many of the + outstanding sequences were embedded, and how many are still missing +- **AND** the partial HDF5 is kept, so a rerun embeds only what is missing +- **AND** no `model_name` attribute is written and no affirmative save line is printed + +#### Scenario: Nothing at all was embedded + +- **WHEN** no sequence reached the HDF5 +- **THEN** the run raises `ValueError` distinguishing this from a partial run +- **AND** no HDF5 is fabricated by the caller writing the `model_name` attribute + +#### Scenario: A complete run succeeds + +- **WHEN** every requested sequence is present in the HDF5 +- **THEN** the run returns the path and exits zero + +#### Scenario: The failure type is catchable by the CLI + +- **WHEN** any completeness failure is raised +- **THEN** it is a `ValueError`, which `cli/embed.py` and `cli/prepare.py` already + catch to render `ERROR: ` and exit 1, rather than a `RuntimeError`, + which would escape those handlers as a raw traceback + +### Requirement: A documented capability limit is skipped, not failed + +Embedding SHALL treat a sequence excluded by a documented capability limit as +skipped rather than failed, and SHALL exit zero when every remaining sequence +embedded. A capability limit is a sequence longer than the configured maximum +length, or one that exhausts GPU memory at batch size 1. + +#### Scenario: A sequence longer than the maximum is skipped + +- **WHEN** the local backend is asked to embed a sequence longer than `--max-length` +- **THEN** that sequence is excluded from the attempt and named as skipped +- **AND** the run exits zero provided the remaining sequences all embedded + +#### Scenario: A sequence that exhausts GPU memory is skipped + +- **WHEN** the local backend exhausts GPU memory for a single sequence at batch size 1 +- **THEN** that sequence is recorded as skipped with that reason and named in the summary +- **AND** the run exits zero provided the remaining sequences all embedded + +#### Scenario: Skipping everything is still a failure + +- **WHEN** every requested sequence was skipped, so the HDF5 gained nothing +- **THEN** the run raises `ValueError` rather than reporting success + +#### Scenario: The progress bar counts only what was written + +- **WHEN** a batch fails or a sequence is skipped +- **THEN** the progress bar does not advance for it, so a run that embedded + nothing cannot render identically to one that embedded everything + +### Requirement: Every run reports what it embedded and skipped + +Embedding SHALL report every skipped sequence on stderr at warning level or above, +grouped by reason and naming the identifiers, and SHALL report the embedded and +skipped counts once per run. Skips go to stderr because a skipped sequence means +the dataset is incomplete even though the run succeeded, and the hosted prep +service discards the subprocess's stdout while keeping stderr; warning level or +above because the default verbosity shows nothing below it. + +#### Scenario: A run with skips names them + +- **WHEN** a run completes with one or more skipped sequences +- **THEN** each reason is reported on stderr with the identifiers it applies to, + previewing the first few when there are many +- **AND** the end-of-run line states how many sequences were embedded and how many + were skipped + +#### Scenario: A clean run is not made noisy + +- **WHEN** a run completes with nothing skipped +- **THEN** no skip warning is emitted, and the end-of-run line reports the + embedded count alone + +#### Scenario: The summary cannot be mistaken for a service outage + +- **WHEN** the summary or a completeness failure message is emitted +- **THEN** it contains none of the substrings the prep service matches to classify + a failure as `BIOCENTRAL_UNAVAILABLE`, so a coverage problem is never reported + to the user as an embedding-service outage + +### Requirement: Identifiers invalid for HDF5 are rejected before any work begins + +Embedding SHALL reject an identifier containing `/` on every backend, before +contacting the embedding service or loading a model. HDF5 treats `/` as a group +separator, so such an identifier silently becomes a group rather than a dataset +and the run cannot produce the requested key. + +#### Scenario: The remote backend rejects the identifier up front + +- **WHEN** the Biocentral backend is given an identifier containing `/` +- **THEN** it raises `ValueError` naming the offending identifiers before + submitting any batch, rather than embedding everything and then reporting an + unexplained shortfall + +#### Scenario: Both backends reject identically + +- **WHEN** either backend is given the same invalid identifier +- **THEN** the same error is raised, from the shared HDF5 layer + +### Requirement: The maximum sequence length is user-controllable + +The CLI SHALL expose the local backend's maximum sequence length as `--max-length` +on both `embed` and `prepare`, so that a skipped sequence is actionable. Without +it a user who is told a sequence was skipped has no way to embed it. + +#### Scenario: Raising the limit embeds a previously skipped sequence + +- **WHEN** a run skipped a sequence for exceeding the maximum length, and the user + reruns with a `--max-length` above that sequence's length +- **THEN** the sequence is attempted and, on success, written to the HDF5 + +#### Scenario: The option is rejected when not positive + +- **WHEN** `--max-length` is given a value below 1 +- **THEN** the CLI rejects it before loading a model + +### Requirement: Embeddings are checked against the supplied FASTA + +The pipeline SHALL compare embedding identifiers against the FASTA supplied with +`-f/--fasta`, normalising both sides through `parse_identifier`, and SHALL fail +when similarity is requested and any embedded protein is absent from that FASTA. +An uncovered protein leaves its similarity-matrix diagonal at zero, which defeats +the all-or-nothing diagonal test that triggers the similarity-to-distance +conversion, so a single uncovered protein inverts the entire MDS projection. + +#### Scenario: Uncovered embeddings block a similarity run + +- **WHEN** similarity is requested and one or more embedded proteins are absent + from the FASTA +- **THEN** the run fails with a message naming how many proteins are uncovered, + rather than producing an inverted projection + +#### Scenario: Uncovered embeddings warn without similarity + +- **WHEN** one or more embedded proteins are absent from the FASTA and similarity + is not requested +- **THEN** the run warns, naming the uncovered count, and continues + +#### Scenario: A FASTA covering more than the embeddings is not reported + +- **WHEN** the FASTA contains proteins that are absent from the embeddings +- **THEN** nothing is reported, because a resumed embedding cache legitimately + covers fewer proteins than the FASTA it was built from + +#### Scenario: Identifier styles are reconciled before comparing + +- **WHEN** the HDF5 keys and the FASTA headers use different identifier styles, + such as `sp|P12345|NAME` against `P12345` +- **THEN** both sides are normalised through `parse_identifier` before comparison, + so no protein is reported uncovered merely because of its key style From 5b72f4d13d5dfe969d865bc1274d7d51f1d72a7b Mon Sep 17 00:00:00 2001 From: tsenoner Date: Wed, 19 Aug 2026 16:23:56 +0200 Subject: [PATCH 8/9] test(embed): pin the --max-length rejection independently of terminal width The CLI assertion matched "backend local" in the rendered output, but typer renders a BadParameter inside a Rich panel that rewraps with the terminal width. It passed locally and failed on all three CI Python versions, where the 80-column panel splits the phrase across lines. Assert only the exit code at the CLI, and pin the message on build_embed_config directly. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Achark1sp4iqreoQdBYc7y --- apps/protspace/tests/test_backend_switch.py | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/apps/protspace/tests/test_backend_switch.py b/apps/protspace/tests/test_backend_switch.py index 7dee1be5..1d987a44 100644 --- a/apps/protspace/tests/test_backend_switch.py +++ b/apps/protspace/tests/test_backend_switch.py @@ -13,6 +13,7 @@ import h5py import numpy as np import pytest +import typer from typer.testing import CliRunner from protspace.cli.app import app @@ -326,8 +327,26 @@ def test_embed_cli_rejects_max_length_for_biocentral(tmp_path): ], ) + # Only the exit code is asserted here: the message is rendered inside a Rich + # panel, which rewraps with the terminal width, so matching on it is brittle. + # The message itself is pinned by the unit test below. assert result.exit_code != 0 - assert "backend local" in result.output + + +def test_build_embed_config_rejects_max_length_for_biocentral(): + """Pinned separately from the CLI so the assertion does not depend on how + Rich happens to wrap the panel at the current terminal width.""" + from protspace.cli.common_options import Backend, build_embed_config + + with pytest.raises(typer.BadParameter, match="backend local"): + build_embed_config(Backend.biocentral, max_length=512) + + +def test_build_embed_config_accepts_max_length_for_local(): + from protspace.cli.common_options import Backend, build_embed_config + + cfg = build_embed_config(Backend.local, batch_size=4, max_length=512) + assert (cfg.batch_size, cfg.max_length) == (4, 512) def test_embed_cli_rejects_nonpositive_max_length(tmp_path): From 0591944008d7a79786a36ccab125f8970a004d57 Mon Sep 17 00:00:00 2001 From: tsenoner Date: Wed, 19 Aug 2026 22:33:14 +0200 Subject: [PATCH 9/9] fix(prepare): make -f reach every HDF5 input and reject a path that is not there MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two gaps upstream of the FASTA coverage check, both of which let -f do nothing without saying so. Only the single-file branch attached `fasta_path`, so `-i dir/ -f x.fasta` loaded the embeddings and dropped the FASTA on the floor: coverage was still checked (that keys off the option, not the set), but `_extract_sequences` found nothing to extract and the bundle shipped without sequences. Both branches now produce the set and share one attach. Neither `-f` accepted `exists=True`, so a typo was swallowed by the `.exists()` guard in `_extract_sequences`. On `project` that was total silence — without `-s` the FASTA is never read, so a typo'd path exited 0 having quietly done nothing. `dir_okay=False` comes along for the ride; `parse_fasta` cannot read a directory either. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Achark1sp4iqreoQdBYc7y --- apps/protspace/docs/cli.md | 4 +- .../src/protspace/cli/common_options.py | 2 + apps/protspace/src/protspace/cli/prepare.py | 13 +- apps/protspace/src/protspace/cli/project.py | 2 + .../tests/test_embed_completeness.py | 121 ++++++++++++++++++ openspec/specs/embed-completeness/spec.md | 21 +++ 6 files changed, 157 insertions(+), 6 deletions(-) diff --git a/apps/protspace/docs/cli.md b/apps/protspace/docs/cli.md index 7da170e2..e340d378 100644 --- a/apps/protspace/docs/cli.md +++ b/apps/protspace/docs/cli.md @@ -185,7 +185,9 @@ asked for. > protein leaves its self-similarity at 0, which suppresses the > similarity-to-distance conversion for the whole matrix and inverts the MDS > projection. A FASTA covering *more* than the embeddings is normal and is not -> reported. +> reported. The same FASTA supplies the sequences carried into the bundle, and it +> applies to every HDF5 input — a directory of them as much as a single file. A +> `-f` path that does not exist is rejected outright rather than ignored. ## `protspace project` diff --git a/apps/protspace/src/protspace/cli/common_options.py b/apps/protspace/src/protspace/cli/common_options.py index 3fb51395..31ae8024 100644 --- a/apps/protspace/src/protspace/cli/common_options.py +++ b/apps/protspace/src/protspace/cli/common_options.py @@ -177,6 +177,8 @@ class Backend(StrEnum): "-f", "--fasta", help="FASTA for -s/--similarity when input is HDF5.", + exists=True, + dir_okay=False, rich_help_panel="Input", ), ] diff --git a/apps/protspace/src/protspace/cli/prepare.py b/apps/protspace/src/protspace/cli/prepare.py index dc505686..d2283b7f 100644 --- a/apps/protspace/src/protspace/cli/prepare.py +++ b/apps/protspace/src/protspace/cli/prepare.py @@ -479,13 +479,9 @@ def prepare( if not h5s: logger.warning(f"No embedding files in: {path}") continue - embedding_sets.append(load_h5(h5s, name_override=name_override)) + emb_set = load_h5(h5s, name_override=name_override) elif path.suffix.lower() in EMBEDDING_EXTENSIONS: emb_set = load_h5([path], name_override=name_override) - # Attach FASTA path from -f flag if provided (for sequence reuse) - if fasta_for_similarity: - emb_set.fasta_path = fasta_for_similarity - embedding_sets.append(emb_set) elif path.suffix.lower() in {".fasta", ".fa", ".faa"}: _embed_all( embedders, @@ -497,9 +493,16 @@ def prepare( force_reembed="embed" in refetch_stages, ) fasta_for_similarity = path + continue else: raise typer.BadParameter(f"Unsupported file: {path}") + # -f carries the sequences into the bundle, and it applies to + # every HDF5 input -- a directory of them as much as one file. + if fasta_for_similarity: + emb_set.fasta_path = fasta_for_similarity + embedding_sets.append(emb_set) + if not embedding_sets: raise typer.BadParameter("No valid input data found.") diff --git a/apps/protspace/src/protspace/cli/project.py b/apps/protspace/src/protspace/cli/project.py index db8101d1..413dd68b 100644 --- a/apps/protspace/src/protspace/cli/project.py +++ b/apps/protspace/src/protspace/cli/project.py @@ -56,6 +56,8 @@ def project( "-f", "--fasta", help="FASTA for -s/--similarity when input is HDF5.", + exists=True, + dir_okay=False, rich_help_panel="Input / Output", ), ] = None, diff --git a/apps/protspace/tests/test_embed_completeness.py b/apps/protspace/tests/test_embed_completeness.py index 0d4b6fd1..edb44404 100644 --- a/apps/protspace/tests/test_embed_completeness.py +++ b/apps/protspace/tests/test_embed_completeness.py @@ -141,3 +141,124 @@ def test_identifier_styles_are_reconciled(self, tmp_path, caplog): with caplog.at_level("WARNING"): check_fasta_coverage(fasta, ["P12345"], required=True) assert caplog.text == "" + + +class TestFastaOptionWiring: + """`-f/--fasta` reaches the pipeline, and a path that is not there says so. + + Both sit upstream of the coverage check: sequences that never get attached + cannot be checked, and a typo'd path used to be swallowed silently by the + ``.exists()`` guard in ``ReductionPipeline._extract_sequences``. + """ + + @staticmethod + def _inputs(tmp_path): + """A directory of embeddings plus the FASTA they came from.""" + d = tmp_path / "embs" + d.mkdir() + _write(d / "prot_t5.h5", ["P1", "P2"]) + with h5py.File(d / "prot_t5.h5", "a") as f: + f.attrs["model_name"] = "prot_t5" + fasta = tmp_path / "seqs.fasta" + fasta.write_text(">P1\nAAAA\n>P2\nCCCC\n") + return d, fasta + + @staticmethod + def _stub_pipeline(monkeypatch, captured): + """Capture the embedding sets instead of reducing and annotating them. + + Also keeps a regression here off the network: without it, a lost + ``exists=True`` would let the run reach the real annotation fetch. + """ + import protspace.data.processors.pipeline as pipeline_mod + + class _Capture: + def __init__(self, config): + pass + + def run(self, embedding_sets): + captured["sets"] = embedding_sets + + monkeypatch.setattr(pipeline_mod, "ReductionPipeline", _Capture) + + @staticmethod + def _prepare(d, fasta, tmp_path): + return [ + "prepare", + "-i", + str(d), + "-f", + str(fasta), + "-m", + "pca2", + "-o", + str(tmp_path / "out"), + "--no-scores", + "--no-log", + ] + + def test_fasta_reaches_a_directory_input(self, tmp_path, monkeypatch): + """``-i -f x.fasta`` must attach the FASTA, as ``-i `` does. + + Only the single-file branch attached it, so a directory of embeddings + got similarity but shipped a bundle carrying no sequences. + """ + from typer.testing import CliRunner + + from protspace.cli.app import app + + d, fasta = self._inputs(tmp_path) + captured: dict = {} + self._stub_pipeline(monkeypatch, captured) + + result = CliRunner().invoke(app, self._prepare(d, fasta, tmp_path)) + + assert result.exit_code == 0, result.output + assert [s.fasta_path for s in captured["sets"]] == [fasta] + + def test_prepare_rejects_a_fasta_that_is_not_there(self, tmp_path, monkeypatch): + """Same invocation as above, only the FASTA path is a typo. + + Pinned on exit code 2 -- the usage error typer raises for a path that + does not exist -- rather than merely non-zero: the coverage check would + also fail this run, at exit 1, from `parse_fasta` deep in the pipeline. + The point is that the typo is caught as a bad argument. Exit codes are + immune to the terminal width that reflows the message in its panel. + """ + from typer.testing import CliRunner + + from protspace.cli.app import app + + d, _ = self._inputs(tmp_path) + self._stub_pipeline(monkeypatch, {}) + + result = CliRunner().invoke( + app, self._prepare(d, tmp_path / "typo.fasta", tmp_path) + ) + + assert result.exit_code == 2, result.output + + def test_project_rejects_a_fasta_that_is_not_there(self, tmp_path): + """`project` swallowed it whole: without -s the FASTA is never read, so + a typo'd -f exited 0 having quietly done nothing with it.""" + from typer.testing import CliRunner + + from protspace.cli.app import app + + d, _ = self._inputs(tmp_path) + result = CliRunner().invoke( + app, + [ + "project", + "-i", + str(d / "prot_t5.h5"), + "-f", + str(tmp_path / "typo.fasta"), + "-m", + "pca2", + "-o", + str(tmp_path / "out"), + ], + ) + + assert result.exit_code == 2, result.output diff --git a/openspec/specs/embed-completeness/spec.md b/openspec/specs/embed-completeness/spec.md index d00c8b15..cedd7981 100644 --- a/openspec/specs/embed-completeness/spec.md +++ b/openspec/specs/embed-completeness/spec.md @@ -171,6 +171,27 @@ conversion, so a single uncovered protein inverts the entire MDS projection. - **THEN** nothing is reported, because a resumed embedding cache legitimately covers fewer proteins than the FASTA it was built from +### Requirement: A supplied FASTA must exist and reaches every HDF5 input + +The CLI SHALL reject a `-f/--fasta` path that does not exist, and SHALL attach the +supplied FASTA to every HDF5 input it loads, a directory of them as much as a +single file. A path read only behind an existence guard turns a typo into silence, +and attaching the FASTA to one input shape but not the other makes both the +coverage check and the sequences carried into the bundle depend on how the input +happened to be spelled. + +#### Scenario: A FASTA path that does not exist is a usage error + +- **WHEN** `-f/--fasta` names a path that is not on disk +- **THEN** the command rejects it as a bad argument, on both `prepare` and + `project`, rather than continuing with the FASTA silently unused + +#### Scenario: A directory of embeddings receives the FASTA + +- **WHEN** the input is a directory of HDF5 files and `-f/--fasta` is supplied +- **THEN** the FASTA is attached to the loaded embeddings, so its sequences reach + the bundle exactly as they do for a single HDF5 input + #### Scenario: Identifier styles are reconciled before comparing - **WHEN** the HDF5 keys and the FASTA headers use different identifier styles,