Detect pangenome reference graphs from rGFA content (#148) by NoopDog · Pull Request #151 · DataBiosphere/meta-disco

NoopDog · 2026-07-09T07:23:43Z

Closes #148.

Problem

After #146, pangenome.reference was detected purely from the -mc- filename token. That missed the two HPRC minigraph (non-cactus) reference graphs, which classified as plain pangenome:

hprc-v1.0-minigraph-grch38.gfa.gz
hprc-v1.0-minigraph-chm13.gfa.gz

Approach

Rather than add another filename token, determine it from file content — the repo's accuracy over efficiency principle ("prefer reading actual file content over guessing from filenames... even if it requires downloading headers").

In rGFA every segment carries a stable rank SR:i: and stable name SN:Z:. Rank 0 is the reference backbone, so a graph whose segments carry rank-0 stable sequences defines a reference coordinate system. These tags sit on the very first segments, so a 256KB range request is both exact and cheap.

Verified by range-requesting the real files from the public human-pangenomics bucket:

S  s1  <seq>  LN:i:10621  SN:Z:chr1  SO:i:0  SR:i:0

Minigraph-cactus, by contrast, is plain GFA 1.0 with untagged segments; its PanSN P/W path lines follow every segment line, ~11 GB into the file. Content inspection is not economical there, so MC keeps the -mc- filename rule.

Changes

fetchers.py — parse_gfa_segment_tags + fetch_gfa_segment_tags. Locates tag columns by tab position so the segment sequence (the bulk of an S line, ~1MB per head) is never copied; drops the record truncated by the byte-range cut.
header_classifier.py — classify_from_gfa_segment_tags, emitting rgfa_stable_rank_reference.
file_types.py — GFA_CONFIG, scoped to text GFA (.gfa, .gfa.gz, .rgfa, .rgfa.gz). The binary vg/GBWT formats (.gbz, .vg, .gbwt, .xg) still classify from extension and filename alone.
Docs + 13 tests, golden fixture regenerated.

Verification against real S3 files

file	segments parsed	rank-0	data_type	reference_assembly	rule
`minigraph-grch38.gfa.gz`	428	428	`pangenome.reference`	GRCh38	`rgfa_stable_rank_reference`
`minigraph-chm13.gfa.gz`	634	634	`pangenome.reference`	CHM13	`rgfa_stable_rank_reference`
`mc-grch38.gfa.gz`	0	0	`pangenome.reference`	GRCh38	`pangenome_reference_mc` (filename)

Both gap files now resolve from content; MC falls back correctly; reference_assembly unchanged throughout.

Scope notes

reference_assembly is deliberately not derived from content: the head only ever exposes a single contig (SN:Z:chr1), too few for contig-length detection. It stays with the shared filename_ref_* rules, which already get both files right.

Filed while working

_decompress_if_gzipped truncates BGZF at the first block (multi-member gzip) #149 — _decompress_if_gzipped truncates BGZF at the first block (multi-member gzip). Found empirically: the MC file is BGZF (15 members in 256KB) and decompresses to 65,280 B instead of 943,411 B. Does not affect this PR (rGFA files are plain gzip; MC has no tags), but it is latent for the VCF/FASTA fetchers.
Claims are unconstructed dicts with a silently-defaulting tier — add ExtendedClassificationResult.add_claim #150 — the content classifiers (FASTA, GFA) override fields instead of appending tier-3 claims, so tier-1 provenance is dropped and conflict detection is bypassed. Deferred to keep FASTA and GFA consistent.

make test: 470 passed.

🤖 Generated with Claude Code

The `-mc-` filename token missed the two HPRC minigraph reference graphs (hprc-v1.0-minigraph-{grch38,chm13}.gfa.gz), which classified as plain `pangenome`. Per the accuracy-over-efficiency principle, determine this from file content instead of adding another filename token. In rGFA each segment carries a stable rank (`SR:i:i`) and stable name (`SN:Z:`); rank 0 is the reference backbone, so a graph with rank-0 stable segments defines a reference coordinate system. Those tags sit on the very first segments, so a 256KB range request is exact and cheap. - fetchers: parse_gfa_segment_tags + fetch_gfa_segment_tags. Locates the tag columns by tab position so the segment sequence (the bulk of an S line) is never copied, and drops the record truncated by the byte cut. - header_classifier: classify_from_gfa_segment_tags emits `rgfa_stable_rank_reference` when a rank-0 stable segment is present. - file_types: GFA_CONFIG, text GFA only — .gbz/.vg/.gbwt/.xg are binary. Verified against the real files on the public human-pangenomics bucket: both minigraph graphs now resolve to pangenome.reference from content (428 and 634 rank-0 segments), reference_assembly unchanged. The minigraph-cactus GFA has untagged segments, so it keeps falling back to the `-mc-` filename rule — its PanSN path lines sit ~11GB in, past every segment line, so content inspection is not economical there. reference_assembly is deliberately not derived from content: only one contig is visible in the head, too few for contig-length detection. Found while validating: #149 (BGZF truncated at first block). Deferred: #150 (route content refinement through the claim model). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot

Pull request overview

Adds content-based detection of pangenome.reference for rGFA (minigraph) graphs by parsing stable-rank (SR:i:0) and stable-name (SN:Z:) tags from the file head, addressing the gap where these reference graphs were previously missed by filename-only heuristics.

Changes:

Introduces a GFA head fetcher and parser to extract rGFA segment tags efficiently from a 256KB range.
Adds a GFA content classifier that refines data_type to pangenome.reference when rank-0 stable sequences are present.
Registers a new gfa file type in the pipeline and updates docs + golden fixtures/tests accordingly.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
`src/meta_disco/fetchers.py`	Adds `parse_gfa_segment_tags` and `fetch_gfa_segment_tags` to range-fetch and parse rGFA S-line tags.
`src/meta_disco/header_classifier.py`	Adds `classify_from_gfa_segment_tags` to refine `data_type` based on rGFA stable-rank tags.
`src/meta_disco/file_types.py`	Registers `GFA_CONFIG` and adds `gfa` to `FILE_TYPE_REGISTRY`.
`tests/test_evals.py`	Adds unit tests for rGFA tag parsing and content-based pangenome reference classification.
`tests/test_output_shape.py`	Extends golden fixture generator inputs and stub payloads to include `gfa`.
`tests/test_pipeline.py`	Updates registry assertions to include the new `gfa` type.
`tests/fixtures/golden/expected_output.json`	Regenerates golden output to include `gfa` pipeline results.
`docs/classification-hierarchy.md`	Documents content-based vs filename-based detection of `pangenome.reference` for graphs.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

- header_classifier: append the rGFA claim to field_evidence["data_type"] instead of assigning over it, so the tier-1 `pangenome_graph` claim survives and the derivation chain matches the engine-resolved `-mc-` case. Tag it tier 3: evaluate_claims defaults a missing tier to 0, which would lose to the tier-1 `pangenome` claim and silently undo the refinement once #150 resolves this path from claims. Verified the appended list resolves to pangenome.reference on its own. (setdefault is unnecessary — field_evidence is pre-populated for every field in CLASSIFICATION_FIELDS.) - fetchers: the `pos == -1` guard trips on fewer than 3 tabs, i.e. fewer than 4 columns, not "fewer than 3 columns". The remaining half of the clobber problem — value set via set_field rather than resolved from claims, bypassing conflict detection — is unchanged here and tracked across all 20 sites in #150. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

… docstring "the reference signal is always visible" was not literally true. rGFA tags follow each segment's sequence, so a graph whose leading segment sequences exceed the decoded head (~64KiB for BGZF, see #149) would push them out of reach. Describe the real bound, note it is not guaranteed, and state the degradation: no tags means no content claim, and the caller falls back to the filename rules. Same-class scan of this PR's prose found one more: the docs called the content check "exact" unconditionally. Scoped to "when those tags are in the head". Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.

Copilot

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

pipeline._filter_records selects records on file_format OR file_name, so file_name can be empty on a record with a real extension. The classifier fell back to "graph.gfa", which would hand the extension-scoped rules the wrong suffix. No outcome changes today: all eight graph extensions appear together in both graph rules, so .gfa and .rgfa.gz resolve identically. Fixed anyway so the fallback stays correct if a rule ever distinguishes them, and to match the BAM/VCF classifiers. file_name still wins when present, since it carries the tokens the tier-2 rules match. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.

…_size note - fetchers: _fetch_range's end_byte is inclusive, so 262144 fetched 262,145 bytes and the "256KB" comment was off by one. Pass 262143; verified against the real files that raw_bytes_fetched is now exactly 262144 and the parsed tags and data_type are unchanged. Docstring now says 256KiB. The three sibling call sites (lines 202, 273, 338) have the same off-by-one; out of this diff, reported separately. - test_output_shape: guard STUB_PAYLOADS coverage in its own test rather than letting a missing type surface as a bare KeyError inside the session fixture, and raise a named error from _make_stub_fetcher listing the stubbed types. - header_classifier: document that file_size is unused and accepted only because ClassifyPipeline calls every classifier with the same keyword arguments. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Copilot flagged "The sequence column is never copied" as a false absolute — `text.split("\n")` copies every line, sequences included. Only the tag scan avoids a second copy. Reworded, and scanned for the same class: - fetchers: "on the HPRC minigraph graphs every segment in the decoded head is rank-0 tagged" stated an empirical absolute; scoped to the two files actually fetched. Also spelled out that the filename fallback leaves an untokenized reference graph unrefined, not misclassified. - header_classifier / docs: the stated reason for not deriving reference_assembly ("too few contigs for contig-length detection") named a method that cannot run here at all — parse_gfa_segment_tags extracts no lengths. The real reasons are that no lengths are parsed, and that the visible stable name (chr1) is shared by GRCh38 and CHM13. - test_output_shape module docstring claimed "content-driven classification is not exercised". The gfa stub payload is a real rank-0 tag and is the only reason that record resolves to pangenome.reference; a content-free stub would silently record the unrefined value. Documented, with a warning not to replace it. Also drop the dead `and lines` conjunct: str.split never returns an empty list. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.

Copilot

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.

1. Unreadable counter, third time wrong. `if was_cached: ... elif content_unreadable:` let the cache branch win, and was_cached is a file existence stat taken before the fetch. A truncated evidence file made both true, so the record counted as "From cache" and vanished from the unreadable tally. A fetcher only raises after its own cache check missed, so it went to the network: the FetchError branch now returns was_cached=False. That also fixes the bogus [cached] indicator. 2. filename_for_rules trusted ANY known extension. A graph record named `hprc-graph.tar.gz` was taken verbatim, the tar rules ran, `pangenome_graph` never fired — and the rGFA content claim then forced data_type=pangenome.reference onto a record whose data_modality was not_classified. Callers now pass their config's extensions. GRAPH_TEXT_EXTENSIONS is defined once and shared with GFA_CONFIG. 3. The fetch_failed note misattributed its own cause. It landed on every unresolved dimension, including reference_assembly — which GFA content never determines (this module's own docstring says so), telling a reader a re-fetch could resolve an assembly only the filename supplies. FileTypeConfig gains `content_fields`, naming the dimensions each type's content can determine; only those are annotated. For GFA that is data_type alone. 4. Making _fetch_range raise turned a silent non-2xx into a printed per-file error in the vcf/fastq/fasta fetchers, from worker threads, garbling the progress line — on the main corpus, where an unknown number of mirror objects are missing (#156). They now catch FetchError and drop silently, as before. 5. `unreadable` was computed, printed, and discarded while `from_cache` was persisted. Now in the run metadata as `content_unreadable`, so a consumer can tell a filename-only fallback from a real content read — necessary because an -mc- graph whose filename settles every dimension leaves no evidence trace. Also: filename_for_rules no longer grafts a non-dotted file_format ("Other" on ~108k records) onto the name, which produced "graphOther". Corpus rerun (669 records): identical classification — 2 minigraph graphs to pangenome.reference, 0 regressions, 0 dropped. Summary now reports "Content unreadable: 14" and metadata carries content_unreadable=14. The note lands on data_type, never on reference_assembly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 5 comments.

… claims 2. classify_single and _process_single_record carried the same FetchError fallback — identical print, identical classify_without_content call, identical None guard. That block changed four times in this PR and had to be edited twice each time. Extracted `_fetch_and_classify`, which returns (classifications | None, content_unreadable) and names the three outcomes. Both paths now wrap one implementation in their own envelope. 3. "Dropped (fetcher gave no cause)" also counted records whose worker raised and printed a cause. Split into `dropped` (fetcher returned None) and `errored` (a worker raised); both are persisted, and `failed` is kept as their sum so existing consumers of that key are unaffected. 4. Truth-in-comments: - The classify_from_gfa_segment_tags docstring cited pipeline.py:242 for the classifier call; the line rotted within this PR when _build_record was extracted. It now names the function. Grepped: no other file:line refs. - test_phase1_job_output_names_follow_the_registry_convention compared the production f-string to an identical copy of itself and could not fail. Replaced with a check against the Makefile — an independent source — that a registered type has a classify-<type> target writing the expected file. Verified it fails for a hypothetical unregistered type. - test_non_header_jobs_are_also_read_by_the_reports was a strict subset of test_every_phase1_output_is_read_by_the_reports. Deleted. - `make lint` was red (ruff I001). Now green. - _process_single_record's docstring still described the pre-content_fields annotation rule. Corrected. Corpus rerun (669 records): identical output — 2 minigraph graphs to pangenome.reference, 0 regressions, 0 dropped, content_unreadable=14. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.

- rule_engine.rules_matched claimed the synthetic rule IDs were "not_classified, infer_assay_type, conflicting_*". Twelve more exist: the content classifiers in header_classifier add contig_length_detection, vcf_contig_length, aligned_to_reference, five fasta_*, two bed_*, and — added by this PR — rgfa_stable_rank_reference and fetch_failed. A caller must not assume an ID names a rule in unified_rules.yaml. Mostly pre-existing; this PR extended it, so it is fixed here. - rule_engine.reasons said "deduplicated list of reasons"; it deduplicates by rule_id, so two rules sharing a reason both appear. - test_content_claim_is_appended_not_clobbered pinned the exact evidence list ["pangenome_graph", "rgfa_stable_rank_reference"]. #147 will add graph data_type rules and break it for an unrelated change. It now asserts the tier-1 claim survives and precedes the content claim, which is the actual behavior under test. - The new GFA tests read record["field"]["value"] directly while every other dict-returning test in the file uses get_val/field_status. Swapped 17 value and 1 status subscript onto the helpers, which also handle the nested layout the raw form would KeyError on. Evidence subscripts left as-is — no helper covers those. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.

…ion and docstring - Remove `$TMPDIR/probe_ev/` — a literal `$TMPDIR` directory committed in c3d382e. A probe script wrote to an unexpanded "$TMPDIR" in the repo root and `git add -A` swept it in, including a corrupt `deadbeef.json` evidence fixture. .gitignore now ignores the literal path so it cannot recur; the corrupt-evidence case is already created at runtime by tests/test_pipeline.py. - parse_gfa_segment_tags dropped the final line whenever the text lacked a trailing newline, losing the last segment's tags for a small complete rGFA. It now takes `truncated`, and the fetcher computes it: fewer bytes than requested means we hold the whole file, and a gzip stream with nothing after it means we decoded all of it. Only both together make the text complete. New `_decompress_head` reports the latter via `decompressobj.eof` / `unused_data`; BGZF correctly reports incomplete, since its first member ends but more follow (#149). Not done by inspecting the text, as the review suggested: a byte-range cut can land exactly on a tag boundary, so a truncated line may be syntactically complete. A test pins that indistinguishability. - _fetch_range's docstring still said vcf/fastq/fasta swallow FetchError in their `except Exception`; they now have an explicit `except FetchError` clause, which must precede it. Live re-check: the three real HPRC graphs are unchanged (428 / 634 / 0 rank-0 segments), all truncated heads. 499 tests, lint clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 14 out of 15 changed files in this pull request and generated 2 comments.

…ecord `record.get("file_name", "")` returns None for a present-but-null key — the default only fires when the key is absent — and `_filter_records` admits such a record when its file_format matches. `update_progress` then sliced None. Sequentially that aborted the run. In the parallel path it was worse: the raise landed in the executor's `except Exception`, so a record that had already classified successfully was counted as `errored` and never written by `writer.write`. A silent loss, not a crash. Both call sites now use `or ""`, matching the idiom already in `_filter_records` and `_process_single_record`, and the print itself is guarded since it is the crash site. Parametrized regression test over workers=1 and workers=2; verified it fails on the old code in both modes. Pre-existing (d9fa60d), not introduced here, but this PR touched both lines. The corpus has 0 null file_names today, so this was latent. Same-class: ~25 `.get(key, "")` sites in scripts/ share the pattern. Out of this diff and unreachable on the current corpus; not touched. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 14 out of 15 changed files in this pull request and generated no new comments.

Merged rather than rebased: rebasing would require a force push, which the repo forbids. Conflicts, both "keep both sides": - pipeline.py imports: #151's FetchError/classify_without_content plus this branch's validate_pipeline_records. - Makefile: #151's classify-gfa target plus this branch's validate-metadata. One real interaction, not a textual conflict. #151 added a test asserting the run loop tolerates a present-but-null file_name; #161 now rejects such a record at load, so the record can never reach the loop. Rather than delete either guarantee, the test is split: - test_null_file_name_is_rejected_at_load — the outer gate (_load_input raises). - test_null_file_name_in_the_run_loop_neither_crashes_nor_loses_the_record — defense in depth, driving _run_parallel directly, since the loader now stops such a record from getting there. Still pins that a raise in update_progress would silently discard an already-classified record in the parallel path. 531 tests, lint clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot AI review requested due to automatic review settings July 9, 2026 07:23

github-project-automation Bot added this to meta-disco Jul 9, 2026

Copilot started reviewing on behalf of NoopDog July 9, 2026 07:24 View session