Add a gnomAD & Hail institutional-knowledge reference by jkgoodrich · Pull Request #843 · broadinstitute/gnomad_methods

jkgoodrich · 2026-08-05T02:38:22Z

Summary

Adds knowledge/, a shared reference of gnomAD + Hail institutional knowledge maintained by the methods group: bug classes that produce plausible-looking wrong output rather than crashing, design tradeoffs that aren't visible in the code, and facts about the released data that are otherwise buried in the source or in meeting minutes.
Publishes it to the documentation site alongside the API reference, so it's reachable by people outside the team.
Adds the repo-level assistant configuration that points at it, plus the team's shared Claude Code skills.

Changes

knowledge/ (14 files, ~2,900 lines). A routing README plus topic sub-documents — missingness, Hail idioms, lazy evaluation/materialization, partitioning/shuffles/OOM, hidden APIs, gnomAD-specific, VDS, biology, analysis patterns, Dataproc/operational, release conventions, testing, and working with an AI assistant. The README is an index: a reader loads only the sub-document they need rather than the whole reference.
Entry conventions. Every entry is tagged [Hail] and/or [gnomAD data], marked [internal only] where it describes artifacts outside the public release, and stamped with the Hail version where behavior is version-dependent. Contribution rules are in the README.
Docs wiring. docs/build.sh stages a copy of knowledge/ into the Sphinx source dir the way api_reference is generated; docs/.gitignore ignores the copy; docs/index.rst gains a toctree entry; docs/conf.py sets myst_heading_anchors = 3 so cross-document file.md#heading links resolve. The canonical copy stays at the repo root — readable on GitHub, not coupled to the build.
CLAUDE.md (new, deliberately minimal): a pointer to knowledge/README.md and its contribution rules, a verbosity rule, and the CLAUDE.md vs CLAUDE.local.md split. Broader repo context is left to the separate PR that adds it.
.gitignore: ignores CLAUDE.local.md, which was untracked but not ignored.
.claude/settings.json (new): allowlists read-only inspection commands (git status/log/diff/show, ls, grep, find, wc, head, tail, pytest, ./docs/build.sh).
.claude/skills/: the team's shared commit / review / PR-draft / history-cleanup workflows. review is bw2's MIT-licensed skill, vendored with its LICENSE.

Hail / Compute notes

No pipeline code, no Hail operations, no resource paths, and no schema changes — the only Python touched is a single config line in docs/conf.py. Nothing here reads or writes GCS, and there is no compute cost.

Claims about Hail and gnomAD data in the reference were verified against Hail 0.2.134 source, the released v4.1 exomes HT globals, and small local jobs. Entries that come from the project's meeting record rather than from re-derivation are labeled reported · project meeting so a reader can weigh them differently.

Testing

Docs build verified locally against the pinned dependencies CI uses (sphinx 6.2.1, myst-parser 2.0.0) with sphinx -W, exactly as ./docs/build.sh runs it: build succeeds, all 13 knowledge pages render.
Every cross-page anchor link in the generated HTML was checked against the ids that actually exist on the target pages — none broken.
No unit tests added: this PR contains no library code. pytest and lint jobs are unaffected.
The docs CI job now covers this content. A malformed entry fails the build rather than merely rendering oddly on GitHub — that's intentional, and the contribution rules tell contributors to run ./docs/build.sh before opening a PR.

Code review

/review run — models: Opus, Sonnet (detection and validation; agy not installed, so not in the roster)

Three findings, all agreed by both validators. Two are fixed in this branch:

Fixed: CLAUDE.md documented an import block in which 12 of 20 names did not exist anywhere in the repo. The file is now cut to the minimum described above, so the list is gone entirely.
Fixed: the draft-PR skill inferred the reviewer roster from detection prompts alone, assuming detection and validation share a roster; the review skill documents them as independent. It now reads both and reports what actually validated.
Not fixed (pre-existing on main): docs/conf.py calls requests.head(...) at config load with no try/except and no timeout=, so an offline or network-restricted environment aborts or hangs the docs build instead of degrading to the no-intersphinx path. This branch didn't touch those lines, but the new contribution rules make people run ./docs/build.sh more often, so it's worth a follow-up PR.

Reviewer notes

This PR puts documentation under CI. The docs job builds with sphinx -W, so from here on a malformed entry in knowledge/ fails the build. Worth agreeing that's the tradeoff you want.
.claude/settings.json changes permission behavior for everyone on the repo, not just the author. It's read-only commands and a local build, but it's the one change here with a blast radius beyond documentation — easy to trim or drop.
Please sanity-check the [internal only] tagging. Parts of the release-conventions and biology entries came from internal meeting records; only facts about released data and public-facing policy were kept, with no names, cost figures, or unpublished work. A second pair of eyes on that boundary is the most valuable review this PR can get.
Domain review welcome on the data claims — annotation-version pinning and its GTEx/pext coupling, LOEUF cutoffs not porting across releases, which subsets exist and why, and the CMRG false-duplication genes (CBS, KCNE1, CRYAA).
CLAUDE.md is intentionally minimal here to avoid colliding with the separate PR that adds the fuller version.

Merge strategy

Rebase, not squash. The five commits are deliberately structured — reference content, docs publishing, contributor config, permissions, and tooling each stand alone and are individually revertible. The .claude/settings.json commit in particular is the one a reviewer might want to drop on its own, which squashing would make awkward.

A shared reference for anyone -- AI or human -- working with gnomAD data in Hail, covering the knowledge that normally only comes from having hit the problem: bug classes that produce plausible-looking wrong output rather than crashing, design tradeoffs that aren't visible in the code, and facts about the released data that are hidden in the source. Organized as a routing README plus topic sub-documents, so a reader loads only what's relevant instead of the whole reference: - missingness -- null and NaN propagate differently and inconsistently; hl.max drops missing but keeps NaN while hl.agg.max ignores both, dict.get's default covers an absent key but not a null value, and parse_json with a mismatched schema returns nulls rather than erroring. - hail-idioms -- source-object binding, large literals, the codegen size cliff, joins that are secretly shuffles, scans, the absence of Python UDFs, and hoisting fields out of a localized entries array. - lazy-eval-and-materialization -- what triggers execution, why a logging count re-runs everything upstream, and cache vs persist vs checkpoint. - partitioning-shuffles-oom -- what actually shuffles, repartition vs coalesce vs read-time partitioning, how to read an OOM trace, and telling a preemption problem from a code problem. - gnomad-specific, vds, biology -- filter semantics, freq array indexing, the column and site traps when subsetting a VDS, reference false duplications, and what QUAL and LOFTEE do not tell you. - analysis-patterns, dataproc-operational, dataset-conventions -- row-set anchoring, transcript picking, cluster and storage costs, and why the released data is shaped the way it is. - testing, hidden-apis, working-with-ai. Every entry is tagged [Hail] or [gnomAD data], marked [internal only] where it describes artifacts outside the public release, and stamped with the Hail version where behavior is version-dependent. Claims are verified against the Hail source, the released tables, or a job that was actually run; the few that come from the project's meeting record rather than from re-derivation are labeled as such. Assisted-by: ClaudeCode:claude-opus-5

Renders knowledge/ alongside the API reference at broadinstitute.github.io/gnomad_methods, without moving the canonical copy under docs/ where it would be coupled to the build and awkward to read on GitHub. build.sh stages a copy into the Sphinx source directory the same way api_reference is generated, and .gitignore ignores that copy. index.rst gains a toctree entry; the tree's README carries a hidden toctree so every sub-document has a parent. myst_heading_anchors makes cross-document links of the form file.md#heading resolve to real anchors. Note this puts the reference under CI: the docs job builds with sphinx -W, so a malformed entry now fails the build rather than merely rendering oddly on GitHub. The contribution rules say to run ./docs/build.sh before opening a PR for that reason. Assisted-by: ClaudeCode:claude-opus-5

A deliberately minimal CLAUDE.md carrying only what this branch introduces: a pointer telling an assistant to consult knowledge/README.md and its contribution rules before writing gnomAD- or Hail-touching code, and a verbosity rule, since too much prose is the default failure mode and the rule is worth stating where it will be read. Broader repo context -- package layout, code style, the API listings -- is left to the separate PR that adds it. CLAUDE.local.md is the per-developer counterpart, holding absolute paths, cluster names, and scratch buckets. It was untracked but not ignored, which is one `git add -A` away from committing someone's machine-specific configuration; this ignores it. The split between the two files is by portability, and knowledge/working-with-ai.md documents how to bootstrap both. Assisted-by: ClaudeCode:claude-opus-5

Inspection commands an assistant runs constantly here -- git status/log/diff/show, ls, grep, find, wc, head, tail, pytest, and the docs build -- each cost a permission round-trip, which is a turn spent on nothing. This allowlists exactly those. Everything listed is read-only or a local build. Nothing that writes to the repo, pushes, or touches the cloud is included; those should keep prompting. Delete the file or trim the list if the team would rather approve each one. Assisted-by: ClaudeCode:claude-opus-5

Project-scoped .claude/skills/ so the team shares the same commit, review, PR-draft, and history-cleanup workflows rather than each person reinventing them. review is bw2's MIT-licensed claude-code-review-skill, vendored with its LICENSE. Three corrections to the vendored-in workflows, each a case of a skill asserting something that does not survive contact with real output: - commit and clean-git-history take an `Assisted-by: ClaudeCode:<model-id>` trailer as a placeholder rather than a fixed model name, and say to use the model actually running and not to copy one from an earlier commit. A hardcoded name silently goes stale and makes the trailer useless for the one thing it records. - draft-pull-request-description inferred the reviewer roster from detection prompts alone, on the assumption that detection and validation share a roster. The review skill documents them as independent -- a reviewer can pass detection and drop out of validation -- so the PR description could credit a model that never validated anything. It now reads both prompt sets and reports what actually validated, since that is what the confidence labels rest on. - The same skill's PR template carried both a checked "review run" and an unchecked "review not run" line, so following it literally emitted two contradictory statements. A skimmer reads the first and concludes no review happened. The template now emits one line and says to delete the other. Assisted-by: ClaudeCode:claude-opus-5

ch-kr · 2026-08-07T17:09:18Z

+| sub-doc | consult it when |
+|---|---|
+| [Missingness](missingness.md) | you touch a nullable field, a `hl.case` / `hl.if_else`, a `group_by` on a derived key, or a max/min over floats that could be NaN |
+| [Hail idioms](hail-idioms.md) | you rebind `ht`, build a big literal or many-branch expression, join with `ht[key]`, sort with `order_by`, use scans, or reach for a Python UDF |
+| [Lazy evaluation and materialization](lazy-eval-and-materialization.md) | you add a logging `.count()`, or you're choosing between `cache` / `persist` / `checkpoint` |
+| [Partitioning, shuffles, and OOM](partitioning-shuffles-oom.md) | a job dies in a shuffle, you're about to `repartition`, or you want to know what shuffles in the first place |
+| [Hidden and undocumented APIs](hidden-apis.md) | you meet an underscore-prefixed Hail parameter in gnomad_qc, or want the `hl.experimental` helpers we use |
+| [gnomAD-specific](gnomad-specific.md) | you filter variants, index into `freq`, or cross HTs with different variant sets |
+| [VDS](vds.md) | you subset or densify a VDS, or you're choosing between Table / MatrixTable / VDS |
+| [Biology](biology.md) | reference builds, multiallelics, sex chromosomes, trios, AC/AF null semantics, or what QUAL and LOFTEE do and don't tell you |
+| [Release conventions and data policy](dataset-conventions.md) | you wonder why the data is shaped the way it is — annotation versions, subsets, constraint cutoffs across releases, labels, requester-pays |
+| [Codebase and analysis patterns](analysis-patterns.md) | you pick a starting table, a VEP transcript, a bucket scheme, or a per-stratum aggregation |
+| [Dataproc / operational](dataproc-operational.md) | you run, size, package, or post-mortem a job on Dataproc or QoB, or you're wondering why storage costs what it does |
+| [Testing](testing.md) | after any refactor of the join graph or aggregation semantics, and before you pay for a cluster run |
+| [Working with an AI assistant](working-with-ai.md) | you're setting up `CLAUDE.md` / `CLAUDE.local.md`, or wondering what's burning tokens and cluster time |


we should break this down into two (or more, if there are other categories) tables (one for Hail, one for gnomAD) to further stratify

we could split the table based on the tags below

ch-kr · 2026-08-07T17:11:03Z

+Entries are tagged **`[Hail]`** (universal Hail behavior) and/or
+**`[gnomAD data]`** (dataset- or schema-specific). Entries tagged
+**`[internal only]`** describe data or artifacts available only to the
+internal production team — unreleasable samples, the pre-release VDS,
+internal QC tables. Everything untagged that way applies to anyone
+working with released gnomAD data.


maybe we should further stratify internal only to split production (QC) work and research work, since some research projects require VDS access but don't need to reference QC outputs

ch-kr · 2026-08-07T17:12:13Z

+## For AI or human reading this file
+
+**What this file is:** a shared reference maintained by the gnomAD
+methods group. It captures data-model quirks, Hail idioms, codebase
+conventions, and design-choice tradeoffs — the kind of institutional
+knowledge that usually only gets learned by running into the problem
+(or being told by someone who already did).
+
+**When to consult it:** any time you're touching gnomAD annotation
+tables, writing Hail aggregations, filtering by QC status, making
+assumptions about coverage between HTs, or picking between two
+reasonable-sounding analysis approaches. See the [When to consult this
+](#when-to-consult-this) section at the bottom for the concrete trigger
+list.


move to top

ch-kr · 2026-08-07T17:15:32Z

+  **`[gnomAD data]`** (dataset/schema-specific), or both. Add
+  **`[internal only]`** for anything that applies solely to the internal
+  production team (unreleasable samples, internal bucket paths).
+- **Hail version** — whenever the behavior is version-dependent, state


also add a section to describe known buggy version of Hail versions that aren't described in the changelog but are findable in zulip, the Hail forum, Hail PRs (e.g., requester-pays is broken in Hail 0.2.138), or comments in the gnomad_qc repo

also add a note which Hail versions are not backwards-compatible and which versions changed table/data formats under the hood

ch-kr · 2026-08-07T17:17:27Z

+
+---
+
+## When to consult this


do we need this? are we able to condense this section?

ch-kr · 2026-08-07T17:21:59Z

+
+Entries are tagged **`[Hail]`** (universal Hail behavior) and/or
+**`[gnomAD data]`** (dataset- or schema-specific). Entries tagged
+**`[internal only]`** describe data or artifacts available only to the


we should add a note to the internal section to make sure that code changes to one repo (gnomad_methods or gnomad_qc) do not break code in the other repo

ch-kr · 2026-08-07T17:22:38Z

@@ -0,0 +1,29 @@
+# gnomad_methods
+
+Shared Hail utility library for gnomAD pipelines. **This is a library, not a pipeline** — it exposes APIs that other repos (`gnomad_qc`, `gnomad-constraint`, and others) import, so public-API changes ripple outward. Prefer additive changes over breaking renames.


add comment here that code changes in this repo need to be checked against gnomad_qc (no breaking changes)

ch-kr · 2026-08-07T17:33:52Z

@@ -0,0 +1,49 @@
+# claude-code-review-skill


@jkgoodrich can you confirm this is the best way to use a skill from another GitHub repo?

https://github.com/bw2/claude-code-review-skill

ch-kr · 2026-08-07T17:35:19Z

+       not run: - [ ] `/review` not run
+     If a review was run, follow the line with its findings and their disposition
+     (fixed here / deferred / rejected as a false positive). -->
+- [x] `/review` run — models: <!-- e.g. Opus, Sonnet -->


should also add whether any libraries added in the pull request are compatible with existing dependencies in the repo. if not, suggest fixes

ch-kr · 2026-08-07T17:37:51Z

there should be a section somewhere in this skill specifying that all code (new or updated) needs to have tests

ch-kr

I ran /doctor on this branch just to see what claude would think. here are the relevant points (it also flagged the conflict between this branch and the newly merged CLAUDE.md work, so I haven't copied those comments over):

.claude/settings.json pre-approves several commands that aren't read-only (details inline) — and since it's checked in, it applies to every contributor's sessions.

For this point, it flagged:

  .claude/settings.json line 8 — "Bash(git branch:*)"

  ▎ This one isn't read-only: git branch <name> creates a branch, -D deletes one, and -f main <sha> force-moves one. :* rules are prefix string
  ▎ matches with no flag analysis, so all of those are pre-approved. Suggest replacing with the exact read-only forms:
  ▎ "Bash(git branch)",
  ▎ "Bash(git branch -a)",
  ▎ "Bash(git branch --show-current)",
  ▎ "Bash(git branch --contains:*)"

  .claude/settings.json line 13 — "Bash(find:*)"

  ▎ find -exec <cmd> is arbitrary command execution and -delete removes files, so this wildcard is effectively an execute/delete grant, not a
  ▎ read-only one. Suggest dropping it — Claude Code's built-in Glob/Grep tools cover most of what find gets used for without a permission rule.

  .claude/settings.json line 11 — "Bash(rg:*)"

  ▎ ripgrep's --pre <cmd> runs an arbitrary program on every file searched, so rg:* is also an execution grant. Either drop it (the built-in Grep tool
  ▎ uses ripgrep internally and needs no rule) or keep it with that tradeoff stated in the PR description.

  .claude/settings.json lines 5–7 — git log / git diff / git show wildcards

  ▎ Lower severity, but worth knowing: all three accept --output=<file>, which writes an arbitrary file, so the :* wildcards admit a write path. If
  ▎ you want to be strict, exact rules for the common invocations are the safe pattern; if the team is fine with this residual risk, no change needed
  ▎ — just deciding it consciously.
  ▎
  ▎ Related: lines 17–19 (pytest, python -m pytest, ./docs/build.sh) execute repo code by design. That's a defensible choice for a dev repo, but the
  ▎ commit message says "routine read-only commands" — suggest the PR description name the execution grants explicitly so they're approved as such.

I don't have strong feelings about these points, e.g., I'm ok with claude being able to create/delete/move branches

The allowed-tools frontmatter values in three skills don't match real Claude Code tool names and are silently ignored (details inline).

  .claude/skills/commit/SKILL.md line 4 (same comment applies to clean-git-history/SKILL.md:4 and draft-pull-request-description/SKILL.md:4)

  ▎ allowed-tools: bash, read_file — Claude Code tool names are case-sensitive title-case, and read_file isn't a Claude Code tool name (these look
  ▎ ported from a different harness). Unrecognized names are silently ignored, so this restriction currently does nothing. Fix:
  ▎ allowed-tools: Bash, Read

ch-kr · 2026-08-27T16:11:41Z

(this might not be the right file, but wanted to drop this comment somewhere) this came up in Kaitlin's lab meeting today, but maybe we should add that 0.2.128 is the most stable Hail version for densify, and double check that we ask AI to verify when format changes make Hail incompatible with previous versions

jkgoodrich added 5 commits August 4, 2026 12:39

ch-kr mentioned this pull request Aug 7, 2026

Add CLAUDE.md #839

Merged

ch-kr reviewed Aug 7, 2026

View reviewed changes

ch-kr reviewed Aug 20, 2026

View reviewed changes

ch-kr reviewed Aug 27, 2026

View reviewed changes

		@@ -0,0 +1,29 @@
		# gnomad_methods

		Shared Hail utility library for gnomAD pipelines. This is a library, not a pipeline — it exposes APIs that other repos (`gnomad_qc`, `gnomad-constraint`, and others) import, so public-API changes ripple outward. Prefer additive changes over breaking renames.

Uh oh!

Conversation

jkgoodrich commented Aug 5, 2026

Summary

Changes

Hail / Compute notes

Testing

Code review

Reviewer notes

Merge strategy

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ch-kr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants