diff --git a/apps/protspace/CLAUDE.md b/apps/protspace/CLAUDE.md index e384661e..d9c3fbec 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 | @@ -156,6 +156,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/ @@ -280,9 +281,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), the notebook-gating sets pinned to the registry each constrains (`COLAB_OVERSIZED`→`LOCAL_CHECKPOINTS`, `BIOCENTRAL_INVALID`→`ALL_SHORT_KEYS`), 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), the notebook-gating sets pinned to the registry each constrains (`COLAB_OVERSIZED`→`LOCAL_CHECKPOINTS`, `BIOCENTRAL_INVALID`→`ALL_SHORT_KEYS`), 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/src/protspace/cli/common_options.py b/apps/protspace/src/protspace/cli/common_options.py index d3e2b240..7f3fe10f 100644 --- a/apps/protspace/src/protspace/cli/common_options.py +++ b/apps/protspace/src/protspace/cli/common_options.py @@ -202,6 +202,18 @@ def require_similarity_extra() -> None: ), ] +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, @@ -209,6 +221,38 @@ def require_similarity_extra() -> None: "-f", "--fasta", help="FASTA for -s/--similarity when input is HDF5.", + exists=True, + dir_okay=False, 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 e98364cd..1dc980c6 100644 --- a/apps/protspace/src/protspace/cli/embed.py +++ b/apps/protspace/src/protspace/cli/embed.py @@ -13,7 +13,9 @@ Backend, Opt_Backend, Opt_BatchSize, + Opt_MaxLength, Opt_Verbose, + build_embed_config, ) logger = logging.getLogger(__name__) @@ -47,6 +49,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. @@ -68,29 +71,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 11cf3d40..bcea8564 100644 --- a/apps/protspace/src/protspace/cli/prepare.py +++ b/apps/protspace/src/protspace/cli/prepare.py @@ -32,6 +32,7 @@ Opt_FpRatio, Opt_LearningRate, Opt_MaxIter, + Opt_MaxLength, Opt_Methods, Opt_Metric, Opt_MinDist, @@ -42,6 +43,7 @@ Opt_RandomState, Opt_Similarity, Opt_Verbose, + build_embed_config, require_similarity_extra, ) @@ -295,6 +297,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, @@ -422,22 +425,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 @@ -486,13 +474,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, @@ -504,12 +488,31 @@ 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.") + # --- 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 --- # Both preconditions were checked before any input was read, so # `fasta_for_similarity` is set here whenever `similarity` is on. diff --git a/apps/protspace/src/protspace/cli/project.py b/apps/protspace/src/protspace/cli/project.py index 8272fc14..7b12a7f2 100644 --- a/apps/protspace/src/protspace/cli/project.py +++ b/apps/protspace/src/protspace/cli/project.py @@ -57,6 +57,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, @@ -115,6 +117,9 @@ def project( raise typer.BadParameter("No valid HDF5 files found.") if similarity: + 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/embedding/biocentral.py b/apps/protspace/src/protspace/data/embedding/biocentral.py index 5cf6a745..0ccb30b4 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. @@ -114,27 +122,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 # --------------------------------------------------------------------------- @@ -158,6 +145,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: @@ -286,33 +276,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 105445e8..c194dd35 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__) @@ -167,19 +172,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) # --------------------------------------------------------------------------- @@ -296,21 +288,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 @@ -343,13 +331,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 @@ -362,13 +348,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 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) diff --git a/apps/protspace/tests/test_backend_switch.py b/apps/protspace/tests/test_backend_switch.py index 9a4d58d7..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 @@ -273,3 +274,100 @@ 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", + ], + ) + + # 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 + + +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): + 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_cli_no_similarity.py b/apps/protspace/tests/test_cli_no_similarity.py index 4b319bf0..69312bd9 100644 --- a/apps/protspace/tests/test_cli_no_similarity.py +++ b/apps/protspace/tests/test_cli_no_similarity.py @@ -78,14 +78,18 @@ def test_cli_rejects_similarity_before_doing_work(command, tmp_path, monkeypatch _stub_pymmseqs(monkeypatch, None) # -f satisfies the sibling precondition (-s on HDF5 input needs a FASTA), so - # the extra guard is what this asserts on. Neither path is read. + # the extra guard is what this asserts on. It has to be a real file -- -f is + # validated as an existing path -- but nothing reads it, and `missing.h5` + # still does not exist. + fasta = tmp_path / "seqs.fasta" + fasta.write_text(">P1\nAAAA\n") args = [ command, "-i", str(tmp_path / "missing.h5"), "-s", "-f", - str(tmp_path / "missing.fasta"), + str(fasta), "-o", str(tmp_path), ] diff --git a/apps/protspace/tests/test_embed_completeness.py b/apps/protspace/tests/test_embed_completeness.py new file mode 100644 index 00000000..edb44404 --- /dev/null +++ b/apps/protspace/tests/test_embed_completeness.py @@ -0,0 +1,264 @@ +"""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 == "" + + +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/apps/protspace/tests/test_local_embedder.py b/apps/protspace/tests/test_local_embedder.py index 16f1e5dc..3bc79606 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 @@ -159,7 +160,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", @@ -211,3 +212,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), + ) diff --git a/docs/guide/python-cli.md b/docs/guide/python-cli.md index bcc534a7..3e3b4e43 100644 --- a/docs/guide/python-cli.md +++ b/docs/guide/python-cli.md @@ -94,11 +94,12 @@ protspace prepare -i emb.h5 -m "umap2:n_neighbors=15" -m "umap2:n_neighbors=50" ### Embedding -| Flag | Description | Default | -| ---------------- | ----------------------------------------------------------------------------------------- | ------------ | -| `-e, --embedder` | pLM model(s), comma-separated. See [Embedder models](#embedder-models). | `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 (Biocentral call) or 8 (local GPU). | - | +| Flag | Description | Default | +| ---------------- | --------------------------------------------------------------------------------------------------------- | ------------ | +| `-e, --embedder` | pLM model(s), comma-separated. See [Embedder models](#embedder-models). | `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 (Biocentral call) or 8 (local GPU). | - | +| `--max-length` | Skip sequences longer than this (`--backend local` only). Skipped sequences are named in the run summary. | `2000` | `-e` requires FASTA input or `-q/--query`; it is rejected with HDF5-only input. When a FASTA is given without `-e`, `prot_t5` is used. @@ -398,9 +399,42 @@ protspace embed -i sequences.fasta -e prot_t5 -e esm2_3b -o embeddings/ # On-device GPU/CPU, works offline 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 ``` -`-i`, `-e` and `-o` are required. `--backend` and `--batch-size` behave as in `prepare`. +`-i`, `-e` and `-o` are required. `--backend`, `--batch-size` and `--max-length` behave as in +`prepare`. + +### 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. 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/openspec/changes/archive/2026-08-19-embed-completeness-contract/.openspec.yaml b/openspec/changes/archive/2026-08-19-embed-completeness-contract/.openspec.yaml new file mode 100644 index 00000000..41c30bab --- /dev/null +++ b/openspec/changes/archive/2026-08-19-embed-completeness-contract/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-08-19 diff --git a/openspec/changes/archive/2026-08-19-embed-completeness-contract/design.md b/openspec/changes/archive/2026-08-19-embed-completeness-contract/design.md new file mode 100644 index 00000000..2a3953ca --- /dev/null +++ b/openspec/changes/archive/2026-08-19-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/archive/2026-08-19-embed-completeness-contract/proposal.md b/openspec/changes/archive/2026-08-19-embed-completeness-contract/proposal.md new file mode 100644 index 00000000..2eecd31e --- /dev/null +++ b/openspec/changes/archive/2026-08-19-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/archive/2026-08-19-embed-completeness-contract/specs/embed-completeness/spec.md b/openspec/changes/archive/2026-08-19-embed-completeness-contract/specs/embed-completeness/spec.md new file mode 100644 index 00000000..8efd4760 --- /dev/null +++ b/openspec/changes/archive/2026-08-19-embed-completeness-contract/specs/embed-completeness/spec.md @@ -0,0 +1,166 @@ +## 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 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 diff --git a/openspec/changes/archive/2026-08-19-embed-completeness-contract/tasks.md b/openspec/changes/archive/2026-08-19-embed-completeness-contract/tasks.md new file mode 100644 index 00000000..22ba84b4 --- /dev/null +++ b/openspec/changes/archive/2026-08-19-embed-completeness-contract/tasks.md @@ -0,0 +1,68 @@ +## 1. Shared HDF5 layer + +- [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()`. +- [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. +- [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 + +- [x] 2.1 `biocentral.py`: call `validate_headers` before the first batch; replace + the inline gate with `finish_run(..., skipped={})`. +- [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. +- [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` + +- [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 + +- [x] 4.1 Add the coverage check to `data/processors/pipeline.py`, normalising both + sides through `parse_identifier`. +- [x] 4.2 Fail when similarity is requested and any embedded protein is uncovered; + warn otherwise; say nothing when the FASTA is a superset. +- [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 + +- [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. +- [x] 5.2 `test_biocentral_embedder.py`: `/` now rejected up front; the disk gate + still catches a writer that under-delivers. +- [x] 5.3 New coverage tests: uncovered + similarity fails, uncovered alone warns, + superset silent, mixed identifier styles reconcile. +- [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 + +- [x] 6.1 Update `apps/protspace/docs/cli.md` and `README.md` for `--max-length` + and the completeness/coverage behaviour. +- [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 + +- [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. +- [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. +- [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..cedd7981 --- /dev/null +++ b/openspec/specs/embed-completeness/spec.md @@ -0,0 +1,200 @@ +# 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 + +### 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, + 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