Skip to content

feat(statistics): consume projection statistics in the bundle pipeline - #295

Closed
jcoludar wants to merge 9 commits into
mainfrom
feat/projection-statistics
Closed

feat(statistics): consume projection statistics in the bundle pipeline#295
jcoludar wants to merge 9 commits into
mainfrom
feat/projection-statistics

Conversation

@jcoludar

Copy link
Copy Markdown
Collaborator

Summary

The protspace_web half of the projection-statistics MVP (issue #219). The engine
(protspace) computes per-projection cluster-validity + faithfulness statistics and bakes them into
the .parquetbundle as an optional fifth part; this PR makes the web side produce and tolerate
that part. Rendering of the statistics is a deliberate follow-up — out of scope here.

Engine PR (lands first): #61. This PR depends on it and on a
stats-bearing protspace release. It is safe to merge ahead of that release: the prep step
feature-probes the protspace stats subcommand and no-ops if absent — statistics simply don't
appear yet, and nothing else changes.

What's in this PR

  • Bundle reader (@protspace/core data-loader + @protspace/utils): accept 3–5 parts.
    The reader now branches on an empty settings slot (statistics-without-settings ⇒ a zero-byte
    4th part) rather than the raw delimiter count, so the optional fifth statistics.parquet is read
    without error. The statistics part is parsed-but-ignored for now; createParquetBundle
    re-export still drops it (documented).
  • Prep service (services/protspace-prep): a best-effort stats step that runs after the
    core bundle is produced and outside the pipeline timeout budget, so it can never cost the job
    or lose the bundle. It has:
    • its own nested timeout (stats_timeout_seconds), caught locally so it never reaches the
      parent handler;
    • a bounded, non-latching version probe for protspace stats (single-flight lock + hard
      timeout + kill of a hung subprocess; transient spawn errors are not latched, so a later job
      retries);
    • an atomic re-bundle — writes a sibling temp .parquetbundle and os.replaces it, so a
      stats timeout/kill mid-write can't corrupt the already-shipped bundle.
  • Frontend: a new computing_statistics SSE stage wired through FastaPrepStage and the explore
    runtime (progress creep stopped, 95%, "Computing statistics…").
  • Tests: the stale "five-part bundles are rejected" assertion is inverted to accept them;
    added 5-part round-trip + zero-byte settings-slot coverage; services/protspace-prep/tests
    exercise the success / timeout / probe-absent paths.
  • OpenSpec: openspec/changes/add-projection-statistics/ (proposal, design, spec, tasks).

Verification

  • @protspace/core bundle suite: 24 passed (incl. the inverted 5-part + zero-byte-slot tests).
  • protspace-prep pipeline suite: 19 passed.
  • Pre-commit (lint-staged && quality && docs:annotations:check && docs:build): green.

Data-format change

Additive, backward compatible — existing 3- and 4-part bundles read and write unchanged.

Refs #219

The protspace_web half of the projection-statistics MVP (engine PR:
#61; tracking issue #219). The prep service folds
engine-computed stats into the bundle, the reader accepts the optional fifth
part, and the UI surfaces the new stage.

- data-loader/bundle.ts (+ @protspace/utils bundle-writer): accept 3-5 parts;
  branch on an empty settings slot (statistics-without-settings) rather than the
  raw delimiter count. The statistics part is parsed-but-ignored for now —
  rendering is a committed follow-up, out of scope here.
- services/protspace-prep: a best-effort `stats` step AFTER the core bundle and
  OUTSIDE the pipeline timeout, with its own nested timeout, a bounded
  non-latching `protspace stats` version probe (lock + timeout + kill, no
  false-latch on transient errors), and an atomic temp-bundle + os.replace
  re-bundle so a stats timeout/kill can never corrupt or lose the shipped bundle.
- app: a `computing_statistics` SSE stage (progress 95%, "Computing statistics…").
- openspec/changes/add-projection-statistics: proposal, design, spec, tasks.
- Invert the stale "five-part bundles are rejected" test; add round-trip and
  zero-byte settings-slot coverage.

Depends on the engine PR (#61) and a stats-bearing protspace
release; the prep step feature-probes the subcommand and no-ops if absent, so
this is safe to merge ahead of the release (stats simply don't appear yet).

Refs #219

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
CI's `quality:ci` runs `format:check` before lint/quality and prettier
flagged this openspec doc (markdown list-continuation indentation).
Pure formatting — no content change.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@tsenoner
tsenoner marked this pull request as draft June 24, 2026 13:51
jcoludar and others added 7 commits June 24, 2026 18:25
…ction metadata

Phase 1B of route-projection-statistics. The engine now folds per-projection
faithfulness (kNN-overlap / trustworthiness / continuity) into each projection's
info_json under a `quality` object. The projection-metadata panel flattened
info_json only one level, so a nested `quality` rendered as a raw JSON.stringify
blob.

- Extract the metadata-row building into a pure, tested helper
  (projection-metadata-helpers.ts), matching the package's *-helpers pattern.
- Expand info_json.quality into discrete per-metric rows: each shows its value
  plus compact provenance (distance metric, k); a skipped metric (value null)
  renders as N/A with its marker; a flat scalar shape is tolerated.
- projection-metadata.ts now delegates to the helper (no behavior change beyond
  the quality expansion).

Tests: projection-metadata-helpers.test.ts (flatten, quality expansion, skip,
flat-scalar). Full core suite green (1073).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…gn/spec/tasks)

The design for routing each statistic to the bundle part whose existing frontend
consumer matches its granularity (tsenoner's PR #61 review): faithfulness →
projections_metadata.info_json.quality, per-protein cluster/silhouette →
protein_annotations, aggregate validity → statistics.parquet. Includes the
4-lens fan-out review outcomes and the phased plan (Phase 1 low-risk routing;
Phase 2 per-protein annotations behind a flag).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…bundle

Phase 2A of route-projection-statistics. `protspace stats` now enriches the
annotations parquet in place with per-protein cluster-membership + silhouette
columns when given -a; the prep re-bundle step passes the annotations path so the
following `bundle -a` carries those columns (and faithfulness rides in
projections_metadata, both from the same stats call). Still best-effort: any stats
failure leaves the core bundle untouched.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…gs part

Phase 2A.4 of route-projection-statistics. The stats step now writes the
auto-generated cluster-membership legend styles (`stats --settings-out
cluster_styles.json`) and the re-bundle folds them in (`bundle --settings ...`)
so clusters are colored when selected. The --settings flag is only added when the
styles file exists, so an older engine without --settings-out degrades gracefully
(columns still ship, just without pre-baked colors). Still best-effort.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… section

Phase 2B of route-projection-statistics. The color-by dropdown already
auto-discovers the computed `cluster_<proj>` / `silhouette_<proj>` annotation
columns (no allowlist hides them, and content-based inference types membership as
categorical and silhouette as continuous). This adds a dedicated "Statistics"
section to `groupAnnotations` so the ~12 computed columns (one pair per projection)
don't flood the catch-all "Other" group.

Test: cluster_/silhouette_ columns land in Statistics (sorted), non-computed labels
stay in Other.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…h review

- prep: don't latch a signal-killed `stats` probe as permanently absent.
  A probe killed by a signal (rc<0, e.g. OOM -9) is transient, not
  "subcommand missing"; only rc>=0 is a definitive answer. Prevents a
  single transient kill from disabling statistics process-wide until
  restart. (pipeline.py `_stats_cli_available`)
- prep: re-bundle on `stats` success instead of gating the whole re-bundle
  on the aggregate statistics.parquet existing, so the enriched per-protein
  cluster_/silhouette_ columns and cluster styles aren't silently discarded
  when only partial stats output is produced. Command is built incrementally
  (-s / --settings appended only when present, -o last). (pipeline.py
  `_maybe_add_statistics`)
- core: document the settings/statistics part-order assumption in the 5-part
  bundle reader and warn (observability only, no parse-behavior change) when
  a 5-part bundle's settings slot fails to parse as settings, surfacing a
  possible silent cluster-styles drop. (bundle.ts)
- core: group only exact `cluster_<projection>` / `silhouette_<projection>`
  columns under the Statistics section by threading projection names through
  groupAnnotations, so user-uploaded columns like cluster_id / cluster_size /
  silhouette_score are no longer misgrouped. (annotation-categories.ts +
  annotation-select / control-bar / query-condition-row wiring)
- core: render skipped or null faithfulness metrics as N/A (with the skip
  marker when present) instead of a raw JSON blob or the string "null".
  (projection-metadata-helpers.ts)
…from branch review

- core/utils: extract a shared `splitBundleParts()` into parquet/delimiter-utils.ts
  and use it in the bundle reader and its round-trip test, replacing the hand-rolled
  (and test-duplicated) delimiter-slicing math. Zero-copy subarray views; the
  statistics part is still never copied; the null-after-decode memory release and
  the delimiter-count guard are preserved.
- prep: extract `_kill_and_wait()` and use it in both `_run_step` and the stats
  probe, removing the duplicated "kill + bounded grace-wait" block. Control flow
  (re-raise in _run_step; log + return False without latching in the probe) is
  unchanged.
- prep: extract `_parse_bool()` and use it for the PREP_STATS / PREP_LOG_JSON_FORMAT
  boolean env vars, removing the duplicated truthy-set idiom.
- core: remove the unreachable `byteLength === 0` guard in extractSettings (its only
  caller passes a non-empty buffer, since part4 is non-null only when the settings
  slot has bytes).

Behavior-preserving; reviewed and verified (packages/utils + packages/core vitest,
services/protspace-prep pytest, and tsc --noEmit all clean).
@peymanvahidi

Copy link
Copy Markdown
Collaborator

Drive-by from #329 (docs consolidation) — your part-ordering assumption is now verifiable in-repo, and it holds.

bundle.ts in this PR carries:

ASSUMPTION (unverified in this repo): part-index 3 = settings and part-index 4 = statistics […] That CLI lives outside this repo (services/protspace-prep shells out to protspace bundle / protspace stats), so this ordering has not been verified against the pinned protspace version.

Since the monorepo merge the writer is in-tree at apps/protspace/src/protspace/data/io/bundle.py, and it confirms all three parts of the assumption:

# module docstring, L8-11
# Positional layout: ``core(3) + settings? + statistics?``.  When statistics are
# present but settings are absent, the fourth part is written as **zero bytes** so
# the statistics part is unambiguously the fifth — readers and writers branch on
# the fourth part's emptiness, not on the raw part count.

# _parse_bundle, L50-54
if len(parts) < 3 or len(parts) > 5:
    raise ValueError(f"Expected 3 to 5 parts in parquetbundle, found {len(parts)}")

settings   = parts[3] if len(parts) >= 4 and parts[3] else None
statistics = parts[4] if len(parts) == 5 and parts[4] else None

So: ordering is [core, core, core, settings, statistics]; the engine emits a zero-byte placeholder rather than omitting the delimiter; and "branch on emptiness, not the raw count" is the writer's own stated contract — which is exactly what this PR does. The failure mode the comment worries about (silently dropping auto-generated cluster styles) can't occur via that path.

Worth noting the two sides now express the bound in different units — Python validates 3-to-5 parts, this PR validates 2-to-4 delimiters. Same constraint, and this PR has it right, but it's an easy off-by-one for the next reader; might be worth a word in the comment.

If you'd like, the ASSUMPTION block and the console.warn observability guard could both be reduced now that the invariant is enforced upstream — though keeping the warn is defensible as cheap insurance against writer drift. Your call; I've left the loader alone.

Context: #329 covers the docs half of #310 (documenting the optional statistics table in data-format.md). I've deliberately not touched bundle.ts, delimiter-utils.ts, bundle-writer.ts or their tests so this PR owns the loader fix outright — no conflict from my branch.

One heads-up: this branch still targets the pre-monorepo layout (app/src/…, services/protspace-prep/…), which has since moved to apps/web/src/… and apps/prep/… on main. A rebase will likely need path fixes.

@peymanvahidi

Copy link
Copy Markdown
Collaborator

Coordination note from #329 (docs consolidation), so this PR does not hit a surprise conflict on its next rebase.

#310 had two halves: documenting the statistics table, and fixing the web loader that rejected 5-part --stats bundles. Since this PR is still a draft, the minimal loader tolerance landed in #390 so #310 could close with the epic:

  • packages/core/src/components/data-loader/utils/bundle.ts: now accepts 2 to 4 delimiters and branches on the zero-byte settings slot, so statistics stay at part 5. That is the same mechanism this PR uses, and it mirrors _parse_bundle on the Python side. The statistics part is deliberately not sliced or read.
  • packages/core/src/components/data-loader/utils/bundle.test.ts: the "reject 5 parts" assertion is inverted, plus a new 6-part rejection.

Rendering the statistics table is untouched and still belongs to this PR.

Suggested rebase resolution: the overlap is only those two files. Take the incoming version of bundle.ts and bundle.test.ts and drop this PR's tolerance hunk, keeping everything additive: projection-metadata*, bundle-writer.ts, delimiter-utils.ts, the control-bar changes, and the prep-service stats step. That leaves this PR as purely "render the statistics", with no functional overlap.

Worth folding into the same rebase: 6 files here are still on pre-monorepo paths.

  • app/src/explore/* is now apps/web/src/explore/*
  • services/protspace-prep/* is now apps/prep/*

For accuracy: #390 is still open, so this tolerance is not on main at the time of writing.

tsenoner added a commit that referenced this pull request Jul 28, 2026
The sweep that qualified refs in the source and tests stopped there, leaving
roughly 45 siblings across eight design docs -- including four hyperlinks
pointing at `tsenoner/protspace/issues/<N>`, which is worse than a bare ref
because it renders as an authoritative link to an unrelated frontend item.

Checking each number against the live API turned up something the first pass
had wrong: three issues were **transferred** into the monorepo rather than
merely stranded, so they have new numbers here. Only #59 was known.

    legacy#31 -> #324    legacy#59 -> #320    legacy#64 -> #318

Those are renumbered everywhere, since the old numbers are now simply wrong.
Everything else is qualified at each document's definitional line ("**Issues:**",
"**Refs:**", "**Trigger:**") rather than in every sentence -- fully qualifying
prose turns headings into "### 2.3 tsenoner/protspace-legacy#57: ...", which
trades one readability problem for another. Two docs that carried bare refs
with no definitional line at all get one.

Deliberately untouched: `#1`-`#6` in the EAT-UX docs are list markers, not
issue refs; `#296`/`#306`/`#295`/`#233` are frontend numbers this repo
inherited, so bare is already correct; and `CHANGELOG.md` is regenerated
wholesale by semantic-release, so editing it would not survive.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019TxEAVCkBQmh3Yy6bPXPSE
@tsenoner tsenoner closed this Aug 10, 2026
@tsenoner
tsenoner deleted the feat/projection-statistics branch August 12, 2026 16:37
tsenoner added a commit that referenced this pull request Aug 13, 2026
Three bookkeeping fixes found reviewing the archive:

- `#66`/`#55`/`#60` in the monorepo change's reconciled tasks are protspace-legacy
  PRs, but bare `#N` autolinks against this repo. `#66` is the silent case: it
  resolves to a merged, unrelated frontend PR ("enhance StructureService with 3D
  Beacons API integration") rather than 404ing, so the link looks fine and points
  somewhere wrong. The intended one is "bundle format v2 — lossless annotation name
  encoding", which is what the surrounding text describes. `#306`/`#295`/`#233`/`#445`
  are genuine refs here and stay bare.
- design.md still described `_INCOMPATIBLE` as (blocked set, why, remedy); 3cdacf9
  collapsed it to (blocked set, note). The archive commit landed before that refactor
  and nothing re-reconciled after.
- A 5.3 checkbox was ticked while its own text read "carried forward to #447 rather
  than ticked". Reworded so the box and the sentence agree.

openspec validate --all --strict: 20 passed, 0 failed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Ar6ME3o6VYLYsie3J4kxE5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants