fix(notebook): recompute projections on every generate

[FlorinSenoner]

Summary

• recompute dimensionality-reduction projections on every Preparation notebook Generate action
• partition retained query FASTA files by query text and other notebook intermediates by input-file content
• validate annotation-cache identifiers before reuse
• consolidate focused cache regressions in the normal pipeline test suite
• document the corrected behavior and issue scope in the existing OpenSpec change

Scope clarification

Issue #338 reports stale projections after changing a dimensionality-reduction slider. The existing projection cache key already includes all reducer parameters, so that exact symptom is not reproduced by the current code or by this PR regression.

The audit found a separate reproducible cache-identity problem in the same notebook flow: one shared cache directory allowed changed queries, disjoint FASTA inputs, same-ID changed sequences, and different H5 datasets to reuse incompatible FASTA, embedding, annotation, or projection intermediates. This PR addresses that verified problem and keeps explicit projection refresh as a notebook-level correctness guarantee.

Fix

• Query downloads use a query-derived retained FASTA path.
• Once an input file is available, its byte content selects the retained intermediate directory. Byte-identical inputs still reuse embeddings and annotations; changed content selects a separate cache.
• ReductionPipeline rebuilds an annotation cache when its identifier multiset differs from the current request.
• The notebook passes embedding sets into annotation fetching so sequence-dependent annotation sources receive the current FASTA sequences.
• Projection refresh remains configured through refetch_stages=frozenset({"projections"}).

Verification

• Focused cache identity regressions: 7 passed
• Python non-slow workspace suite: 794 passed, 6 deselected
• Ruff check and format check: passed (143 files)
• Notebook nbformat validation and code-cell compilation: passed (4 code cells)
• openspec validate fix-notebook-projection-cache --strict: passed
• pnpm precommit: passed before commit and in the commit hook

Related to #338; this PR no longer auto-closes it because the reported slider-only symptom remains unconfirmed.

[tsenoner]

Automated review

Does it solve #338? Partially — forcing refetch_stages={"projections"} in the notebook makes the "always recompute" guarantee unconditional and does fix a real adjacent bug (all five bundled example H5s load as name="prot_t5", so switching examples previously rebundled the previous dataset's coordinates). But the scenario #338 actually reports — moving a DR slider — already recomputed on main, since the projection cache key has hashed the full reducer-parameter dict since a030370 (2026-03-31). So the reported symptom is neither reproduced nor diagnosed here, and the dataset-switch case the PR does target is only half-fixed. Worth confirming the repro with the reporter rather than auto-closing #338 on merge.

Found 4 issues:

1. Scenario 1 of the new spec already held before this change — _projection_cache_path hashes {"embedding", "method", "dims", "params"} and all six notebook sliders are ReducerParams fields, so n_neighbors 25 vs 50 and min_dist 0.40 vs 0.41 each produce distinct proj_*.npz names on main (also documented at apps/protspace/docs/cli.md:356). The added test covers scenario 2 only, so Closes #338 will close the reporter's issue with behaviour unchanged for their steps.

protspace/openspec/changes/fix-notebook-projection-cache/specs/notebook-projection-cache-safety/spec.md

Lines 6 to 12 in 3d9d670

	#### Scenario: Reducer parameters change between Generate actions
	- **WHEN** a user changes a dimensionality-reduction parameter and activates Generate again
	- **THEN** the selected reducer runs with the current parameter value
	- **AND** the downloaded bundle contains coordinates produced by that run

2. The PR's own root-cause scenario still yields a broken bundle: the annotation cache is keyed on columns, not identifiers, so missing = required - cached_annotations is empty after a dataset switch and the previous dataset's rows are returned. Globin → Generate → Phosphatase → Generate now gives correct coordinates but every annotation blank (left-merge on identifier → NaN → fillna("")), with no warning, since the "all cached annotations are empty" check inspects the cached frame.

protspace/apps/protspace/src/protspace/data/processors/pipeline.py

Lines 396 to 413 in 3d9d670

	if cache_path.exists():
	cached_df = pd.read_parquet(cache_path)
	cached_annotations = set(cached_df.columns) - {"identifier"}
	if annotations_list is None:
	from protspace.data.annotations.configuration import (
	ANNOTATION_GROUPS,
	)
	required = set(ANNOTATION_GROUPS["default"])
	else:
	required = set(annotations_list)
	missing = required - cached_annotations
	if not missing and not refetching_annotations:
	logger.warning("Using cached annotations")

3. On the FASTA/query tabs the reducer now always re-runs, but over stale inputs: sequences.fasta is reused whenever non-empty with no comparison against inp["query"], and embedding_cache is keyed only on the embedder name, so uploading a second FASTA leaves output/tmp/prot_t5.h5 holding the union (149 + 50 = 199 proteins in the bundle) and never recomputes same-ID/changed-sequence entries. Neither cache consults refetch_stages. Deriving cache_dir (or a key prefix) from the selected input's identity would cover all three caches.

protspace/apps/protspace/notebooks/ProtSpace_Preparation.ipynb

Lines 600 to 609 in 3d9d670

	" step_html.value = \"<b>Step 1/6: Downloading FASTA...</b>\"\n",
	" fasta_cache = cache_dir / \"sequences.fasta\"\n",
	" if fasta_cache.exists() and fasta_cache.stat().st_size > 0:\n",
	" from protspace.data.loaders.query import (\n",
	" extract_identifiers_from_fasta,\n",
	" )\n",
	" headers = extract_identifiers_from_fasta(fasta_cache)\n",
	" fasta_path = fasta_cache\n",
	" print(f\"Using cached FASTA ({len(headers):,} sequences)\")\n",
	" else:\n",

4. The new test hand-rolls InputRecordingBase and bypasses the constructor via object.__new__(ReductionPipeline), so it keeps passing even if __init__ stops wiring self.base/get_reducers(). tests/test_pipeline_utils.py already covers this _run_reductions path (TestPrecomputedMDSConfigIsolation, L591-L627) by building the pipeline normally and patching pipeline.base.process_reduction, and has a _make_es factory at L409. Folding the case in there also resolves the issue-numbered filename (no other such file exists; bare #N is ambiguous post-rename) and the missing ### Test Files row in apps/protspace/CLAUDE.md.

protspace/apps/protspace/tests/test_issue_338_reproduction.py

Lines 15 to 30 in 3d9d670

	class InputRecordingBase:
	def __init__(self, config):
	self.config = config
	self.reducers = {"umap": object()}
	self.inputs = []
	def process_reduction(self, data, method, dims):
	self.inputs.append(data.copy())
	return {
	"name": f"{method}{dims}",
	"dimensions": dims,
	"info": {},
	"data": data[:, :dims].copy(),
	}

🤖 Generated with Claude Code

_{Reviewed at 3d9d670 against issue #338.}

[FlorinSenoner]

Independent triage against current head 3d9d670f6129a21df6dd27025d822af3e1ed6c7b:

1. Actionable - issue/reproduction mismatch. Confirmed that _projection_cache_path hashes the effective reducer params, and the notebook passes all six sliders into ReducerParams; issue [BUG] Invalidate cached projections #338 only describes the slider-change flow, while the new regression covers changed same-name embeddings. Direction: confirm/reproduce the reporter symptom before auto-closing [BUG] Invalidate cached projections #338, and distinguish that flow from the separate dataset-identity cache collision in the spec/tests and issue linkage.
2. Actionable - annotation cache identity. Confirmed the cache-hit decision compares requested annotation columns only, not current identifiers. The notebook then left-merges cached rows onto current headers, and bundle creation converts resulting nulls to empty strings. Direction: validate or partition annotation cache by current input/identifier identity and add a cross-dataset regression.
3. Actionable - query/FASTA and embedding cache identity. Confirmed sequences.fasta is reused solely on existence, while both embedding backends resume one {embedder}.h5 by skipping existing IDs and appending new ones; same-ID/changed-sequence entries are therefore not recomputed. Direction: derive/validate upstream caches from the selected input identity (or explicitly refresh them) and cover query changes, disjoint FASTA inputs, and same-ID sequence changes.
4. Actionable, but test-maintainability rather than proof the production change fails. Confirmed the regression bypasses ReductionPipeline.__init__; existing test_pipeline_utils.py cases construct the pipeline normally and patch pipeline.base.process_reduction, and the test-file inventory has no entry for the new standalone file. Direction: use the normal constructor in the existing pipeline suite, or otherwise document the standalone test and justify the narrower seam.

All current CI checks are green, but none of the added coverage exercises the upstream cache-identity cases in items 2-3. No thread is being resolved by this response.

[FlorinSenoner]

Implemented the verified review items in 3c4b9f4c:

• corrected the OpenSpec/PR scope: reducer sliders were already part of projection identity, so [BUG] Invalidate cached projections #338 is now related rather than auto-closed;
• partitioned query FASTA caches by query and all other notebook intermediates by input content, covering changed queries, disjoint FASTA inputs, and same-ID changed sequences;
• required annotation-cache identifiers to match the current request before reuse;
• moved the projection regression into test_pipeline_utils.py, constructed ReductionPipeline normally, and updated the test inventory.

Verification: focused regressions 7 passed; non-slow Python suite 794 passed, 6 deselected; Ruff clean/format clean across 143 files; notebook parse/compile passed for 4 code cells; strict OpenSpec validation passed; pnpm precommit passed before commit and in the commit hook. No review threads were resolved.

[FlorinSenoner]

[P1] Scope the embedding cache by backend

This content-owned directory is later paired with only {emb_name}.h5, although the notebook lets the user switch between Local and Biocentral. Both production backends resume by skipping IDs already present in that H5 (local.py:283-285, biocentral.py:150-167). On this head I seeded this exact cache layout with a Local-produced vector, then called embed_fasta(..., backend="biocentral"); it returned the unchanged Local vector without invoking Biocentral. A backend change can therefore silently generate projections and bundles from the previously selected backend. Please include the backend (and any embedding-affecting configuration) in cache ownership, or validate producer metadata before reuse, and add a backend-switch regression.

[FlorinSenoner]

Implemented in 82b6dbb2. The existing input-content directory now uses backend/model-owned H5 names ({backend}-{embedder}.h5), so Local and Biocentral cannot satisfy each other’s resume-by-ID cache while identical input/backend/model runs still select the same file. Regression coverage exercises both the Local→Biocentral switch (Biocentral is invoked and its vector returned) and same-backend reuse. Verification: focused cache/backend/query suite 21 passed; full non-slow Python suite 801 passed, 6 deselected; Ruff, notebook parse, strict OpenSpec, and pnpm precommit all passed.

[FlorinSenoner]

[P2] Publish query FASTA caches atomically

The new query-addressed path is still accepted solely when it exists and has nonzero size. query_uniprot writes save_to directly, so interruption during gzip extraction can leave a truncated-but-nonempty FASTA; the next Generate extracts whatever headers are present and permanently treats that subset as the full query result. I reproduced acceptance with a one-record partial file at this path. Please write to a temporary sibling and atomically rename only after extraction succeeds (or persist equivalent completion metadata), and cover partial-cache recovery.

[FlorinSenoner]

Implemented in 82b6dbb2. query_uniprot now extracts into a temporary sibling, verifies the complete write and ordered identifiers against the downloaded gzip, atomically replaces the query-addressed final path only after validation, and cleans compressed/staged artifacts in finally. The interruption regression writes partial bytes and proves neither the final cache nor a staged sibling survives; successful publication and same-query path reuse are covered. Verification: focused suite 21 passed; full non-slow Python suite 801 passed, 6 deselected; Ruff, notebook parse, strict OpenSpec, and pnpm precommit all passed.

[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. 13 raised, 8 survived refutation.

Applied and pushed (af4ffcaf)

Behavior-preserving cleanups, verified green before pushing:

• Fix 2 (query.py): removed the unreachable short-write guard in query_uniprot - written = out.write(content) + if written != len(content): raise OSError("Incomplete FASTA extraction") collapsed to out.write(content). TextIOWrapper.write always returns len(s), so the OSError could never fire; a real truncation still propagates from the close-time flush of the with open(...) block.
• Fix 1 (test_query.py): in test_query_uniprot_does_not_publish_partial_fasta, swapped the process-wide monkeypatch.setattr(builtins, "open", interrupt_cache_write) for monkeypatch.setattr(query_module, "open", interrupt_cache_write, raising=False). Kept import builtins and real_open = builtins.open (the fake still needs the real builtin) and left interrupt_cache_write and its filter untouched.

Issue resolution — partially resolves the issue

The triage is half right, and the half it gets wrong is the half the reporter cares about.
Correct: the claim that the slider symptom is not reproducible holds up under independent
verification. _projection_cache_path (pipeline.py:593-601) has included the full
asdict(ReducerParams) since before the issue was filed (confirmed at 77e696d), and the notebook
already wired all six sliders into ReducerParams at 817a704 (2026-07-03, ~2 weeks before the
2026-07-16 report). I reproduced both directions in a live pipeline: slider change → reducer runs;
identical params → "Using 2 cached projections". So no version-consistent notebook+package pairing
can serve a stale projection after a parameter change. Declining to claim a fix for a bug that does
not exist is the right instinct.
Also correct: the redirect found genuine bugs, not busywork. Pre-PR fasta_cache = cache_dir / "sequences.fasta" meant every subsequent UniProt query in a session silently reused the first
query's FASTA — an entire wrong dataset in the bundle. Shared {emb}.h5 across inputs plus
identifier-only resume meant a changed sequence under the same ID kept its stale vector. Those are
worse than what #338 describes, and the fixes are sound.
Where the triage falls short: it audited the Python cache layer and stopped. The question "what
would have to be true for the user to have seen this?" was never answered, and it has two concrete
answers sitting in the repo. The notebook's default method set puts PCA first, PCA ignores every
slider, and the viewer opens projections[0] — so the default post-change view is bit-identical and
its name and metadata are identical too. And the bundle is written to one fixed filename and pushed
through files.download, so a browser leaves the user with two files and no way to tell them apart.
Either explanation reproduces the report verbatim on the fixed notebook. The design doc asserts "the
slider-only symptom is not reproduced by the current implementation," which is true of the cache and
false of the user experience.
Closing semantics: "Related to #338, does NOT auto-close" is the right call — closing on a fix for a
different bug would be wrong. But leaving it open without action is not the end state either. Ask
the reporter which projection they compared and whether they re-uploaded the freshly downloaded
file; that will confirm or eliminate (a) and (b) in one round trip. Land the PCA/param-echo and
filename changes before closing. As it stands, if the reporter retests the merged notebook by their
original steps, they will very likely re-report the same thing.

Gaps found by the issue audit (7)

• PCA (projection[0], the viewer's default) is unaffected by every slider and carries no distinguishing name or metadata, so the reported symptom survives this PR.
  • Why it matters: This is the most probable actual cause of [BUG] Invalidate cached projections #338. Notebook defaults are PCA+UMAP; PCA is first in the bundle and apps/web/src/explore/data-renderer.ts:36 opens projections[0]. Verified empirically: PCA coordinates are bit-identical across an n_neighbors change, the projection name is identical ('ProtT5 — PCA 2'), and PCAReducer.get_params() emits no parameter that differs. The reporter can repeat their exact steps on the fixed notebook and see the same thing.
  • Suggested follow-up: Either order UMAP first when both are selected, or make the parameter set visible on the projection: pass the notebook's reducer params through format_param_suffix / into the projection name, and/or add the effective ReducerParams to every reduction's info dict so protspace-projection-metadata (packages/core/src/components/scatter-plot/projection-metadata/projection-metadata.ts) shows them for PCA too. Also print in the notebook, e.g. 'Recomputed 2 projections with n_neighbors=50, min_dist=0.4'.
• The bundle filename is fixed (output/data.parquetbundle) and re-downloaded via files.download on every Generate.
  • Why it matters: The issue literally says the notebook 'redownloaded the parquetfile'. A browser saves the second download as 'data (1).parquetbundle'; re-uploading the first file to protspace.app shows old coordinates no matter how correct the Python side is. The PR does not touch naming, and nothing in the notebook output tells the user which file is which.
  • Suggested follow-up: Include a short run discriminator in the filename (timestamp or a hash of methods+reducer params), and print the resolved filename plus the parameter values used next to the 'Done!' status.
• The notebook now imports three private helpers (_query_fasta_cache_path, _input_cache_dir, _embedding_cache_path) from the installed protspace package, but the Colab badge loads the notebook from main while cell 1 runs pip install -qqq "protspace[local]" (latest PyPI).
  • Why it matters: README.md:29 and apps/web/src/components/Hero.tsx:75 point Colab at .../blob/main/... . The instant this merges, main's cell 1 raises ImportError for every user until protspace-release.yml → protspace-publish.yml lands the new wheel on PyPI, and permanently for anyone on a pinned/cached older protspace. Cell 1 failing means the whole notebook fails, which is a worse user-visible outcome than the bug being fixed.
  • Suggested follow-up: Define the three helpers inline in the notebook (they are ~10 lines of pure stdlib) or import them inside a try/except with an inline fallback. Keeping notebook-only cache-path policy inside the library is what creates the coupling.
• Content-addressed input directories destroy incremental embedding resume: any FASTA edit re-embeds everything.
  • Why it matters: _input_cache_dir (pipeline.py:91-97) hashes the whole input file, so adding a single sequence to a 5,000-sequence FASTA yields a new digest and a brand-new empty H5. embed_sequences resumes purely by identifier (local.py:284-287), so all 5,001 sequences are re-embedded on a Colab GPU, and the previous multi-GB H5 is orphaned under output/tmp/inputs/. 'Iterate on my FASTA' is a far more common loop than 'silently change a sequence under the same ID'. design.md's Risks section lists the extra read cost but not this.
  • Suggested follow-up: Keep the H5 keyed by (backend, model) and store a per-identifier sequence hash as an H5 attribute, invalidating only the identifiers whose sequence changed — or at minimum document the full-re-embed consequence in the notebook and design.md risks.
• docs/cli.md caching table not updated for the annotation identifier check, which is a CLI-visible behavior change.
  • Why it matters: _fetch_annotations is shared with protspace prepare. apps/protspace/docs/cli.md:352 still says the annotation cache means 'Fetch only missing annotation sources'; with pipeline.py:417-437 a CLI user who reuses -o out with a different input now discards the whole cache and refetches every annotation for every identifier. Correct behavior, undocumented cost.
  • Suggested follow-up: Update the --keep-tmp table row and the bullet list in apps/protspace/docs/cli.md to state that the annotation cache is reused only when the identifier multiset matches exactly.
• The projection cache in the notebook is now write-only, and no test covers the issue's literal scenario.
  • Why it matters: With refetch always on, _save_projection_cache (pipeline.py:731) still writes proj_*.npz files that _load_cached_projection can never read — pure disk churn per Generate. Separately, the AST test only asserts refetch_stages is present in the notebook's PipelineConfig; nothing asserts the notebook passes an input-derived intermediate_dir, and no test runs two Generates with different reducer params (the exact [BUG] Invalidate cached projections #338 scenario).
  • Suggested follow-up: Either skip the projection cache write when the stage is being refetched, or drop keep_tmp projection caching for the notebook path. Add a two-run test that varies a ReducerParams field and asserts the reducer saw both values, and extend the AST assertion to cover intermediate_dir=cache_dir.
• A 'Using cached annotations' warning still prints on every re-Generate, and nothing prints what actually got recomputed.
  • Why it matters: pipeline.py:453 logs at WARNING; with no logging configured in Colab, Python's lastResort handler writes it to the cell output. A user who re-runs after a slider change still sees a 'Using cached …' message and a fast run — plausibly the very signal behind the report — while nothing confirms the projections were recomputed with the new values.
  • Suggested follow-up: Print an explicit line in the notebook after _run_reductions, e.g. 'Recomputed N projections (n_neighbors=50, min_dist=0.40)', and consider demoting the cached-annotations message to info in the notebook path.

Findings needing a decision (6)

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

1. The identifier-mismatch branch copy-pastes the existing "no cache" fetch branch instead of falling through to it.

apps/protspace/src/protspace/data/processors/pipeline.py:426 · medium · simplification

Lines 426-436 build ProteinAnnotationManager(headers=..., annotations=annotations_list, output_path=cache_path, sequences=sequences).to_pd() and return self._merge_csv(api_df, csv_df) —
byte-for-byte the else branch at lines 518-525 that already handles "no usable cache".
_fetch_annotations now has three copies of the same fetch-and-merge tail inside a five-level-deep
if/else, so any future change to the fresh-fetch call (a new manager kwarg, a retry, a log line) has
to be made in three places or the cache-miss paths drift apart.

Suggested fix

In apps/protspace/src/protspace/data/processors/pipeline.py, replace lines 417-437 with:
cached_df = pd.read_parquet(cache_path) if cache_path.exists() else None
if cached_df is not None:
cached_identifiers = (
Counter(cached_df["identifier"].astype(str))
if "identifier" in cached_df.columns
else Counter()
)
if cached_identifiers != Counter(map(str, headers)):
logger.info(
"Annotation cache input changed; fetching annotations "
"for the current identifiers"
)
cached_df = None
if cached_df is not None:
...then leave existing lines 439-517 as the body of that block (unchanged, same indentation) and
existing lines 518-525 as its else:. This removes the duplicated fetch-and-merge tail at 431-437.
If finding [8] is also adopted, change the predicate in the same edit to if Counter(map(str, headers)) - cached_identifiers: rather than making two passes over this block.

2. Requiring exact identifier-multiset equality (rather than "every requested id is cached") turns a previously-free subset run into a full API refetch that also destroys the larger cache.

apps/protspace/src/protspace/data/processors/pipeline.py:426 · medium · correctness

Reproduced empirically. Cache out/tmp/all_annotations.parquet holds P0..P4; a run requesting only
["P0","P1"] now takes the new branch: it calls ProteinAnnotationManager(..., output_path=cache_path) (API calls: [['P0','P1']]) and _save_and_load rewrites the parquet,
leaving only ['P0','P1']. On origin/main this request was served entirely from cache with zero API
calls and the 5-row cache survived. Real CLI sequence: protspace prepare -i prot_t5.h5 -o out
(573K Swiss-Prot ids, hours of UniProt/InterPro fetching) then protspace prepare -i prot_t5.h5 -i esm2_650m.h5 -o out — _validate_headers returns the intersection (say 500K), so
cached_identifiers != requested_identifiers fires, all 500K annotations are refetched from the
API, and the 573K cache is overwritten with 500K rows; the next single-input run then refetches all
573K again. The old subset behavior was not a bug: run() builds full_metadata = pd.DataFrame({'identifier': all_headers}).merge(metadata, how='left') (pipeline.py:264-273), so
extra cached rows were always dropped. Only missing identifiers were ever a correctness problem.
This also contradicts docs/cli.md:351 ("Fetch only missing annotation sources").

Suggested fix

In apps/protspace/src/protspace/data/processors/pipeline.py, replace lines 426-437 with:
missing_identifiers = requested_identifiers - cached_identifiers
if missing_identifiers:
logger.warning(
"Annotation cache is missing %d requested identifier(s); "
"fetching annotations for the current identifiers",
sum(missing_identifiers.values()),
)
api_df = ProteinAnnotationManager(
headers=headers,
annotations=annotations_list,
output_path=cache_path,
sequences=sequences,
).to_pd()
return self._merge_csv(api_df, csv_df)
Also update openspec/changes/fix-notebook-projection-cache/specs/notebook-projection-cache-
safety/spec.md: change 'only when its identifier multiset matches the identifiers requested' to
'only when it contains every identifier requested by the current run', and adjust the 'Input
identifiers change between runs' scenario to say the cache is rebuilt when requested identifiers are
absent from it. Add a test asserting a superset cache still serves a subset request with zero API
calls and without truncating the parquet. If [2] is applied too, fold this predicate into the single
cached_df = None cache-miss path rather than editing twice.

3. After this diff the "query" and "fasta" branches of _on_gen are line-for-line identical from the cache setup onward.

apps/protspace/notebooks/ProtSpace_Preparation.ipynb:640 · low · simplification

Once the fasta branch was changed to hoist fasta_path = Path(inp["path"]) (line 638), lines
640-657 are identical to lines 617-633 except for the step-label string (Step 1/5 vs Step 2/6):
same _input_cache_dir + mkdir, same _resolve_backend_and_config, same _drop_incompatible +
empty guard, same embed_fasta loop, same emb_set.fasta_path assignment. This PR had to apply the
backend-qualified cache path twice for that reason, and the next cache-ownership change will have to
be applied twice again — which is precisely how the two paths drift. Separately,
cache_dir.mkdir(parents=True, exist_ok=True) is repeated at lines 618, 641 and 660 although every
branch either returns or assigns cache_dir.

Suggested fix

In apps/protspace/notebooks/ProtSpace_Preparation.ipynb, cell 5, add a local helper next to
_drop_incompatible and call it from both branches:
def _embed_input(fasta_path, embs, step_html, step_label):
cache_dir = _input_cache_dir(cache_root, fasta_path)
cache_dir.mkdir(parents=True, exist_ok=True)
backend, _emb_cfg = _resolve_backend_and_config()
embs = _drop_incompatible(embs, backend)
if not embs:
print("No embedder compatible with the selected backend.")
return None, cache_dir
sets = []
for emb_name in embs:
step_html.value = f"{step_label}: Computing {emb_name} embeddings ({backend})..."
emb_set = embed_fasta(
fasta_path, emb_name,
backend=backend,
embed_config=_emb_cfg,
embedding_cache=_embedding_cache_path(cache_dir, emb_name, backend),
)
emb_set.fasta_path = fasta_path
sets.append(emb_set)
return sets, cache_dir
(cache_root is a closure variable of _on_gen, so define the helper inside _on_gen or pass
cache_root explicitly.) Query branch: sets, cache_dir = _embed_input(fasta_path, embs, step_html, "Step 2/6"); fasta branch: "Step 1/5"; each then if sets is None: return /
embedding_sets.extend(sets). Keep the mkdir INSIDE the helper (before any embed_fasta call) and
keep the H5 branch's own cache_dir.mkdir(...); do NOT hoist a single mkdir to after the
if/elif/else chain.

4. 30 lines of notebook-JSON + AST walk + eval(compile(...)) recover a literal frozenset({"projections"}) that is never asserted.

apps/protspace/tests/test_pipeline_utils.py:30 · low · simplification

_preparation_notebook_projection_refetch_stages loads the .ipynb, joins the code cells, finds the
one containing def _on_gen, walks the AST for the first PipelineConfig(...) call, pulls the
refetch_stages keyword and evals it in a restricted namespace — then feeds the value into a
pipeline behavior test without ever asserting what it is. Two costs: a notebook regression (someone
drops the kwarg) surfaces as assert 1 == 2 inside
test_notebook_refreshes_same_name_changed_input_through_pipeline, which names neither the notebook
nor refetch_stages; and any notebook restructuring (renaming _on_gen, wrapping the config build
in a helper) makes the three chained next(...) calls raise a bare StopIteration during
collection of an apparently unrelated pipeline test.

Suggested fix

In apps/protspace/tests/test_pipeline_utils.py: keep
_preparation_notebook_projection_refetch_stages() but replace each bare next(...) with a
next(..., None) + pytest.fail("PipelineConfig(...) with refetch_stages not found in the notebook Generate cell") guard. Add a dedicated test:
def test_notebook_requests_projection_refetch():
assert _preparation_notebook_projection_refetch_stages() == frozenset({"projections"})
and in test_notebook_refreshes_same_name_changed_input_through_pipeline pass the literal
refetch_stages=frozenset({"projections"}) instead of calling the extractor.

5. Staging through tempfile.NamedTemporaryFile changes the published FASTA's mode from 0644 to 0600, because os.replace preserves the source file's permissions.

apps/protspace/src/protspace/data/loaders/query.py:93 · low · edge-case

Reproduced: after query_uniprot("x", save_to=d/'sequences.fasta') the resulting file is mode
0o600; on origin/main it was created by a plain open(save_to, 'w') and got 0o666 & ~umask (0o644
with the default umask). Concrete failure: a user runs protspace prepare -q "family:globin" -o /shared/project --keep-tmp on a group-shared filesystem; a collaborator (or a downstream container
step running as a different uid) that previously read /shared/project/tmp/sequences.fasta now gets
PermissionError. The notebook's output/tmp/queries/<digest>.fasta is affected identically.

Suggested fix

In apps/protspace/src/protspace/data/loaders/query.py, add import os to the imports, and
immediately before staged_path.replace(save_to):
umask = os.umask(0)
os.umask(umask)
staged_path.chmod(0o666 & ~umask)
staged_path.replace(save_to)
Cover it with a test asserting the published FASTA's mode matches 0o666 & ~umask rather than
0o600.

6. The --keep-tmp reuse table still claims annotations only ever "fetch missing sources"; it now also silently discards the cache when the identifier set differs.

apps/protspace/docs/cli.md:351 · low · docs

A CLI user reading docs/cli.md:345-357 expects all_annotations.parquet reuse to be keyed on
annotation columns only, and is told --refetch is the way to bypass caches. After this change,
running protspace prepare -i subset.h5 -o out against an out/ populated from a larger input
silently re-hits the UniProt/InterPro/TED APIs for every protein and overwrites the parquet — with
no --refetch flag involved and no warning at WARNING level (the new message is logger.info). The
openspec spec documents the new rule but the user-facing CLI reference does not.

Suggested fix

In apps/protspace/docs/cli.md, update the table row at line 351 and add a bullet under the table. If
the exact-multiset rule is kept:
| Annotations | all_annotations.parquet | Fetch only missing annotation sources; the cache is
discarded and rebuilt when the run's protein identifiers differ from the cached ones |

• Reusing one -o directory across runs with different protein sets (e.g. adding a second -i
  whose identifiers only partially overlap) discards the annotation cache and re-hits the
  UniProt/InterPro/TED APIs.
  If finding [8] is adopted instead, word it as '...the cache is rebuilt only when it is missing
  identifiers the current run requests; a cache covering more proteins is still reused.' Either way,
  raise the logger.info at pipeline.py:427 to logger.warning so the refetch is visible at default
  verbosity, matching the existing 'Using cached annotations' warning.

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

[FlorinSenoner]

Follow-up to the 2026-08-06 adversarial review, addressed in 55837338.

Dispositions

Cleanups already applied in af4ffcaf

• The unreachable text-write length guard remains removed.
• The interrupted-write test continues to patch query_module.open rather than process-wide builtins.open.

Issue-audit gaps

1. PCA/default projection: no code change. PCA ignoring the notebook's nonlinear-reducer sliders is a plausible explanation for [BUG] Invalidate cached projections #338, but it is not evidence of stale projection-cache reuse. Reordering projections or attaching unrelated slider values to PCA conflicts with this change's explicit non-goals around reducer parameters/projection naming. The PR remains related to [BUG] Invalidate cached projections #338 and does not close it; reporter evidence about the compared projection is still needed.
2. Fixed bundle filename: no code change. Re-uploading the first of two browser downloads is also plausible, but changing output paths is an explicit non-goal and the reporter's upload steps are unknown. This remains an issue-level UX question rather than a verified cache defect.
3. Notebook imports versus PyPI publication: no fallback copy added. protspace-release.yml runs on every apps/protspace/** push to main and dispatches the package publish; duplicating cache-identity policy in the notebook would create a permanent drift path to cover that repository-wide release window. The notebook does not pin an older package version.
4. Full re-embed after any FASTA edit: accepted tradeoff, now documented in the OpenSpec design. Per-identifier sequence hashes would redesign the shared H5 resume format and remain outside this notebook-scoped fix.
5. CLI annotation-cache documentation: implemented. The table and accompanying note now describe requested-ID coverage, safe cached supersets, and rebuilds when requested IDs are absent.
6. Projection cache writes and literal slider test: no cache-write change. --refetch recomputes and refreshes the stored entry; suppressing the save would leave an older entry for a later non-refetching caller. A different-parameter two-run test passes before this PR because reducer params already key the cache, so it would not regress this change. The existing notebook-wiring behavioral regression now reports missing callback/config/keyword anchors explicitly and asserts the expected projection-only refetch contract before exercising two real reductions.
7. Cached-annotation warning / recompute message: no code change. The warning accurately refers to annotations while the notebook status separately enters “Reducing dimensions.” Additional UX output is reasonable follow-up work, but it does not establish a cache correctness failure without reporter evidence.

Findings needing a decision

1. Duplicated fresh annotation fetch: implemented. Both cache-missing paths now reuse one local fetch_current_annotations implementation.
2. Exact identifier equality destroys safe supersets: implemented. Cache eligibility now uses multiset subtraction (requested - cached); cached supersets remain reusable, while any missing requested occurrence triggers a visible rebuild. A RED regression proved the previous branch invoked the annotation manager and could replace the larger cache.
3. Duplicated query/FASTA notebook branches: not changed. This is optional refactoring without a demonstrated behavioral defect, and extracting it would enlarge a correctness-focused review patch.
4. Notebook AST helper diagnostics: implemented without splitting the test into a source-only change detector. Missing notebook anchors now fail with precise messages, the literal refetch value is asserted, and the same test still proves reducer behavior.
5. Atomic publication changes FASTA mode to 0600: implemented. Before replacement, the staged file receives 0666 & ~umask; a RED regression observed 0600 under umask 0027, then GREEN observed 0640.
6. CLI cache docs/logger visibility: implemented with item 2. Missing requested identifiers now log at warning level, and the CLI docs describe the corrected superset/missing-ID behavior.

Verification

• RED: 2 expected failures (published mode 0600 != 0640; cached superset invoked the annotation manager).
• GREEN/focused: 7 passed, then affected files 92 passed.
• Full Python non-slow suite: 803 passed, 6 deselected.
• Ruff check/format, notebook validation (4 code cells), and strict OpenSpec validation: passed.
• Fresh pnpm precommit passed before commit and again in the commit hook.
• Exact-head CI for 55837338ca191db09bd497c3c69f8eff193503dd: Python 3.12/3.13/3.14, lint, code quality, docs, bundle contract, and container build passed; E2E and deploy pinning were legitimate skips.

No review threads were resolved.