Repository navigation
Diff assembly versions instead of assuming a +1 increment (Phase 4, 2 of 3) - #14
Merged
rjchallis merged 7 commits intoSep 7, 2026
Conversation
Behaviour-neutral fixes to the column names Phases 0-3 read and write and to where their files land. Addresses F1, F2, F3, F4, F6 and F9 from the Phase 4 plan; no change to which assemblies are detected or what the pipeline decides. F1/F2 - Phase 1 read the non-existent `accession` column and wrote snake_case version metadata, so the Phase 0 -> Phase 1 handoff raised KeyError on the first real row. Adds get_accession / get_assembly_id / get_version_status to assembly_versions_utils and routes every Phase 1 TSV read through them; Phase 1 now emits the canonical `versionStatus` / `assemblyID`. The JSONL read is left alone, where `accession` is the correct NCBI key. F3 - append_superseded_to_tsv derived its header from one arbitrary row, so merging Phase 0 and Phase 1 row sets silently dropped columns. merge_fieldnames now takes the union across all rows, ordered by the YAML header or the order already on disk. F4 - Phases 1-3 hardcoded `assembly_current.tsv` while the production config emits `ncbi_datasets_eukaryota.tsv.gz`, and Phase 1 opened the gzipped `.previous` snapshot as plain text. resolve_current_tsv_paths derives the name from config.meta["file_name"], and open_tsv detects gzip by magic bytes (the snapshot is compressed but does not end `.gz`). F6 - Phase 0 wrote relative to the YAML's directory. resolve_output_path pins the output to --work_dir, so the copy-the-YAML workaround is gone from the READMEs. F9 - Phase 0 wrote with mode "w". Harmless for a one-time backfill, but the F5 decision makes the gap-fill path re-invoke it daily, where it would truncate the ~8,500-row file. write_or_append_parsed writes the whole file only when it does not exist (so the header is produced) and otherwise appends the rows not already present, deduplicating on genbankAccession via a cheap accession-only pass. Also adds `superseded_by`, `superseded_by_version` and `superseded_date` to assembly_historical.types.yaml so they survive the GenomeHubs write path. Tests: fixtures rebuilt on the real headers - building them inline with the wrong keys is why F1-F3 went undetected - plus a schema-conformance test against the YAML and a non-destructive-write test. 76 -> 109 passing.
parse_ncbi_assemblies has written processedAssemblyInfo.assemblyId (lowercase d) since 5180090, while assembly_historical.types.yaml declares assemblyID and Phase 0 writes assemblyID. Nothing is broken today - the two spellings never meet in one file - but this is the class of defect PR-A exists to remove, and it bites as soon as anything joins current and historical rows on that key. get_assembly_id now reads all three spellings. The write side deliberately stays on assemblyID: it is what the YAML declares, so a row emitted with the lowercase form would be dropped by the GenomeHubs write path. A test pins that asymmetry so a future edit cannot quietly flip the write side. Narrow by inspection - versionStatus and genbankAccession have no case variants upstream, so assemblyID is the only divergence. Which case is canonical is a question for Rich; repointing is a one-line change to COL_ASSEMBLY_ID plus the YAML once he answers.
Two defects in this PR's own surface, found running the phases together. Phase 2 counted an ebpStandardDate of "None" as an EBP metric while Phase 3 read the same cell as absent. Upstream writes that string wherever a value was missing when the row was formatted, so on real data the summary and the milestones would claim different things about the same assembly. Every phase now reads cells through assembly_versions_utils.cell, and a test asserts the two predicates agree on "", "None" and a real date. The gap fill appended under config.headers. That is right until the Phase 1 parser rewrites the file, which widens the header to the union of every row's columns — a superseded row copied from the current TSV brings columns the historical config never declared, and appended rows then come out a few columns short. The pass that collects existing accessions now keeps the on-disk header too, and appends against that; load_existing_accessions becomes read_existing_output to match. Phase 2.2 had no unit tests. It has them now, including the cross-phase agreement on the sentinel. The pytest CI job comes with this PR rather than the last of the three: F1-F3 survived review because nobody re-ran the suite at the right moment, which is an argument for the job gating the PR that fixes them.
Contributor
Reviewer's GuideThis PR replaces arithmetic version assumptions with a historical, per-accession diff that avoids false-positive daily gaps, detects genuine skipped versions, preserves superseded and backfilled history safely, and standardizes TSV schema/path handling with CI-backed regression tests. Sequence diagram for historical assembly version diffsequenceDiagram
participant Daily as Daily parser
participant Current as Current JSONL
participant Previous as Previous current TSV
participant Historical as Historical TSV
participant Backfill as Gap-fill backfill
Daily->>Current: Read today's accession
Daily->>Previous: load_previous_parsed_by_base()
Daily->>Historical: load_versions_by_base()
alt Version unchanged
Daily-->>Daily: Skip assembly
else Version advanced
Daily->>Daily: build_superseded_row()
Daily->>Historical: append_superseded_to_tsv()
alt Candidate gap is not historical
Daily->>Backfill: Report missing version
else Candidate gap already parsed
Daily-->>Daily: Suppress missing report
end
else Version regressed
Daily->>Daily: print_version_regressions()
end
Entity relationship diagram for historical assembly version trackingerDiagram
ASSEMBLY_SERIES ||--o{ ASSEMBLY_VERSION : contains
ASSEMBLY_SERIES {
string base_accession
}
ASSEMBLY_VERSION {
string genbankAccession
int version
string versionStatus
string superseded_by
int superseded_by_version
}
Flow diagram for assembly version gap detectionflowchart TD
A["Today's current accession"] --> B["parse_accession()"]
B --> C{Previous version exists?}
C -->|No| D["Candidate versions below current"]
C -->|Yes| E["Compare current and previous"]
E -->|Unchanged| F[Skip]
E -->|Advanced| G["build_superseded_row()"]
E -->|Regressed| H["print_version_regressions()"]
G --> I["Candidate versions between versions"]
D --> J["build_gap_records()"]
I --> J
J --> K{Version in historical TSV?}
K -->|No| L["Report missing version"]
K -->|Yes| F
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="flows/parsers/parse_backfill_historical_versions.py" line_range="259-260" />
<code_context>
+ output_path = config.meta["file_name"]
+ header, existing = read_existing_output(output_path)
+
+ if not existing:
+ write_to_tsv(parsed, config)
+ return len(parsed)
+
</code_context>
<issue_to_address>
**issue (bug_risk):** An existing historical TSV whose rows all lack `genbankAccession` is treated as empty because `read_existing_output` only adds non-empty accessions to `existing`; `write_to_tsv` then rewrites the file and discards those rows instead of preserving them.
**Triggers:** When the historical file contains one or more rows without an accession but does not have any accession-bearing rows.
**Suggested fix:** Track whether the output file contains data separately from the accession set, and preserve rows with no accession when deciding between rewrite and append.
</issue_to_address>read_existing_output only collects non-empty accessions, so a historical TSV whose rows all lack one came back with an empty set and took the write_to_tsv branch, which opens the file with mode "w" — the very rows that made the file non-empty were discarded. The header is the exact signal instead: it is empty only when the file is missing or zero-length. Keying the branch off it sends such a file down the append path, where its rows survive. That also makes the header non-empty on every append, so the config-header fallback is now dead and goes away.
A Phase 1 row is copied wholesale from the current TSV, whose schema is
upstream's rather than this pipeline's, and append_superseded_to_tsv
unioned the columns across every row. Every current-only column
therefore landed in assembly_historical.tsv despite the YAML never
declaring it — upstream's assemblyId spelling among them, sitting next to
the assemblyID that build_superseded_row sets, so the file carried two
assembly-ID columns disagreeing about the same assembly.
Three parts:
- canonicalize_columns folds each alias spelling onto the column the
YAML declares, and build_superseded_row copies through it. Reads
still tolerate every alias; a row about to be written now carries one
column per field.
- When headers are supplied, they bound the schema as well as ordering
it: a row may contribute the declared columns plus whatever is already
on disk, and nothing else. Columns already in the file are still
never dropped, which is what F3 was about.
- load_historical_headers resolves assembly_historical.types.yaml from
work_dir, then from the repo's configs/, and both entry points pass
the result. The headers parameter existed but nothing ever supplied
it, so the bound would have applied to nothing. Every failure to
resolve degrades to the order already on disk: a missing schema is not
a reason to fail a daily run.
identify_newly_superseded inferred supersession arithmetically — it looked for new_version - 1 in the previous parse and reported it missing when absent. Since the previous parse holds only the current version of each assembly, that misfired three ways: 1. Steady state. An assembly unchanged at v3 since yesterday made the code look for v2, not find it, and report v2 missing. On a quiet day with no new versions at all, missing_versions.json would have held roughly 3,694 entries — every multi-version assembly in the dataset — each of which update_assembly_versions would then re-fetch from NCBI. 2. Skipped versions. With v3 current yesterday and v5 today, the code looked for v4, reported it missing and moved on. v3 — sitting in the previous parse — was never marked superseded, and was lost when assembly_current.tsv was overwritten. 3. No historical check. The function never read assembly_historical.tsv, so versions the Phase 0 backfill had already parsed were re-reported as missing on every run. Replace the arithmetic with a per-base diff of the version current now against the version current at the last run: equal versions are skipped, a higher version supersedes the one it replaced, a lower version is logged as a regression (NCBI suppression or rollback), and a base not seen before treats everything below its current version as a gap. Gap candidates are then filtered against a version index of assembly_historical.tsv, so only genuinely unparsed versions are reported. Gaps can span several versions, so build_gap_records emits one missing-version record per version rather than widening the record — the downstream shape read by update_assembly_versions is unchanged. Both reads are local; the step still makes no network calls. Adds load_versions_by_base to assembly_versions_utils for the historical index, and 25 tests covering all three failure modes, multi-version gaps, regressions, and new bases.
append_superseded_to_tsv keyed its merge on the assembly ID alone, so two rows without one collapsed onto the empty string and all but the last were dropped when the file was rewritten. Every row Phase 0 and this parser write carries an ID, so nothing is losing data today, but the merge is the wrong place to depend on that: it reads whatever is already on disk. Falls back to the accession, then to a per-row sentinel, since two rows with neither cannot be told apart and keeping both is the safe answer.
fchen13
force-pushed
the
fix/assembly-version-supersession-diff
branch
from
September 6, 2026 14:18
66a68d3 to
3d0540f
Compare
fchen13
added a commit
to fchen13/genomehubs-data
that referenced
this pull request
Sep 10, 2026
Steps ran 1, 2, 4, 3. The production taxonomy path is now Step 3 and the staged run Step 4, which is also the order they happen in. The file table listed test_assembly_summary.py and pytest.yml without saying they merged with genomehubs#13, so two of ten rows are not in this diff. Say up front that Phase 4 shipped as three stacked PRs and mark those rows. "Unit tests for Phase 2.2, which had none" read as though the test file had no tests; it meant the summary generator shipped without any. The sentinel paragraph said Phase 2 and Phase 3 disagreed but never said how it was settled: Phase 3 was already reading through cell, Phase 2 was routed through the same helper in genomehubs#13 rather than either place special-casing the string. PR-A and PR-B merged on 2026-09-07 as genomehubs#13 and genomehubs#14, so the "do not run against real data until PR-A is merged" blocker is stale. Refer to the merged PR numbers instead of the pre-submission labels. Drop the PYTHONUTF8 note. It is a Windows dev-environment workaround, not something a reviewer needs, and it sat between list items 2 and 3 breaking the numbering. The full explanation is already in project_doc/Phase_4_plan.md; the runbook keeps a short inline comment.
fchen13
added a commit
to fchen13/genomehubs-data
that referenced
this pull request
Sep 11, 2026
validate_no_ncbi_fetches.py claimed something false. The pipeline does fetch from NCBI -- Phase 0 backfills, Phase 1.2 fills gaps, and update_ncbi_datasets pulls the bulk JSONL daily, all by design. The module's first paragraph existed to walk the title back, which is a sign the title was doing the wrong work. What it actually checks is that the daily diff stays local: parse_assembly_versions copies superseded rows from the previous parse and checks gaps against assembly_historical.tsv, so it should reach the network for no input at all. "Local" also covers the failure the old name missed. The step can reintroduce daily fetching without making a single call itself, by emitting a bogus missing-versions list that update_assembly_versions then works through -- which is exactly what the pre-genomehubs#14 diff did to ~3,694 assemblies. The unchanged-input check catches that; a name about sockets does not describe it. Renamed the module, its entry point, the import alias and the output banner. No behaviour change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Phase 4 groundwork, part 2 of 3. Stacked on #13 — that PR's three commits appear in this diff until it merges. The two commits that are new here are
PR-B: diff assembly versions instead of assuming a +1 incrementandKey the historical merge on something every row has.Where #13 was behaviour-neutral, this one changes what the daily pipeline actually does.
The defect (F5)
identify_newly_supersededassumed the current version had incremented by exactly one since the last run, so for an assembly current at v3 it reported v2 as "missing" — every day, whether or not anything changed. On the real dataset that is roughly 3,694 assemblies reported as missing a version on every run, each one an NCBI fetch that is not needed. The premise of the module is that it makes no fetches, so this defeated its purpose.It also missed the case it most needed to catch: when a version is genuinely skipped (v3 current yesterday, v6 today), v4 and v5 are real gaps and the arithmetic never looked for them.
The fix
Diff today's current version against the version current at the last run, per base accession:
A gap is only reported if no phase has already parsed it: the versions already in
assembly_historical.tsvare indexed first, so a settled assembly stops being re-reported on every subsequent run. That index is a local read; the module still opens no network connections.Per your note on the original review — you had spotted that the old code looked at any version > 1 — the gap check now goes through
assembly_historical.tsvbefore anything is handed to the fetch path.Also here
append_superseded_to_tsvkeyed its merge on the assembly ID alone, so two rows without one collapsed onto the empty string and all but the last were dropped when the file was rewritten. Every row Phase 0 and this parser write carries an ID, so nothing is losing data today, but the merge reads whatever is already on disk and is the wrong place to depend on that. It now falls back to the accession, then to a per-row sentinel — two rows with neither cannot be told apart, and keeping both is the safe answer.Testing
160 passing on this branch. The diff paths are covered case by case: unchanged single-version, unchanged multi-version (the case the arithmetic got wrong), +1, skipped versions, new series, and regression. A separate test asserts an unchanged input supersedes nothing and reports nothing missing — the assertion that actually captures "no unnecessary fetches", and the one the old code failed.
Exercised end to end against real NCBI data as well: a two-day run over a 207-assembly Malacostraca slice, with the multi-version assemblies rewound a version to make day 1, so every version the gap path goes looking for is one NCBI actually has.
Notes
The Phase 4 validation suite and the staged run follow in a third PR, once this one and #13 land.
Summary by Sourcery
Replace arithmetic version assumptions with historical diffs so daily assembly tracking reports only real supersessions and missing versions while preserving existing data.
New Features:
Bug Fixes:
Enhancements:
CI:
Documentation:
Tests: