Skip to content

Complete the presentation layer, then fix six engine fragilities at the root - #28

Merged
mikhaeelatefrizk merged 19 commits into
mainfrom
claude/bindsight-repo-structure-t8ey1h
Aug 1, 2026
Merged

Complete the presentation layer, then fix six engine fragilities at the root#28
mikhaeelatefrizk merged 19 commits into
mainfrom
claude/bindsight-repo-structure-t8ey1h

Conversation

@mikhaeelatefrizk

@mikhaeelatefrizk mikhaeelatefrizk commented Jul 28, 2026

Copy link
Copy Markdown
Owner

Summary

Two phases, in order. Phase 1 completes the presentation layer over the results already
committed to this repository. Phase 2 fixes six fragilities in the engine — two of which
turned out to be live, user-visible breakage rather than latent risk.

main is untouched. v0.2.0 and the Zenodo DOI remain authoritative. Every item is its
own commit, so any single one can be reverted independently.


Phase 1 — the presentation layer

The engine already produced validated results; nothing surfaced them. This wires the
committed contents of benchmarks/ into the web app and the docs site, and gives the
project a landing surface.

  • bindsight/report/theme.py (new) — one source of truth for the brand palette,
    identity strings, canonical URLs, and the app stylesheet, shared by the Streamlit app and
    the docs site so they cannot drift apart.
  • bindsight/report/showcase.py (new) — read-only loaders over benchmarks/
    (load_validation, load_designer_benchmark, headline_stats). Stdlib-only at module
    scope, so importing it costs nothing.
  • bindsight/report/webapp.py — a Results page backed by the real committed
    validation and designer benchmarks, a 3-D complex viewer (py3Dmol, binder in teal /
    target in navy), and a landing page. Navigation is routed through a pending-key so
    programmatic page changes do not collide with the sidebar widget.
  • Docsscripts/build_docs_results.py regenerates docs/results.md and its figures
    from the benchmark files, so the published numbers cannot drift from the data. Logo,
    favicon and Open Graph card added; mkdocs build --strict passes (it previously did not:
    nine links in the existing docs were 404ing live and are now fixed).
  • .streamlit/config.toml (new) — theme, upload limit, telemetry off.

Both hosted surfaces are free-tier permanently: GitHub Pages for the docs, a Hugging Face
Space for the live app.


Phase 2 — six engine fixes

1. The surfaceome could not be populated by anyone

wlab.ethz.ch now serves an HTML error page, so pd.read_excel died with
BadZipFile: File is not a zip file; the relocated wollscheidlab.org serves a 132-byte
Git-LFS pointer rather than the spreadsheet. With the default
surfy_allow_offline_fallback: false a real run hard-failed. With the demo's true, it
silently degraded to the bundled 10-protein list and produced ~0 candidates.

This is live breakage affecting every user, not a local or environment artifact — I
initially mis-diagnosed it as one, and this description previously said so.

Fixed by vendoring the full list. scripts/build_surfy_list.py (new, reproducible) fetches
surfaceome_ids.txt and resolves every entry name to a UniProt accession — 2,886 of
2,886, zero unresolved
— writing bindsight/surfaceome/data/surfy_v1.uniprot.txt with a
provenance header and CC BY 4.0 attribution. Resolution order is now cache → vendored →
tiny fallback, so discovery needs no network for the surfaceome at all. The remote refresh
path is repaired as an explicit opt-in, with a retry policy matching targets/gtex.py and a
content check that rejects HTML pages and LFS pointers before parsing, so the next
relocation raises a legible error instead of BadZipFile.

Verified: cold cache, no network access to the SURFY host, bindsight demo301 genes →
32 candidates
.

2. The Snakemake front-end had been dead for ~5 weeks

scripts/run_discover.py called _do_discover() with six of its eight required keyword-only
arguments — topology_client and gtex_client were added to the signature in June and the
wrapper was never updated. It raised TypeError before doing any work.
tests/test_snakemake_dag.py would have caught it but was slow-marked and therefore
skipped in CI. A second, independent breakage: scripts/assemble_manifest.py began with
from __future__ import annotations, which Snakemake's injected preamble pushes off line 1,
producing a SyntaxError.

Both fixed. Beyond the repair:

  • Each wrapper now emits a provenance fragment carrying real inputs, outputs and sha256
    digests (bindsight/provenance/fragments.py, new), and assemble_manifest.py builds full
    StageRecords from them. The Snakemake manifest was previously a completion log with no
    inputs, outputs, digests, params or ended_at — this is what makes the "two equivalent
    front-ends" claim in ARCHITECTURE.md actually true.
  • DAG ordering fixed so run_manifest.jsonld exists before report renders; the Snakemake
    report's provenance section was always empty.
  • scripts/run_rank.py now passes weights, which it silently dropped.
  • The DAG test runs in CI on Linux (~13 s), no longer slow-marked.

3. --cheap built as specified

ARCHITECTURE.md promised "RFdiff+MPNN on T4, 10 trajectories, ESM-2 pre-screen". All three
parts now exist. gpu_type is threaded into cost.estimate_full_run, which already accepted
it and had never been passed it, so --dry-run stops quoting A100 prices for a T4 profile:
$27.89 → $4.26.

The pre-screen (bindsight/design/prescreen.py, new) embeds designed sequences and keeps the
top-k most representative by distance from the embedding centroid. Because it changes what
gets validated, it is off by default (prescreen_top_k=None → bit-identical behaviour),
degrades with a warning rather than failing when the optional embed extra is absent, and
records a StageRecord naming what it discarded and why. Its 14 tests stub the embedder, so
the selection logic, both degradation paths and the executor seam are covered, but the real
ESM-2 weights are not downloaded in CI.

4. Run directories are now self-contained

The per-run structures/ directory was created and never written to, while candidate rows
stored an absolute user-cache path. Every downstream consumer and every exported RO-Crate
inherited a machine-local reference. Structures are now copied into the run directory and
stored run-relative; resolve_run_path() accepts both forms, so runs produced before this
change keep working. The run's config is persisted to config.yaml inside the run directory
and Manifest.config_path points at it rather than at the counts directory.

RO-Crate export extends to the artifacts actually produced — config, taxonomy,
design/metrics.jsonl, the structures/, design/_targets/ and validate/ trees — and
propagates the per-file sha256 the manifest already carried.

Verified: an exported crate unzipped on an unrelated path resolves 25 of 25 references
inside itself.

5. Importing bindsight.report no longer pulls in pandas

__init__.py eagerly imported render_run, which imports pandas and jinja2 at module scope,
so the deliberately stdlib-only theme.py and showcase.py dragged them in anyway. Replaced
with a PEP 562 module __getattr__ that raises AttributeError (never ImportError) for
unknown names — seven call sites import a submodule via from bindsight.report import ...
and depend on that fallback.

6. mypy and coverage actually gate CI

pyproject.toml sets strict = true; CI swallowed the result with || true, so 53 errors
had accumulated unnoticed — including the arity bug in item 2. --no-cov meant the configured
coverage reporting had never run. Both removed, and all 53 errors fixed (annotations and one
pandas override stanza; no logic changes).


Cleanliness

Tests were writing into the developer's real ~/.cache/bindsight on every run. An autouse
session fixture now redirects the cache via BINDSIGHT_CACHE_DIR, which also crosses
subprocess boundaries where monkeypatch cannot. Verified byte-identical cache contents
before and after a full test run.


Verification

  • mypy bindsight — 0 errors (was 53)
  • pytest -m "not gpu and not slow" — 393 passed, 77% coverage
  • bindsight demo on a cold cache with no SURFY host reachable — 301 genes → 32 candidates
  • bindsight run … --cheap --dry-run — $4.26 on T4 (was $27.89 on A100)
  • RO-Crate unzipped elsewhere — 25/25 references resolve
  • mkdocs build --strict, python -m build, twine check — all pass
  • Clean CI-equivalent venv runs the revived Snakemake DAG test in 14 s
  • CI on 92c1cca: all 9 checks green, including blocking mypy and the revived DAG test

Risk

Two changes alter real behaviour, called out plainly: structures are copied into the run
directory
(item 4), and the pre-screen can drop designs when explicitly enabled (item 3,
off by default). Everything else is corrective or additive.

Summary by CodeRabbit

  • New Features

    • Added a branded Home experience with workflow highlights, calls to action, benchmark statistics, and responsive layouts.
    • Added a Real Results page with validation findings, binder-design benchmarks, rankings, visualizations, structure viewing, and downloads.
    • Added cost-conscious design execution with optional representative-design screening.
    • Runs now support portable artifacts, offline surfaceome discovery, richer provenance, and reproducible exports.
  • Documentation

    • Refreshed hosted documentation with branding, benchmark results, improved navigation, citations, and corrected links.
  • Bug Fixes

    • Improved demo persistence, path handling, report portability, link validation, and offline behavior.

The brand navy, page title, and page icon were re-declared independently in
report/webapp.py, report/streamlit_app.py and report/templates/report.css,
while the docs site used an unrelated teal. Three surfaces of one product had
drifted apart, and there was no Streamlit theme at all -- so Streamlit's own
chrome (radio selection, spinners, focus rings) rendered in the default
red/pink against navy headings on every page.

- bindsight/report/theme.py: single source of truth for palette, identity,
  canonical URLs, and the app stylesheet. Stdlib-only so it stays importable
  without Streamlit. Same pattern as pipelines/caveats.py.
- .streamlit/config.toml: theme matching the brand, plus
  gatherUsageStats = false -- `bindsight ui` has always documented itself as
  "local-first; no telemetry" and nothing in the repo made that true.
- The stylesheet gains real media queries. The webapp docstring claimed the
  layout "works on phones"; there were no responsive rules whatsoever.
- docs/assets/: logo, favicon, and a social-preview card. Links to the project
  previously previewed with no image anywhere.
- scripts/make_og_image.py: reproducible source for the PNG, since Open Graph
  consumers do not render SVG.

No pipeline, CLI, config, or artifact behaviour is touched.
benchmarks/ holds the strongest evidence this project has -- the rediscovery
experiment over six real TCGA cohorts (ERBB2 at rank 4) and 20 real ERBB2
binders designed on a free Kaggle P100, each with its actual Boltz-2 predicted
complex .cif, per-design metrics, developability descriptors and ESM-2
embedding coordinates.

None of it was reachable from the web app. A grep for those artifact paths
outside benchmarks/ hits only markdown and the paper, so a visitor to the
hosted app saw a Demo button and had to take the science on faith.

bindsight/report/showcase.py loads that tree so the Streamlit app and the
documentation site read the same numbers and cannot drift from what is
actually committed. Landing-page headline figures are derived, never typed in,
so the marketing surface cannot overstate the benchmarks.

Read-only, network-free, stdlib-only at module scope. Every loader degrades to
None rather than raising, because benchmarks/ is deliberately not packaged
into the wheel -- the HF Space and the Streamlit Cloud mirror both deploy the
full repository and get the real data.

tests/test_showcase.py pins the published claims (ERBB2 rank 4, 20 designs,
best ipTM 0.84, 50% success@0.65) so a silent change to the committed results
becomes a loud test failure, and covers the missing/corrupt-tree paths.
The app now surfaces the evidence that was already in the repository but
invisible to anyone using it.

Rediscovery: ERBB2 at rank 4 in HER2-enriched TCGA-BRCA (log2fc 4.36),
recall@k, the six cohort volcanoes, and the specificity result -- with the
benchmark's own framing that antigens are grouped by measured over-expression
rather than clinical fame, and the data-limited cohorts listed openly.

Designer benchmark: 20 real ERBB2 binders from a free Kaggle P100, every
design scored, plus a 3D viewer over the actual Boltz-2 predicted complexes.
Chain B (the designed binder) is coloured against chain T (the target), so
what you see is the designed interface, not a decorative ribbon.

This also makes a long-standing docstring true. py3Dmol has been a declared
dependency of the `report` extra since the beginning and was never imported
anywhere, while report/html.py documented a structure viewer that did not
exist. No new dependency was needed -- it was already installed on both
hosted deployments.

Every number is read through report/showcase.py from benchmarks/, so the page
cannot drift from the committed results. Verified headlessly with Streamlit's
AppTest: ERBB2 rank 4, best ipTM 0.84, 50% success@0.65, mean PAE-int 13.7 A
-- matching benchmarks/designer_benchmark/RESULTS.md exactly.

Also: the About page linked to raw GitHub blob URLs rather than the deployed
documentation site, and the sidebar had no docs link at all.
Home. Replaces the plain title with a real landing page: hero, the pipeline
rendered as a nine-stage visual that marks which stages need a GPU, three
calls to action, and a headline stat strip. The stats are derived through
report/showcase.py from benchmarks/, so the page cannot claim more than the
committed results support.

Result persistence. The app used no st.session_state at all, so demo and
user-run output was rendered inside the `if st.button(...)` block and vanished
the moment any other widget was touched -- a user who ran their own cohort and
then adjusted a threshold lost the whole run. Results are now stashed and
rendered outside the click branch.

Cross-page navigation routes through a pending key that main() applies before
the radio is instantiated: Streamlit refuses assignment to a widget's own key
once that widget exists. Caught by the AppTest smoke suite.

Browse a run. The page asked for a server-side filesystem path, which a
visitor to the Hugging Face Space or Streamlit Cloud simply does not have. It
now finds local runs by their provenance manifest and offers them in a picker,
explains itself when there are none, and keeps the path box as a fallback.

Mobile. initial_sidebar_state is now explicit. All navigation lives in the
sidebar and Streamlit collapses it on phones by default, which left mobile
visitors on Home with no visible way to reach any other page -- while the
module docstring claimed the layout "works on phones".
CI asserted only that bindsight.report.webapp imports. Every page could raise
on render and the build would stay green -- on the app that is the project's
primary public face.

Fourteen tests now boot the real entry point and exercise each page, the
navigation, the injected theme, the published metrics, the structure picker,
and the Browse fallback. AppTest ships with Streamlit, so no new dependency;
the suite adds ~5 s and touches no network, since the pipeline pages only run
on a button click and no test clicks them.

This suite already earned its place: it caught the session-state navigation
bug in the preceding commit before it could reach a deployment.
The docs site is the surface that never cold-starts, so it should be what
convinces a first-time visitor. It was a plain mkdocs-material default in teal
while the app and the HTML report were navy, with no landing page, no logo, no
favicon, no social preview, and no mention of what the project has actually
demonstrated.

- docs/index.md is now a real landing page: hero, headline results, the
  pipeline as a nine-stage visual marking which stages need a GPU, and clear
  routes to the live app, the results, and the source.
- docs/results.md is new and generated by scripts/build_docs_results.py from
  the same report/showcase.py the app reads, so the two surfaces cannot
  disagree. It is committed so mkdocs serve works from a fresh clone, and
  tests/test_docs_results.py fails if it drifts from the benchmarks.
- Brand unified with bindsight/report/theme.py: navy palette, logo, favicon.
  The logo is drawn white because Material renders it on the primary colour
  bar -- the first version was navy-on-navy and invisible.
- Social previews finally have an image: og:image plus summary_large_image.

mkdocs build is now --strict, and it immediately paid for itself: nine links
in how-to-use, positioning, use-cases and what-is-bindsight pointed at
../ARCHITECTURE.md and friends, which resolve nowhere once the site is built.
Those have been 404s on the live site; they now point at the repository.

The figures are copied to docs/assets/figures rather than assets/results
because .gitignore excludes any directory named results/ -- so the copies were
silently untracked and would have been missing images on a fresh clone.
This project's fourth development principle is to be honest about what does
not work. A few docstrings had drifted ahead of the implementation, so they
now either describe reality or the code was changed to match.

Made true:
- `bindsight ui` documented itself as "no telemetry" and nothing enforced it.
  It now passes --browser.gatherUsageStats=false explicitly, which covers a
  packaged install launched from outside a checkout where .streamlit/
  config.toml is never read. No test asserts on this command's argv.

Corrected to match reality:
- report/html.py claimed the report embedded an NGL viewer from a CDN. It
  never has. The report is genuinely self-contained -- which is the more
  useful property, since it survives being emailed or opened offline -- and
  3-D viewing lives in the Streamlit app, where a CDN script is acceptable.
- report/streamlit_app.py claimed tables were "sortable + filterable via
  Streamlit's data_editor"; it uses st.dataframe, whose columns sort by
  clicking a header. Also points at webapp.py as the full interface.
- export/ro_crate.py claimed crates were bit-identical for the same manifest,
  while stamping datePublished with the current time. Payload files are
  byte-identical; the docstring now says exactly that.
- ARCHITECTURE.md referenced schemas/run_manifest.schema.json as validated on
  every run. That file does not exist; validation is Pydantic with
  extra="forbid". Also updated the py3Dmol claim, which is now true of the app.

No pipeline, CLI surface, config schema, or artifact format changes.
The rediscovery table blanked log2fc and padj for every antigen that was not
surfaced -- NECTIN4, FOLH1, MSLN, CEACAM5, EGFR -- because it read them from
`expected`, which the benchmark only populates when the antigen made the
shortlist. The measured differential expression lives in `deg_expected` and is
present for all six cohorts.

That was the wrong number to drop. The benchmark's whole framing is that
antigens are grouped by measured over-expression rather than clinical fame, so
NECTIN4 at log2fc 1.59 and FOLH1 at 1.32 -- real elevation that still fell
below the shortlist -- are exactly the rows the write-up is careful to
disclose. Showing them as missing understated the data.

Normalisation now lives in ValidationShowcase.rows(), so the web app and the
generated documentation page share it, and each cohort's explanatory note is
surfaced alongside its row.

Also in this commit:
- Ranks render as an em dash rather than a greyed "None": "not surfaced" is a
  reportable outcome, not absent data. Uses pd.isna, since building the frame
  coerces the mixed int/None column to float64 and the Nones arrive as NaN.
  The AppTest suite caught that one as a hard page failure.
- Dropped the redundant `project` column.
- Every predicted complex is downloadable as mmCIF, so the structures stay
  usable when a network blocks the CDN the 3-D viewer needs, or when someone
  would rather open them in PyMOL or ChimeraX. Stated plainly on the page.
- README points at the documentation site instead of raw GitHub blob paths,
  and CHANGELOG records this work under [Unreleased].
The web app's cached demo pointed at examples/demo/counts.tsv and design.tsv.
Those files do not exist -- tests/test_demo_e2e.py asserts they must stay
absent -- and the demo config carries a download: block, so the pipeline would
fetch the TCGA-BRCA cohort and write it inside the installed package. That
directory is read-only in the Docker image behind the Hugging Face Space, the
project's primary deployment.

`bindsight demo` has always done this correctly, caching the cohort under the
OS user-cache directory. The web app now does the same, which also means
whichever runs first warms the other instead of downloading twice.

The filename matters too: the GDC fetcher writes counts.tsv.gz and pandas
infers compression from the extension, so pointing at a bare .tsv would have
read a gzip stream as text even once the file existed.

Path handling is split into _demo_config() so it can be asserted without
executing the pipeline, and tests/test_webapp_demo_paths.py now covers a code
path that had no coverage at all.
@coderabbitai

coderabbitai Bot commented Jul 28, 2026

Copy link
Copy Markdown

Review Change Stack

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: b11ba600-c06c-49bf-afc9-b8b4aeccc925

📥 Commits

Reviewing files that changed from the base of the PR and between d216120 and 92c1cca.

⛔ Files ignored due to path filters (11)
  • docs/assets/favicon.svg is excluded by !**/*.svg
  • docs/assets/figures/antigen_rank.png is excluded by !**/*.png
  • docs/assets/figures/recall_at_k.png is excluded by !**/*.png
  • docs/assets/figures/volcano_blca.png is excluded by !**/*.png
  • docs/assets/figures/volcano_brca_her2.png is excluded by !**/*.png
  • docs/assets/figures/volcano_coad.png is excluded by !**/*.png
  • docs/assets/figures/volcano_luad.png is excluded by !**/*.png
  • docs/assets/figures/volcano_paad.png is excluded by !**/*.png
  • docs/assets/figures/volcano_prad.png is excluded by !**/*.png
  • docs/assets/logo.svg is excluded by !**/*.svg
  • docs/assets/og-image.png is excluded by !**/*.png
📒 Files selected for processing (62)
  • .github/workflows/ci.yml
  • .github/workflows/docs.yml
  • .streamlit/config.toml
  • ARCHITECTURE.md
  • CHANGELOG.md
  • README.md
  • Snakefile
  • bindsight/cli.py
  • bindsight/config.py
  • bindsight/design/developability.py
  • bindsight/design/prescreen.py
  • bindsight/export/ro_crate.py
  • bindsight/io/__init__.py
  • bindsight/io/paths.py
  • bindsight/pipelines/discover.py
  • bindsight/pipelines/full_run.py
  • bindsight/provenance/fragments.py
  • bindsight/report/__init__.py
  • bindsight/report/html.py
  • bindsight/report/showcase.py
  • bindsight/report/streamlit_app.py
  • bindsight/report/theme.py
  • bindsight/report/webapp.py
  • bindsight/runners/colab.py
  • bindsight/runners/job_exec.py
  • bindsight/runners/notebook.py
  • bindsight/structures/topology.py
  • bindsight/surfaceome/data/surfy_v1.uniprot.txt
  • bindsight/surfaceome/surfy.py
  • bindsight/targets/open_targets.py
  • docs/how-to-use.md
  • docs/index.md
  • docs/positioning.md
  • docs/results.md
  • docs/stylesheets/extra.css
  • docs/use-cases.md
  • docs/what-is-bindsight.md
  • mkdocs.yml
  • overrides/main.html
  • pyproject.toml
  • scripts/assemble_manifest.py
  • scripts/build_docs_results.py
  • scripts/build_surfy_list.py
  • scripts/make_og_image.py
  • scripts/run_deg.py
  • scripts/run_design.py
  • scripts/run_discover.py
  • scripts/run_rank.py
  • scripts/run_report.py
  • scripts/run_validate.py
  • tests/conftest.py
  • tests/test_cheap_profile.py
  • tests/test_docs_results.py
  • tests/test_package_imports.py
  • tests/test_report_lazy_import.py
  • tests/test_run_self_contained.py
  • tests/test_showcase.py
  • tests/test_snakemake_dag.py
  • tests/test_surfy_offline.py
  • tests/test_surfy_vendored.py
  • tests/test_webapp_demo_paths.py
  • tests/test_webapp_smoke.py

📝 Walkthrough

Walkthrough

This PR adds offline SURFY discovery, portable run artifacts, structured provenance, optional cheap design pre-screening, benchmark-backed reporting, branded documentation, and stricter CI validation.

Changes

Bindsight execution and provenance

Layer / File(s) Summary
Structured provenance and Snakemake execution
Snakefile, bindsight/provenance/*, scripts/run_*.py, scripts/assemble_manifest.py, bindsight/export/ro_crate.py, tests/test_snakemake_dag.py, tests/test_run_self_contained.py
Stage fragments now provide structured records. Snakemake assembles the manifest before reporting. RO-Crate exports include referenced files and matching SHA-256 values.
Offline discovery and portable artifacts
bindsight/io/*, bindsight/pipelines/discover.py, bindsight/surfaceome/*, bindsight/surfaceome/data/*, pyproject.toml, tests/test_surfy_*.py, tests/test_run_self_contained.py
SURFY uses cache, vendored data, and controlled fallback order. Structures are copied into runs and resolved through portable or legacy paths.
Cheap profile and ESM-2 pre-screening
bindsight/config.py, bindsight/cli.py, bindsight/design/prescreen.py, bindsight/runners/job_exec.py, bindsight/pipelines/full_run.py, tests/test_cheap_profile.py
--cheap applies RFdiffusion, ProteinMPNN, T4, and 10 trajectories. Optional ESM-2 screening selects representative designs before validation and records fallback or screening details.

Reporting and presentation

Layer / File(s) Summary
Benchmark loaders and Streamlit results
bindsight/report/showcase.py, bindsight/report/theme.py, bindsight/report/webapp.py, bindsight/report/__init__.py, tests/test_showcase.py, tests/test_webapp_smoke.py
The app now loads committed validation and designer benchmarks, displays real results and structures, persists run state, supports local run browsing, and uses shared branding.
Generated documentation and branded presentation
docs/*, mkdocs.yml, overrides/main.html, scripts/build_docs_results.py, scripts/make_og_image.py, README.md, CHANGELOG.md, .streamlit/config.toml
The documentation site and README expose the new results page. Generated benchmark content, responsive styling, social metadata, Streamlit settings, and changelog entries are added.

CI and type safety

Layer / File(s) Summary
CI enforcement and annotations
.github/workflows/ci.yml, .github/workflows/docs.yml, bindsight/**/*.py, tests/conftest.py, pyproject.toml
Mypy failures now block CI. Linux tests install the workflow extra and retain coverage. Documentation builds generate results and use strict link checking. Type annotations and cache isolation are expanded.

Estimated code review effort: 5 (Critical) | ~120 minutes

Possibly related PRs

Suggested reviewers: claude

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/bindsight-repo-structure-t8ey1h

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

The docs workflow installs the package to run scripts/build_docs_results.py,
but `pip install -e "."` alone is not enough: importing bindsight.report runs
its __init__, which imports report/html.py, which imports pandas at module
scope. The generator would have died with ModuleNotFoundError on the first
push to main.

The pull-request checks cannot catch this -- docs.yml only triggers on pushes
to main -- so it was verified by reproducing the job in a clean virtualenv:
core install + pandas + mkdocs-material generates the page and builds the site
with --strict, while core install alone fails exactly as described.

pandas rather than the whole [discover] extra, which would drag pydeseq2,
pyarrow, numpy, biopython and gql into what is only a Markdown build.
An earlier commit forced initial_sidebar_state="expanded" so mobile visitors
would not land on Home with no visible navigation. Checking it on a 390px
viewport showed the cure was worse: Streamlit renders the mobile sidebar as a
full-width overlay, so the hero and the headline results -- the entire point
of the landing page -- were hidden behind a panel the visitor had to dismiss
before seeing anything.

Back to "auto" (expanded on desktop, collapsed on phones). Mobile navigation
is solved on the page instead: the Home call-to-action buttons reach Real
results, Demo and Run directly, and Streamlit still renders its own control to
open the sidebar. A phone now lands on the hero, the four headline stats in a
2x2 grid, and three full-width buttons.
bindsight/report/__init__.py eagerly imported report/html.py, and therefore
pandas and jinja2, for anyone touching anything under the package. That
defeated the design of report/theme.py and report/showcase.py, both written to
be stdlib-only at module scope precisely so they work in minimal environments.

It was not theoretical: the documentation workflow installs neither pandas nor
jinja2, and the results generator only needs showcase.py. It had to be given
pandas explicitly to work around this. That workaround is reverted here -- the
core install is enough again.

render_run is now resolved through a PEP 562 module __getattr__. The hook
raises AttributeError, never ImportError, because `from bindsight.report import
showcase` consults __getattr__ before falling back to the submodule import
machinery -- seven call sites depend on that fallback and would break otherwise.

tests/test_report_lazy_import.py proves the property in a clean subprocess
(pandas is already resident in the pytest process, so an in-process assertion
would prove nothing), and covers the submodule imports, the AttributeError
contract, and that render_run is still the same object as the direct import.
…it at all

The surfaceome could not be obtained on any machine. The URL the package
shipped with (wlab.ethz.ch) now serves an HTML landing page, so pandas handed
those bytes to openpyxl and users saw "BadZipFile: File is not a zip file".
The relocated host (wollscheidlab.org) serves a 132-byte Git-LFS pointer, not
the spreadsheet. Neither is retrievable.

The consequences were silent and severe. With the default
surfy_allow_offline_fallback=false a real run hard-failed. With the demo's
true it degraded to the ten-protein bundled list, so 298 of 300 enriched genes
were discarded as not_surfaceome and the pipeline surfaced nothing. Nobody
could have told from the output that the surfaceome was missing.

What still works upstream is surfaceome_ids.txt, which lists all 2,886 surface
proteins by UniProt entry name. bindsight joins on accessions, so
scripts/build_surfy_list.py resolves that mapping once against UniProt (2,695
in one bulk request, 191 individually, zero unresolved) and vendors the result
as bindsight/surfaceome/data/surfy_v1.uniprot.txt, with the CC-BY attribution
the licence requires. The generator refuses to write a partial list.

load_surfy() now resolves cache -> vendored full list -> tiny fallback, and
the discovery path no longer reaches for the network at all: retrying a
download that cannot succeed would only have added latency before the same
failure. allow_offline_fallback now gates the ten-protein list alone -- the
thing that silently degrades results -- since the vendored list is the real
SURFY set.

populate_surfy_cache survives as an explicit refresh path, repaired: a @Retry
matching targets/gtex.py, and a content check that names what came back
(HTML page, Git-LFS pointer, non-zip bytes) instead of BadZipFile.

Verified end to end: `bindsight demo` on real TCGA-BRCA data now reports
"surfaceome filter: 301 -> 32" and returns 32 candidates with AlphaFold
structures, where before this commit it reported 300 -> 0. A subprocess test
with DNS and connect() blocked proves resolving the surfaceome touches no
network.
io/paths.py has always documented <run>/structures/ and <run>/config.yaml as
part of the run layout. Neither was ever written. run_dir() created an empty
structures/ directory, the AlphaFold model stayed in the machine-local cache,
and candidates.parquet stored its absolute path -- so a run, and every RO-Crate
exported from one, was valid on exactly one machine. That directly undercuts
the project's central claim, which is that a designed binder can be walked back
to the evidence that produced it.

Root fix, not a patch at the export boundary:

- io/paths.adopt_structure() copies each fetched structure into the run and
  returns a run-relative reference. Copying rather than linking is deliberate:
  the cache is shared and prunable, and a link would let an unrelated cleanup
  silently gut a finished, exported run.
- io/paths.resolve_run_path() resolves either form. Runs made before this
  change stored absolute paths and must keep working, so both are accepted.
  cli._top_targets resolves once, and every design/validate consumer funnels
  through it unchanged.
- The effective config -- after CLI overrides -- is written to <run>/config.yaml
  and recorded as the manifest's config_path, which previously pointed at the
  counts *directory* rather than any config at all.
- The exporter picks up the artifacts it was silently dropping: the failure
  taxonomy (a manifest-declared output the HTML report already reads), design
  metrics, per-target archives, per-binder validator output, the structures,
  and the config; and it carries the sha256 digests the manifest already held
  into the crate metadata, so a reader can verify what they received.

Also adds BINDSIGHT_CACHE_DIR to io/paths. XDG_CACHE_HOME is ignored by
platformdirs on macOS (which is in the CI matrix) and monkeypatch cannot cross
a process boundary, so this is the only cache-redirect seam that works
everywhere -- needed next to test the Snakemake front-end.

Verified on a real TCGA demo run: structures/ holds 25 models, all 25
references are run-relative, mean_plddt still resolves for every one, and the
exported crate unzipped on an unrelated path resolves 25 of 25 references with
none dangling.
ARCHITECTURE.md §10 specified the profile as "RFdiff+MPNN on T4, 10
trajectories, ESM-2 pre-screen". Click accepted the flag and bound it to a
parameter that was never referenced again, so a user asking for the cheap
profile got the full-price run -- and --dry-run quoted A100 pricing for it.

All three parts now exist:

- designer=rfdiff_mpnn and n_trajectories=10, applied between the existing
  override block and the --dry-run branch so the printed estimate describes
  the run that would actually happen.
- DesignParams.gpu_type, threaded into cost.estimate_full_run. That function
  has always accepted gpu_type and no caller ever passed it, so every estimate
  silently used the backend default. On the demo config against Modal the
  estimate goes from ~$27.89 (A100-40GB) to ~$4.26 (T4).
- bindsight/design/prescreen.py: embeds designed sequences with ESM-2 and
  keeps those nearest the embedding centroid, applied in job_exec.run_job
  between design and validation -- the only point where dropping a design
  actually saves GPU, since the executor runs both halves in one job.

The pre-screen is the only change here that can alter scientific output, so it
is deliberately conservative. It is off unless prescreen_top_k is set, so
default runs are bit-identical. It degrades to validating everything when the
optional embed extra is missing or embedding fails -- losing an
already-successful GPU run to a missing dependency would be indefensible. It
preserves design order, breaks ties on input order so selection is
reproducible, and reports exactly which designs it screened out, because
silently discarding candidates would contradict the project's own rule that
failures are recorded rather than swallowed.

ARCHITECTURE.md now describes the profile as shipped rather than planned.
… in CI

The Snakemake front-end could not run at all, and had not been able to for
roughly five weeks. Two independent breakages, both hidden by the same cause:
tests/test_snakemake_dag.py was slow-marked, so CI never ran it.

1. scripts/run_discover.py called _do_discover() with six of its eight
   required keyword-only arguments. topology_client and gtex_client were added
   to the signature in June and the wrapper was never updated, so the rule
   raised TypeError before doing any work.
2. scripts/assemble_manifest.py began with `from __future__ import
   annotations`. Snakemake prepends its own preamble to script files, which
   pushes that off line 1, and Python rejects it outright. The manifest rule
   had never executed. It is the only Snakemake-executed script with that
   import; the standalone CLI scripts are unaffected.

Meanwhile ARCHITECTURE.md claimed `bindsight run <cfg> ≡ snakemake`.

Provenance is now genuinely shared rather than reimplemented. The new
bindsight/provenance/fragments.py builds the same StageRecord objects both
front-ends use, and each rule records real inputs, outputs, sha256 digests,
params and timings. Snakemake manifests previously had empty inputs, empty
outputs, no digests anywhere, a placeholder tool repeated for all six stages,
and ended_at=None -- a completion log, not provenance, while the assembler's
docstring claimed it emitted "the same populated provenance artifact the Click
CLI does". Older fragments still assemble.

Two more silent divergences fixed:
- The manifest was assembled after the report rendered, and report/html.py
  reads the manifest to build its provenance table -- so every Snakemake report
  shipped with an empty Provenance section. The manifest rule now runs first
  and report appends its own terminal record.
- scripts/run_rank.py never passed weights, so the Snakefile's params.rank
  block was silently discarded while the CLI honoured it.

The DAG test now runs offline and in CI. Hermeticity comes from the
BINDSIGHT_CACHE_DIR seam: the DAG is a subprocess and each rule another, so
conftest's monkeypatch-based offline fixture cannot reach it, and
XDG_CACHE_HOME is ignored by platformdirs on macOS. The fixture seeds a SURFY
list and minimal parseable mmCIF models instead. Module-scoped so the DAG runs
once (~13 s rather than ~68 s), Linux-only because Snakemake's Windows support
is unreliable and a flaky signal is worse than none.
pyproject has set `strict = true` all along while ci.yml ran `mypy bindsight
|| true`, so the result was computed and thrown away. 53 errors had
accumulated behind it. Coverage was configured in pyproject and then disabled
at the command line with --no-cov, so it had never run either.

Both are now real. mypy reports zero errors across 70 source files and gates
merges; the suite reports 77% coverage.

The fixes are annotations, not logic:

- 28 bare `dict` generics given parameters, with the `Any` imports they
  needed. Twelve of those were in the showcase module added earlier in this
  branch, so this is partly cleaning up after myself.
- Missing return/parameter annotations on eight functions.
- json.loads / DataFrame.to_dict return Any; the narrowing is now explicit
  rather than implicit.
- JobHandle.model_extra is Optional under extra="allow", so colab.py's two
  `.get()` calls were genuine latent AttributeErrors on a handle without
  extras.
- theme.PAGE_LAYOUT is a Literal, because st.set_page_config types that
  parameter as Literal["centered", "wide"]. LAYOUT stays as an alias.
- discover.py's `[None]` sentinel for "gene mapped to no UniProt accession" is
  now typed and commented rather than looking like an oversight -- the row is
  deliberately kept so the failure taxonomy can record no_uniprot instead of
  dropping it silently.

pandas gets an ignore_missing_imports override alongside the eight libraries
that already have one. pandas-stubs was tried first and rejected on evidence:
it clears the 12 "no stubs" errors but introduces ~16 overload conflicts
against correct code, chiefly .loc indexing, each needing a cast or ignore
that a stub update could invalidate. The goal is strict checking of
bindsight's own code, and the override keeps that without the churn.
Client tests call cache_dir("afdb_test") and friends, which created
directories inside ~/.cache/bindsight and left them there after every run --
afdb_test, opentargets_test, opentargets_test_none accumulating in a real
user's cache.

An autouse session fixture now points BINDSIGHT_CACHE_DIR at a temporary
directory, so the suite cannot touch the real cache. The env var was added for
the Snakemake DAG test and it solves this too, including for tests that shell
out, which a monkeypatch could never have covered.

Also records this round of engine work in CHANGELOG.md under [Unreleased].
offline_real_data isolated the cache by patching io.paths.user_cache_path.
cache_root() now consults BINDSIGHT_CACHE_DIR first and only falls back to
platformdirs, so that patch had quietly become a no-op: those tests were
sharing the session-wide cache from _isolate_cache instead of getting a fresh
one each, which would let state leak between them.

It now sets the env var, which is both the real seam and the one that reaches
subprocesses. Also corrects _isolate_cache's return annotation -- it is a
generator, not a Path.
@mikhaeelatefrizk mikhaeelatefrizk changed the title Complete the presentation layer: surface the real results the repo already had Complete the presentation layer, then fix six engine fragilities at the root Jul 29, 2026
@mikhaeelatefrizk
mikhaeelatefrizk marked this pull request as ready for review August 1, 2026 23:50
@mikhaeelatefrizk
mikhaeelatefrizk merged commit 805197c into main Aug 1, 2026
9 of 10 checks passed
@mikhaeelatefrizk
mikhaeelatefrizk deleted the claude/bindsight-repo-structure-t8ey1h branch August 1, 2026 23:50
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.

1 participant