fix(embed): one completeness contract for both backends, and a FASTA coverage check - #474
Merged
Conversation
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Achark1sp4iqreoQdBYc7y
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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Achark1sp4iqreoQdBYc7y
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Achark1sp4iqreoQdBYc7y
…larity 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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Achark1sp4iqreoQdBYc7y
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Achark1sp4iqreoQdBYc7y
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Achark1sp4iqreoQdBYc7y
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Achark1sp4iqreoQdBYc7y
… 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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Achark1sp4iqreoQdBYc7y
…s not there 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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Achark1sp4iqreoQdBYc7y
#430 merged and semantic-release cut 4.11.3, so this branch had to come forward onto a main that had also consolidated the docs and hardened the similarity path. Four conflicts, all resolved toward main's structure: - `cli/prepare.py`, `cli/project.py`: main hoisted the "-s requires a FASTA" precondition to the top of both commands, alongside the new `require_similarity_extra()`. Dropped this branch's duplicate guard in `project` and kept only the coverage check, which still runs immediately before `compute_similarity`. - `docs/cli.md`: main reduced it to a pointer at the documentation site, so the `--max-length` row, the "When embedding fails" section and the `-f` coverage note move to `docs/guide/python-cli.md` where the reference now lives. - `apps/protspace/CLAUDE.md`: both sides rewrote the `test_local_embedder.py` row; merged the two descriptions. `test_cli_no_similarity.py` passed a deliberately absent `-f` path to satisfy the sibling precondition without touching disk. `-f` now validates as an existing path, so that argument was rejected before the mmseqs2 guard the test is about; it writes a real FASTA instead, and `missing.h5` still proves nothing was loaded first. 904 passed (`-m "not slow"`), ruff clean, all 24 specs valid. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Achark1sp4iqreoQdBYc7y
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #430 — base is
fix/embed-silent-partial-failure, so merge #430 first. Do not squash (apps/protspace/is touched).Why
#430 made the Biocentral backend fail on an incomplete embedding. The local backend still did not: it dropped every sequence over
max_length(2000 aa, not exposed on the CLI) and every sequence that OOMed at batch size 1, logged a warning, and exited 0 as long as one embedding landed.resolve_default_backend()returns"local"on any Colab GPU runtime — so the unguarded backend was the default for exactly the audienceBIOCENTRAL_UNAVAILABLEroutes to Colab.The divergence was accretion, not design. The tell is
local.py's import line:One backend importing its HDF5 layer from its sibling backend means a shared layer already existed — it just had no home, so the parts that were not shared drifted.
The contract
expected = requested − skipped, in a newdata/embedding/store.pyowned by neither backend./in an identifiervalidate_headers, both backends, before any work> max_lengthSkips go to stderr at warning level (the hosted prep service discards stdout and passes no
-v); the counts go to the end-of-run line; a clean run stays quiet. Every new message avoids all eight_BIOCENTRAL_DOWN_PATTERNSsubstrings, so a coverage problem can never be re-tagged as a service outage and routed to Colab, which would not fix it.--max-lengthis now exposed onembedandprepare, and rejected for--backend biocentral(no length cap — silently ignoring it would let a user believe they had raised a limit that does not exist). Without a lever, "skipped 3 sequences" is a dead end.The FASTA check is not cosmetic
Nothing anywhere compared an embedding set to the FASTA it came from.
compute_similarityzero-fills the row for a protein it cannot find, leaving its self-similarity at 0, andbase_processoronly converts similarity → distance whennp.allclose(np.diag(data), 1). One uncovered protein makes that false for the whole matrix, so MDS consumes raw similarities as distances:Near-identical proteins placed furthest apart, silently, for every pair.
The check is directional.
h5 − fastais the damaging direction and the only one reported;fasta − h5is routine, because a resumed embedding cache legitimately covers fewer proteins than the FASTA it was built from. Uncovered proteins fail under-s/--similarityand warn otherwise — failing outright would break deliberately projecting a subset, warning alone would let the inverted bundle ship. Both sides go throughparse_identifier, sinceload_h5keeps raw HDF5 keys while FASTA ids are always parsed. Placed beforecompute_similarityin bothprepareandproject;projecthad the same exposure and no plumbing for it.Verification
880 passed, 2 skipped;-m "not slow":876 passed. +24 tests.ruff check+ruff format --checkclean oversrc/ packages/ tests/.stdout=DEVNULL, stderr=PIPEexactly asapps/prep/_run_stepinvokes it — exit 1 and the message lands on stderr where the service captures it.model_namestamped only on the successful run.Scope
OpenSpec change
embed-completeness-contract(proposal, design, spec deltas, 29 tasks), archived on this branch perAGENTS.mdsoopenspec/specs/is true at merge.Deliberately not fixed here, filed instead:
np.allclose(np.diag(data), 1)heuristic itself. This PR stops a partially-covered FASTA reaching it; it does not make it robust.load_existing_ids(flat) disagrees with the loader's grouped view: a grouped third-party.h5re-embeds everything and silently swaps stale for fresh.embedwrites rawsp|…keys wherepreparewrites parsed accessions.Two gaps in the
-fwiring, now closedBoth sat upstream of the coverage check and let
-fdo nothing without saying so.fasta_pathwas attached only in the single-file branch, so-i dir/ -f x.fastadropped the FASTA on the floor. Coverage was still checked — that keys off the option, not the set — but_extract_sequencesfound nothing and the bundle shipped without sequences. Both branches now build the set and share one attach.Neither
-fhadexists=True, so a typo was swallowed by the.exists()guard in_extract_sequences. Onprojectthat was total silence: without-sthe FASTA is never read, so a typo'd path exited 0 having quietly done nothing with it. Now a usage error (exit 2) on both commands, which is what the added tests pin — exit codes, not panel text that reflows with terminal width.dir_okay=Falsecomes along;parse_fastacannot read a directory either.