fix(protspace): derive missing sequence length from fasta

[FlorinSenoner]

Summary

• derive missing protein sequence lengths from matching local FASTA sequences
• preserve non-empty sequence lengths returned by UniProt
• document the behavior in an OpenSpec change and add regression coverage

Root cause

ReductionPipeline already parsed FASTA files and passed an identifier-to-sequence map into ProteinAnnotationManager. The manager used those local sequences only for InterPro and Biocentral requests, while the exported length field remained sourced exclusively from UniProt. An unmapped identifier therefore kept an empty UniProt length even though its FASTA sequence was available.

Fix

Normalize the primary annotation rows immediately after UniProt retrieval. When a row has an empty length and a matching non-empty local sequence, copy the row and fill length with the sequence's residue count. Existing UniProt lengths remain authoritative.

Reproduction

Against main (fbbaefa1):

1. Pass custom_protein with FASTA-derived sequence MPEPTIDE (8 residues) to the annotation manager.
2. Return an unmapped UniProt annotation row with an empty length.
3. Observe actual_length=''.

After this change, the same reproduction returns actual_length='8'.

Tests

• TDD RED: focused regression test failed with assert '' == '8'
• TDD GREEN: fallback and UniProt-precedence tests passed (2 passed)
• annotation manager module: 81 passed
• Ruff lint and format checks: passed
• Python non-slow suite: 789 passed, 6 deselected
• pnpm precommit: passed before commit and push
• strict OpenSpec validation: passed

Closes #336

[FlorinSenoner]

One cache-path correctness issue found.

[FlorinSenoner]

[P2] Apply the fallback before the complete-cache early return

This helper only runs once ProteinAnnotationManager.to_pd() is reached, but ReductionPipeline._fetch_annotations() returns cached_df directly when all_annotations.parquet already contains the requested columns (and protspace prepare keeps that cache by default). I reproduced this at this head with cached custom_protein,length='' plus FASTA MPEPTIDE: the pipeline logged Using cached annotations and returned '', not '8'. Users rerunning an existing output after upgrading therefore continue to see N/A. Route the complete-cache branch through the same enrichment (or invalidate/migrate cached empty lengths) and add a warm-cache pipeline regression test.

[FlorinSenoner]

Fixed in 0e000cb. I independently reproduced the complete-cache branch returning an empty length for cached custom_protein plus FASTA MPEPTIDE (cached_length='', expected 8). The cache-hit branch now enriches a copy of the selected DataFrame through the same missing-only scalar resolver used by ProteinAnnotationManager; non-empty cached lengths remain authoritative and all_annotations.parquet is not rewritten. The new warm-cache pipeline regression failed as ['', '110'] != ['8', '110'] before the fix and passes afterward. Fresh evidence: focused affected tests 160 passed; full Python suite 753 passed, 2 skipped; Ruff clean/143 formatted; strict OpenSpec valid; pnpm precommit passed; bundle contract 11 passed.

[tsenoner]

Automated review

Does it solve #336? Partially — the FASTA length fallback is correctly implemented in ProteinAnnotationManager and mirrored on the pipeline's complete-cache branch, and I traced all four cache branches of ReductionPipeline._fetch_annotations to confirm each ends up with the fallback applied. What it misses is the protspace annotate entry point, which parses the FASTA for identifiers but constructs the manager without sequences=, so unmapped IDs still get an empty length. That is the path the hosted prep backend uses for every FASTA uploaded at protspace.app, so the CLI prepare path is fixed and the browser upload path is not.

Found 2 issues:

1. protspace annotate builds the annotation manager without sequences=, so the new fallback never runs on that path — _fill_missing_fasta_lengths early-returns on if not self.sequences, and apps/prep/src/protspace_prep/pipeline.py invokes exactly this command (protspace annotate -i <fasta> -a <annotations>) on uploaded FASTAs, so a protspace.app user still sees N/A lengths for custom identifiers. The reporter's own bundle carries that signature: all 112 GT* rows have empty length and empty Biocentral predictions while all 37 UniProt-mapped rows are fully populated. The fix is local — parse the file with parse_fasta and normalise keys with parse_identifier (the same two calls ReductionPipeline._extract_sequences makes), then pass sequences=; or state the exclusion in the change's Non-Goals, since the spec delta currently says "The annotation pipeline SHALL derive..." without qualification.

protspace/apps/protspace/src/protspace/cli/annotate.py

Lines 98 to 104 in 0e000cb

	# Fetch annotations
	df = ProteinAnnotationManager(
	headers=headers,
	annotations=annotations_list,
	output_path=None,
	).to_pd()

2. The annotation reference still tells users that UniProt-sourced annotations cannot be filled from a -f FASTA, which is now wrong for length. The Input Requirements table marks the whole UniProt row No — accession needed, yet length is listed as a UniProt annotation in the default group and is exactly what now gets filled from -f on the prepare path — so a user hitting [FEATURE] Calculate sequence length from FASTA when no UniProt mapping exist #336 reads this page and concludes the capability does not exist. A footnote on the table plus a sentence in the length row (and in docs/guide/annotations.md L141-L147) closes it; CONTRIBUTING.md L227 asks for docs to stay synchronized with code.

protspace/apps/protspace/docs/annotations.md

Lines 21 to 27 in 0e000cb

	| Requirement | Sources | Works with `-f` FASTA? |
	| ----------- | ------- | ---------------------- |
	| **UniProt accession** | UniProt, Taxonomy, TED | No — accession needed |
	| **Protein sequence** | InterPro, Biocentral, Pfam CLANS | Yes — provide `-f` |
	If your H5 keys are not valid UniProt accessions (e.g., `NCBI|...`, custom IDs), accession-dependent annotations will be empty. Sequence-dependent annotations can still work if you provide the original FASTA file with `-f`.

🤖 Generated with Claude Code

_{Reviewed at 0e000cb against issue #336.}

[FlorinSenoner]

Independent triage at 0e000cbf764cff159f2e7e62cffa4c3f9835f942: both findings are actionable.

1. Confirmed: protspace annotate extracts normalized identifiers from FASTA but constructs ProteinAnnotationManager without sequences=; the hosted prep pipeline invokes that command with the normalized uploaded FASTA. Consequently the manager's FASTA-length fallback cannot run for that path. Required direction: build the normalized identifier-to-sequence map in the FASTA branch, pass it to the manager, and cover the standalone/hosted annotate path with a regression.
2. Confirmed: the annotation references still describe UniProt-backed data as unavailable from FASTA and describe length only as the canonical UniProt length, while the current prepare path and OpenSpec requirement define a missing-only FASTA fallback. Required direction: update both references to document the length exception and UniProt-over-FASTA precedence.

No fix was applied in this triage, and no thread was resolved.

[FlorinSenoner]

Implemented the two actionable findings in 5cab3198d6ee059a65502879437b5d1bbc546c91.

• protspace annotate now parses FASTA sequences, normalizes their identifiers with the existing parse_identifier policy, and passes the map to ProteinAnnotationManager; the real CLI regression covers the hosted command path and proved RED (length="") then GREEN (length="8").
• Both annotation references and the existing OpenSpec change now document missing-only FASTA fallback and UniProt-over-FASTA precedence.

Verification: affected tests 161 passed; ProtSpace non-slow suite 791 passed, 6 deselected; prep workspace tests and full Ruff checks passed; strict OpenSpec validation passed; pnpm precommit passed including generated-doc sync and VitePress build. No review thread state was changed.

[tsenoner]

Adversarial review

Reviewed in an isolated worktree by three independent lenses (code quality, adversarial correctness, issue-resolution audit), with every finding then put through a refuter whose default position was that it is a false positive. 8 raised, 4 survived refutation.

Applied and pushed (b5845824)

Behavior-preserving cleanups, verified green before pushing:

• apps/protspace/tests/test_annotate_cli.py: merged the function-local UniProtRetriever import (previously lines 14-16, inside test_annotate_fasta_derives_missing_length_from_normalized_sequence) into the existing module-level import from the same module at lines 5-7. Behavior-preserving: monkeypatch.setattr patches a class attribute, so import timing is irrelevant, and UniProtRetriever had no other reference in the file.

Issue resolution — partially resolves the issue

Scoping is correct and the author's stated root cause checks out against the code: the pipeline
already had the sequence map and the manager was using it only for InterPro/Biocentral
(manager.py:284-295 _build_sequence_map), so length stayed UniProt-only. Restricting the
change to length (rather than back-filling sequence, protein_name, etc.) matches the issue
text exactly, and "UniProt wins when non-empty" is the right default — it is tested at
tests/test_annotation_manager.py and at the warm-cache level.
The commit history shows good triage under review: the initial commit fixed only the manager, and
the two follow-ups correctly identified that (a) the complete-cache early return at pipeline.py:420
bypasses the manager entirely on warm reruns, and (b) protspace annotate — the path the hosted
prep backend actually invokes (apps/prep/src/protspace_prep/pipeline.py:116-123) — never passed
sequences. Both are genuine, non-obvious holes and both are now closed with tests. The design.md's
justification for enriching at the manager boundary rather than post-formatting or inside
UniProtRetriever is sound.
Where the triage falls short is the precondition it takes for granted. design.md lists "Identifier
mismatch prevents fallback" as a risk and resolves it with "continue using the pipeline's existing
parse_identifier normalization and require an exact key match" — but that is only safe because
protspace embed pre-normalizes its H5 keys. load_h5 does not normalize keys it did not create
(h5.py:135), so for a bring-your-own H5 with piped keys the two sides disagree and the fallback no-
ops silently. I verified this rather than inferring it: the manager returns length == '' for
headers NCBI|WP_12345 with FASTA >NCBI|WP_12345. The change also updates
apps/protspace/docs/annotations.md to promise the fallback works via -f in the same paragraph that
uses NCBI|... as its example — so the documented claim is broader than the delivered behavior.
Nothing in the PR is wrong; the risk was named and then under-verified.
On "Closes #336": defensible but slightly overstated. The dominant user path for this symptom —
upload a custom FASTA at protspace.app — is genuinely fixed, as are prepare -i x.fasta and
prepare -i x.h5 -f x.fasta. Three named paths (directory input with -f, annotate -i x.h5,
piped external H5 keys) still emit N/A. I would keep "Closes #336" only if a follow-up issue is
opened for the identifier-normalization gap and the annotate -i <h5> FASTA option; otherwise
"Related to #336" is the more honest label. The */- counting issue should be fixed before merge
regardless — it converts a visible N/A into a silently wrong number on the exact hosted path the PR
targets.

Gaps found by the issue audit (7)

• Identifier mismatch silently disables the fallback for externally-produced H5 files whose keys contain pipes — the exact NCBI|... case named in the doc paragraph this PR edits. load_h5 keeps the raw key (apps/protspace/src/protspace/data/loaders/h5.py:135, header = raw_key), while _extract_sequences normalizes FASTA headers through parse_identifier (apps/protspace/src/protspace/data/processors/pipeline.py:306). I reproduced this: H5 key NCBI|WP_12345 + >NCBI|WP_12345 FASTA yields sequences == {'WP_12345': 'MPEPTIDE'} but protein.identifier == 'NCBI|WP_12345', so to_pd() returns length == '' — still N/A.
  • Why it matters: apps/protspace/docs/annotations.md now states "the missing-length fallback can still work if you provide the original FASTA file with -f" and, two lines earlier, uses NCBI|... as the canonical example of non-UniProt H5 keys. For that user the promise is false. (The same mismatch already silently breaks InterPro/Biocentral sequence reuse via _build_sequence_map, so this is pre-existing — but the PR newly documents it as working.)
  • Suggested follow-up: Either normalize self.headers with parse_identifier at the same boundary that normalizes sequences, or fall back to a parse_identifier(identifier) lookup inside _resolve_fasta_sequence_length. Add a regression with a piped H5 key + matching FASTA header, and if it is deliberately out of scope, soften the -f claim in apps/protspace/docs/annotations.md.
• protspace annotate -i <h5> — the staged workflow documented at apps/protspace/docs/cli.md:169 (protspace annotate -i embeddings/prot_t5.h5 -a default -o annotations.parquet) — has no -f/--fasta option. cli/annotate.py:72-77 leaves sequences = None for the HDF5 branch, so the fallback can never fire there.
  • Why it matters: Users who run the four stages separately (embed → annotate → project → bundle), which is what the docs show and what the hosted service does internally, get the fix only if they happen to pass the FASTA to annotate. With an H5 input there is no way to.
  • Suggested follow-up: Add an optional -f/--fasta to protspace annotate that populates sequences for the HDF5 branch (mirroring prepare's -f), and document it next to the cli.md example.
• protspace prepare -i <directory-of-h5> -f seqs.fasta never attaches the FASTA. The directory branch at apps/protspace/src/protspace/cli/prepare.py:489-496 calls embedding_sets.append(load_h5(h5s, ...)) without the emb_set.fasta_path = fasta_for_similarity assignment that the single-file branch does at prepare.py:498-501. _extract_sequences therefore returns {}.
  • Why it matters: Directory input is a supported form of -i, and -f is the only way to supply sequences there. The fallback silently no-ops with no warning, indistinguishable from "no FASTA given".
  • Suggested follow-up: Set emb_set.fasta_path in the directory branch too (one line), and add a test asserting _extract_sequences is non-empty for -i dir -f fasta.
• len(sequence) counts every character, including * (terminator) and - (gap). apps/prep/src/protspace_prep/validation.py:37 explicitly admits both into _PROTEIN_ALPHABET, and the prep service writes the raw upload to disk (jobs.py:120-121) with only headers rewritten, so those characters survive into parse_fasta.
  • Why it matters: A translated-CDS FASTA with trailing *, or an aligned FASTA with gaps, produces a length that is systematically wrong (off-by-one to badly inflated) while looking authoritative — on the hosted path this PR specifically targets. It is worse than N/A because it is silently incorrect.
  • Suggested follow-up: Count residues rather than characters (strip * and -, e.g. len(sequence.strip('*').replace('-', ''))) and state the counting rule in the spec's "Unmapped protein has a FASTA sequence" scenario.
• _fill_missing_fasta_lengths will raise TypeError: 'NoneType' object is not iterable when sources_to_fetch['uniprot'] is False and cached_data is None while sequences is non-empty — manager.py:140-145 assigns uniprot_annotations = cached_uniprot, which is None in that combination, and only the not self.sequences early return guards the loop.
  • Why it matters: ProteinAnnotationManager is a public export (protspace.data.__all__), so this is reachable by any external caller that passes sources_to_fetch without cached_data. No in-repo path hits it today, so no test would catch a regression.
  • Suggested follow-up: Change the guard to if not self.sequences or not proteins: return proteins, and add a unit test for the sources_to_fetch={'uniprot': False} + cached_data=None combination.
• packages/utils/src/visualization/annotation-metadata.ts:146-152 still declares length as source: 'UniProt'. That field drives the annotation-dropdown grouping and the docs popover attribution, and the file's own header comment calls it "the single source of truth" that must "be kept in sync with the backend reference when the annotation set changes".
  • Why it matters: The generated long-form docs were updated (docs/scripts/annotation-details.ts, docs/guide/annotations.md), but the in-app attribution now over-claims: a FASTA-derived length is shown to the user as UniProt-sourced. Low severity, but it is exactly the "metadata not updated in step" class of gap.
  • Suggested follow-up: Either leave source: 'UniProt' deliberately (primary source) and note that decision in the OpenSpec design, or extend the short description to mention the local-FASTA fallback so the popover matches the generated page.
• When cached UniProt rows do not carry a length key at all (possible via determine_sources_to_fetch returning uniprot: False for a request whose required set excludes length while the cache also lacks it), {**protein.annotations, 'length': resolved} appends a new key rather than filling one. AnnotationWriter.write_parquet derives its header list from proteins[0].annotations.keys() (apps/protspace/src/protspace/data/io/writers.py:84), so the column only survives if the first protein happened to get a sequence.
  • Why it matters: Narrow, but it means the enriched column is either silently dropped or silently persisted into all_annotations.parquet depending on which protein sorts first — order-dependent output from a caching layer.
  • Suggested follow-up: Only fill when 'length' in protein.annotations, i.e. treat a missing key as "this source does not carry length" rather than "empty length".

Findings needing a decision (3)

These were left for you rather than auto-applied: each changes behavior, needs a product call, or reaches outside this diff.

1. The FASTA input is now read and header-parsed twice in the same function — once by extract_identifiers_from_fasta and again by parse_fasta.

apps/protspace/src/protspace/cli/annotate.py:68 · medium · efficiency

extract_identifiers_from_fasta (data/loaders/query.py:85-95) opens the file, scans every line and
applies parse_identifier(line[1:].strip().split()[0]); the new parse_fasta(input) call
immediately re-opens the same file, re-scans every line and re-derives the identical header token,
after which the diff applies parse_identifier to it a second time. For a Swiss-Prot-scale input
(~570K records, hundreds of MB) that is a full extra pass over the file and a full extra header
parse for zero new information — sequences.keys() is already exactly headers (modulo dedup). It
also leaves two header-parsing implementations with slightly different semantics live in one
function.

Suggested fix

In apps/protspace/src/protspace/cli/annotate.py replace lines 65-72:
if is_fasta_file(input):
sequences = {
parse_identifier(header): sequence
for header, sequence in parse_fasta(input).items()
}
headers = list(sequences)
and delete the now-unused function-local from protspace.data.loaders.query import extract_identifiers_from_fasta import. Before applying, confirm the two intended behaviour deltas:
(a) duplicate FASTA headers are collapsed to one query/row instead of being queried once per
occurrence, and (b) records with a zero-length sequence no longer appear in headers at all
(previously they were annotated with an empty length). If (b) must be preserved, keep those
identifiers explicitly rather than re-reading the file.

2. _fill_missing_fasta_lengths inserts a length key into rows that never had one, producing heterogeneous annotation dicts; since DataFormatter.to_dataframe / AnnotationWriter.write_parquet build their column list from proteins[0].annotations.keys() only, the derived lengths are silently discarded (or a whole column appears) depending on whether the first protein happens to be in the FASTA.

apps/protspace/src/protspace/data/annotations/manager.py:222 · medium · correctness

protein.annotations.get("length") returns None when the key is absent, which is falsy, so the
helper adds the key instead of only filling an existing empty value. This is reachable through
_fetch_uniprot's failure fallback, which returns ProteinAnnotations(identifier=header, annotations={"organism_id": ""}) — no length key at all. Verified by running the real manager
(UniProtRetriever patched to raise): headers ["A","B","C"], sequences={"B":"MPEPTIDE","C":"MM"},
annotations=["length"] -> output DataFrame has only an identifier column; B's and C's
derived lengths are dropped because row 0 (A) has no FASTA sequence. Same run with
sequences={"A":...,"B":...,"C":...} -> DataFrame gains length = 8/8/2. So with UniProt
unreachable, prepare -i emb.h5 -f seqs.fasta -a length either emits lengths for everyone or for no
one purely as a function of whether the first header is present in the FASTA — silent, order-
dependent data loss. Before this PR that path consistently produced no length column, so the new
behavior is non-deterministic rather than merely incomplete.

Suggested fix

Preferred (keeps the PR's benefit and makes rows uniform): in
apps/protspace/src/protspace/data/annotations/manager.py:255-258, change _fetch_uniprot's failure
fallback from annotations={"organism_id": ""} to annotations=dict.fromkeys(UNIPROT_ANNOTATIONS, ""), matching what UniProtRetriever already does for invalid headers and batch failures.
Alternative (minimal, suppresses the fill on that path): in _fill_missing_fasta_lengths
(manager.py:220-241), guard on key presence so the row key-sets stay uniform:
for protein in proteins:
if "length" not in protein.annotations:
result.append(protein)
continue
length = protein.annotations["length"]
resolved_length = _resolve_fasta_sequence_length(
protein.identifier, length, self.sequences
)
...
Both leave the PR's two new manager tests green (they mock rows that already carry a length key).

3. The warm-cache length fill runs before the "All cached annotations are empty" check, so the warning is suppressed in exactly the scenario it was written for.

apps/protspace/src/protspace/data/processors/pipeline.py:425 · low · correctness

The emptiness probe at pipeline.py:444 is api_df[data_cols].apply(lambda col: (col != "").any())
and it now runs on the filled frame. Scenario: a --keep-tmp run over non-UniProt identifiers
(e.g. NCBI|... keys) produced all_annotations.parquet where every column is ""; the user
reruns prepare -i emb.h5 -f seqs.fasta --keep-tmp with the same annotation set. Cache is complete,
so the warm branch is taken; length is now populated from the FASTA, non_empty.any() becomes
True, and the "All cached annotations are empty. This may be from a previous run with non-UniProt
identifiers. Use --refetch annotations to re-fetch..." warning is never logged. The user ships a
bundle in which every annotation except length is empty, with no diagnostic — previously they were
told to use --refetch.

Suggested fix

In apps/protspace/src/protspace/data/processors/pipeline.py, capture the frame before the fill and
probe that instead. Insert pre_fill_df = api_df immediately before line 425 (if "length" in api_df.columns and sequences:), then change lines 442-446 from data_cols = [c for c in api_df.columns if c != "identifier"] / non_empty = api_df[data_cols].apply(...) to use
pre_fill_df:
data_cols = [c for c in pre_fill_df.columns if c != "identifier"]
if data_cols:
non_empty = pre_fill_df[data_cols].apply(
lambda col: (col != "").any()
)
The api_df = api_df.copy() inside the fill guarantees pre_fill_df still holds the un-filled
values. Add a pipeline test asserting the warning still fires for an all-empty cache when a FASTA is
supplied.

4 further finding(s) were raised and refuted during verification.

[FlorinSenoner]

Addressed the adversarial review at #401 (comment) in 5826d8d9c9512af8660bec3ac2e853b52da995f6.

1. Piped external-H5 identifiers: no normalization change. The active design explicitly keeps exact matching and excludes a second identifier-normalization policy. I corrected the broader documentation claim: -f fallback is now documented as requiring H5 keys that match the identifiers parsed from FASTA.
2. protspace annotate -i <h5> -f: not added. This is a new public CLI option, while the active design explicitly states that HDF5 input to the standalone annotate command supplies no local sequences. The supported combined path remains protspace prepare -i <h5> -f <fasta>.
3. Directory HDF5 input drops -f: fixed. The directory branch now attaches the supplied FASTA path to the loaded embedding set, matching the single-HDF5 branch. Regression: fasta_path=None before, expected path after.
4. * / - counting: fixed. FASTA-derived length now counts amino-acid residues and excludes terminator/gap markers. Regression: M-PEP* returned 6 before and 4 after. The counting rule is recorded in OpenSpec and both annotation references.
5. sources_to_fetch["uniprot"]=False with no cache: no guard added. Returning None unchanged only moves the same failure to AnnotationMerger.merge, which must iterate primary rows; disabling UniProt without cached primary data is not a valid complete input. Supporting that state needs a separate contract decision rather than a guard that masks the root cause.
6. Frontend source metadata: kept source: "UniProt" because that field drives primary-source dropdown grouping, but updated the visible description to state the local-FASTA fallback. Generated docs were synchronized.
7. Missing length key / heterogeneous failure rows: fixed. A top-level UniProt failure now emits the complete UNIPROT_ANNOTATIONS schema, matching invalid-ID and batch-failure paths. Regression proved the length column previously disappeared when the first row had no matching FASTA sequence; it now remains uniform and yields ["", "8"].
8. Double FASTA read: unchanged. The two existing parsers preserve different behavior: identifier extraction retains duplicate and empty-sequence records, while parse_fasta intentionally collapses duplicates and skips empty sequences. Replacing both with list(sequences) would change output cardinality without evidence of a bottleneck.
9. Order-dependent formatter finding: duplicate of item 7 and addressed by the same schema-uniform failure rows.
10. Warm-cache warning timing: unchanged. The warning asks whether the output-ready cached annotation set is entirely empty. After a valid FASTA length is filled, that predicate is false; probing the pre-fill frame would produce a false warning when length is the only requested annotation.

Verification:

• RED: marker count 6 != 4; missing length column on UniProt failure; directory HDF5 fasta_path=None.
• GREEN regressions: 3 passed.
• Affected Python modules: 174 passed.
• ProtSpace non-slow suite: 794 passed, 6 deselected.
• Hosted prep suite and full Ruff check/format gates: passed.
• Utils suite: 321 passed.
• Strict OpenSpec validation, generated annotation-doc check, and repository-required pnpm precommit (typecheck, knip, dependency check, docs sync/build): passed.

No review thread was resolved and no PR metadata was changed.