Repository navigation
Phase 4: validation, the production taxonomy path, and the staged run (3 of 3) - #15
Merged
rjchallis merged 17 commits intoOct 7, 2026
Merged
Conversation
validate_pipeline.py checks the four output TSVs against each other
rather than against a mock: assembly IDs unique across current and
historical, summary version_gaps reconciling with the versions actually
present, every superseded_by resolving to a known accession of the same
base, every base accession summarised and no summary row stale, the
nested milestone dates never inverting, and every first_*_in_ranks value
a canonical rank.
Two readings of the plan are deliberately looser than its wording. A
superseded_by referent is looked up across current and historical
together, since a v1 superseded by v2 stays correct once v2 has itself
been superseded. Milestone ordering is checked pair by pair rather than
by whole triples, so an inversion is caught when the third date is
absent.
A seventh check asserts the upstream {rank}TaxId columns arrived
populated. print_to_tsv writes only the columns the types YAML declares,
so the enrichment can be a silent no-op upstream and Phase 3 then
produces empty milestones. It is a warning by default, because a dev run
off a taxdump legitimately has no lineage columns; --strict promotes it,
which is what production runs should use.
validate_no_ncbi_fetches.py runs parse_assembly_versions with sockets
and subprocess.Popen blocked, over a fixture that has real supersessions
and a real gap so a run that proves nothing cannot pass. Its second
check is the one that captures "minimise fetches": an unchanged input
must supersede nothing, report nothing missing, and leave the historical
TSV byte-identical. Before the PR-B diff fix that failed on roughly
3,694 spurious entries.
F1-F3 survived review because nobody re-ran the suite against a
realistic schema at the right moment, so pytest.yml now runs the suite
on every pull request. pip rather than conda: the tests need no network
and no datasets CLI.
Fixtures are built inline, since tests/test_data is excluded from the
repo and anything placed there would not be committed.
Splitting lineage-coverage off from lineage-columns, and adding a third severity for it. An error always fails, a warning fails only under --strict, and a note never fails. The structural problem is worth failing a production run for: the columns absent entirely, or present and empty on every row, means the upstream enrichment was a no-op and Phase 3 has no taxonomy source. Some rows sitting on a taxid the upstream lookup does not cover is a different thing — a number to watch, not a defect, and not a reason for one uncovered row to fail a 57,000-row run.
register_row_taxa took a taxid's rank from whichever row reached it first. An assembly submitted at genus level carries a taxid another row names as its genus, so the same input could label that taxon a species or a genus depending on the order rows happened to appear in the file. Ancestors are now registered before row taxids. The outcome no longer depends on order, and it matches what the taxdump path already does with such an assembly: registered as a genus it has no species ancestor, so the sweep skips it and says so. The per-row skip print became a five-line summary with a count, since a full run would otherwise emit thousands of lines. LEVELS derives from LINEAGE_RANKS rather than repeating the rank list a third time, the two milestone predicates read through the shared cell helper, and a dead import went.
Every other test in the suite exercises one module. test_two_day_simulation runs all four in sequence over one working directory: a backfilled day 1, then a snapshot, a JSONL covering the unchanged, +1, skipped-version and new-series diff paths, today's current TSV, the summary, the milestones and both validators. It is the synthetic half of step 3 stage 2 of the staged run, so what the real staged run adds is real data rather than new coverage of the composition. Docs follow the code. The Phase 3 README described a production path that was a raise when it was written; it now points at assembly_lineage and says what each taxonomy source can and cannot supply. The Phase 2 README gains the "None" sentinel rule. The Phase 4 README picks up the rank-registration order, the shared cell helper, the new test files, and a table that said three ways to run while listing four.
staged_run.py drives the two stages that do not need an overnight window: fetch a clade, parse it, backfill its historical versions, aggregate and validate; then run the daily path twice over it. Stage 2 needs a yesterday that differs from today. Fabricating tomorrow would leave the gap fill asking NCBI for accessions that do not exist, so it fabricates yesterday instead, rewinding the multi-version assemblies by one version. Everything the pipeline then goes looking for is a version NCBI actually has. Run against Malacostraca: 207 assemblies, 18 above v1, one at v4. Both stages complete and both validators pass. Two defects in validate_pipeline turned up that no fixture had provoked: referential-integrity flagged all 19 backfilled rows, because their superseded_by reads "None" — PR-A declares those columns in the historical YAML, Phase 0 has no values for them, and the GenomeHubs write path stringifies the absence. The check was reading the column raw rather than through cell, making it the last place in the codebase not applying our own sentinel rule. assembly-id-uniqueness failed with "186 rows carry no assembly ID". True, and not a defect: the types YAML in test/ excludes the assembly_id identifier, so the current TSV has no such column. Which identifiers reach the TSV is a config decision, not a pipeline error. Uniqueness keeps the duplicates and the mixed case; a source with no ID column at all is now a note. The rewound count and the supersession count are not expected to match, and the script no longer pretends otherwise: RefSeq bases fold into their GenBank row, and a rewind can leave the parser holding two versions of one base, which the diff rightly calls unchanged. It now fails only if nothing was superseded, which would mean the path never fired.
The encoding failure is a developer-machine issue: production is Linux with a UTF-8 locale, and Python coerces the C locale to UTF-8 anyway, so the flow cannot hit it there. Not worth a patch to Rich's fetcher.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
Reviewer's GuideThis PR completes Phase 4 by adding real-working-directory validators and offline fetch checks, wiring Phase 3 to upstream taxonomy columns with taxdump fallback and species-taxid handling, and providing staged-run tooling plus end-to-end CI coverage for the composed pipeline. Sequence diagram for production taxonomy milestone computationsequenceDiagram
participant TSV as Assembly TSVs
participant CM as compute_taxon_milestones
participant AL as assembly_lineage
participant TAX as Optional taxdump
participant OUT as Milestone summary TSV
CM->>TSV: load_assemblies()
CM->>TAX: build_taxonomy(taxdump_path)
CM->>AL: register_row_taxa(taxonomy, rows)
CM->>AL: row_species_taxid(row)
alt speciesTaxId is populated
AL-->>CM: species taxid
else speciesTaxId is absent
AL->>AL: get_row_taxid(row)
AL-->>CM: row taxid fallback
end
CM->>AL: row_lineage(row)
AL-->>CM: upstream lineage or empty
CM->>CM: compute_milestones(rows, taxonomy)
CM->>OUT: write milestone summary
Flow diagram for staged validationflowchart TD
Stage1[Stage 1: generate current and historical TSVs]
Stage2[Stage 2: rewind multi-version assemblies]
Pipeline[Composed pipeline run]
Validate[validate_pipeline.py]
Offline[validate_no_ncbi_fetches.py]
Pass[Validated staged result]
Stage1 --> Stage2
Stage2 --> Pipeline
Pipeline --> Validate
Pipeline --> Offline
Validate --> Pass
Offline --> Pass
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Rich added species to the assembly parser's canonical ranks in 630d327 (2026-09-07), so every enriched row now names its species alongside its genus and above. Phase 3 was written before that column existed and resolved species by walking a taxdump parent chain, which is the last thing tying the production path to a taxdump it is not guaranteed to have. The sweep now takes speciesTaxId when the row carries one, and register_row_taxa registers that taxon rather than assuming the row's own taxid is a species. A subspecies-level assembly is therefore attributed to its species with no taxonomy source but the row itself, and two subspecies of one species stop appearing as two pseudo-species. The walk stays as the fallback, and the fallback covers the column-present-but-empty case rather than reading it as "no species". Upstream builds the lineage by walking rec["lineage"], and whether that array includes the taxon itself is not visible from this repo: if it does not, every species-level row carries an empty speciesTaxId beside a populated genus, and dropping those would discard most of the dataset. Falling back is correct under either shape and never does worse than the behaviour before the column existed. Species is kept out of LINEAGE_RANKS: it is the level rows are grouped at, not an ancestor the sweep walks up to, and conflating the two would have it credited twice per row.
fchen13
force-pushed
the
feature/assembly-version-phase4
branch
from
September 9, 2026 19:12
1b3eaeb to
5dbb19e
Compare
blobtk includes a taxon in its own lineage array. Node::to_json in rust/src/parse/nodes.rs pushes the node at node_depth 0 before appending its ancestors, and write_taxdump writes every nodes.jsonl line through it. Nodes::lineage() alone returns ancestors only, which is what made the shape ambiguous from outside the repo. So a species-level row carries its own taxid in speciesTaxId, and Rich's column reaches the species-level majority rather than only the subspecies minority. Nothing to raise: drop the spot-check from the unsent §8 draft and record why. No behaviour change. The empty-column fallback stays, now justified by the two cases that remain empty -- an older TSV, or a row above species rank -- instead of by an unknown.
Step 4 carried a "Still open with Rich" list and three "Resolved..." blocks recording how each question got answered and when. That is the history of working it out, not what someone reviewing the PR needs. The one remaining open item -- whether the literal "None" is deliberate -- is handled either way and already documented in the traps above it, so it is not a question Rich has to answer to approve. Rewrite the block as three claims about the code: why the taxdump is no longer needed in production, why scientific names do not change that, and that the lineage columns are declared in goat-data so the lineage-columns check passes on a real run. Also stop implying names are wanted. No phase consumes a scientific name; the milestone output declares the column but nothing reads the file back, since it is a GoaT import that resolves taxids. The taxdump is a dev and test convenience.
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
marked this pull request as draft
September 11, 2026 14:18
fchen13
marked this pull request as ready for review
September 11, 2026 14:24
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.
Contributor
|
thanks @fchen13! This is looking good - I've managed to get to the taxon summary table on the full assembly list, but just hit a couple of minor bugs on the way: I had to and It seems that update/backfill historical versions are never quite happy with GCF accessions so despite running the one time and daily backfills, the tests at the end still report the GCFs as missing, and the fetch runs multiple times for each GCF version: Aware that the GCF issue is largely down to the GCA being treated as first class in the assembly TSV but hopefully there is a workaround to this. Will work on wiring up the prefect triggers and getting it ready to import so the new fields can be in GoaT soon after these bugs are clear |
datasets emits a paired assembly twice, as its GCA record and its GCF record. parse_ncbi_assemblies files both under genbankAccession, but the daily diff and the Phase 0 scan read each record's own accession. So a GCF record found no history for its base, reported every RefSeq version below it missing on every run, and queued it for a gap-fill that could never satisfy it: the backfill discovered RefSeq versions and wrote each under the GCA it pairs with. The numbering is independent -- GCF_000004015.1 and .2 both pair with GCA_000004015.1 -- so those rows never answer the gap, and can overwrite the row the GCA record's own backfill wrote for the same version. Both now read the JSONL through load_current_versions, which applies the parser's rule: a GCF record with a GCA pair is read as that pair, an unpaired GCF keeps its own accession. One entry per base, the highest version kept. The JSONL sometimes lists a base's previous version beside its current one, which the diff reported as a regression. superseded_date was read from a top-level releaseDate. The datasets report nests it under assemblyInfo, so it was empty on every real row; the fixtures had it at the top level. A GCF record read alone contributes no date, since its own is RefSeq's. update_assembly_versions fetched the same current accession once per missing version and wrote a JSONL line for each, so Phase 0 rediscovered the base that many times. It now fetches each accession once. On the 2026-07 eukaryote JSONL the old diff reports 647 missing versions, all on GCF bases, and Phase 0 queues 432 GCF bases. Both are now 0.
Phase 0 and Phase 1 both mint the historical assembly ID as
{base}_{version} from the accession they record. A row where the two
disagree was parsed from one assembly and filed under another, which is
what the backfill did with a paired GCF record: RefSeq versions written
under the GCA they pair with. Nothing caught it -- the IDs were unique
and the accessions resolved -- so a work_dir backfilled before the
previous commit passed every check.
historical-id-agreement is an error. Current rows are not checked; the
current parser writes assemblyId in another form.
The README said nine checks; there were ten, now eleven.
Contributor
Author
|
@rjchallis, thanks for the full run. cause: 'datasets summary genome taxon' lists a paired assembly twice, once as its GCA record and once as its GCF record. every TSV we write is keyed on genbankAccession, but the daily diff and phase 0 scan read each record's own accession. so for each GCF record
fix: a record is now read under the GenBank accession it's filled under, using the same rule as parse_ncbi_assemblies. The new function is load_current_versions in assembly_versions_utils.py. Both the daily diff and phase 0 scan use it. A GCF record with no GCA pair keeps its own accession, which matches what the parser writes for it. The updater now fetches each accession once. test run: on my local 57k-record eukaryote JSONL, the old code reports 647 missing versions, all on GCF bases, and queues 432 GCF bases for backfill. with the fix, both are 0. Two more bugs found along the way False "regression": in 46 cases the JSONL lists a base's previous version next to its current one. The old diff reported those as regressions. it now keeps the highest version. Your historical TSV will need rebuilding. the backfill will have written RefSeq-derived rows under GCA accessions, and in some cases replaced the real GCA row. there is a new check 'historical-id-agreement' could check this. python -m tests.validate_pipeline --work_dir <dir> # see what it flags
mv <dir>/assembly_historical.tsv <dir>/checkpoints /somewhere/aside/
# re-run the one-time backfill, then the daily step, summary and milestonesgzip: I can' reproduce this one. 'generate_assembly_summary' reads a gzipped current TSV fine in a clean directory. Could you send the error it gave, and an 'ls' of the work dir? Passing '--yaml_path' with the ncbi_datasets_eukaryota types YAML skips that step entirely, if you want a workaround in the meantime. |
Contributor
|
ok, thanks for the fixes, will merge this one now - can fix the gzip bug separately if it resurfaces |
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, the last of three — the deliverable the previous two were groundwork for. #13 and #14 fixed what the pipeline writes; this validates it, wires Phase 3 onto the taxonomy columns you emit, and scripts the staged run.
What's included
Two validators, runnable modules rather than pytest files, since they check a real working directory after a real run:
tests/validate_pipeline.py— nine cross-file checks over the four output TSVs: assembly IDs unique across current + historical, summaryversion_gapsreconciling with the versions actually present, everysuperseded_byresolving to a known accession of the same base, every base summarised and no summary row stale, the nested milestone dates never inverting, everyfirst_*_in_ranksa canonical rank, and three on whether the upstream enrichment reached the TSV. Checks carry a severity: an error always fails, a warning fails only under--strict, a note never does. That distinction is load-bearing — a dev run off a local taxdump legitimately has no lineage columns, while a production run should treat their absence as fatal.tests/validate_no_ncbi_fetches.py— runsparse_assembly_versionswithsocket.socket,create_connection,getaddrinfoandsubprocess.Popenall raising, over a fixture that has real supersessions and a real gap so a run that proves nothing cannot pass. Its second check asserts an unchanged input supersedes nothing and reports nothing missing, which is what "no unnecessary fetches" means day to day.Phase 3 on your taxonomy columns (
flows/lib/assembly_lineage.py). Phase 3 previously rebuilt every lineage from a taxdump and its production path was araise. It now readsgenusTaxId…kingdomTaxId, and — since630d327last week —speciesTaxId, so it runs off the columns alone, off a taxdump alone, or off both with the columns winning and the taxdump supplying the names they cannot carry.Two things about that contract are handled in one place,
RANK_COLUMN_TEMPLATEbeing the single point of repoint if the names change:"None"is not absent-looking.f14ea28writes the four-character string into a taxid column for a rank present with a null taxid;""comes from the other branch. Read as a taxid,"None"would collapse every affected lineage into one bogus taxon.speciesTaxIdfalls back to the row's own taxid rather than being read as "no species". Whether the lineage you walk includes the taxon itself is not visible from this repo — if it does not, every species-level row carries an emptyspeciesTaxIdbeside a populated genus, and dropping those would discard most of the dataset. The fallback is correct under either shape. Worth a word from you if you know which it is.The staged run (
tests/staged_run.py). Stages 1 and 2 are scripted and were run against Malacostraca (6681) — 207 assemblies, 18 above version 1, one at v4. Both stages complete, both validators pass. Stage 2 needs a yesterday that differs from today, so rather than fabricate tomorrow — which would leave the gap fill chasing accessions that do not exist — it rewinds the multi-version assemblies to make day 1, so every version the pipeline goes looking for is one NCBI actually has. Stages 3 and 4, the full backfill and full run, are the operational half and are not in this PR.Tests. 264 passing, up from 160 on #14.
test_two_day_simulation.pyruns all four phases in sequence over one working directory — the only test that exercises how the phases compose rather than one module at a time.Also here
Ten committed
.pycfiles and a stalePhase_0_PR_SUMMARY.mdare deleted. Unrelated to Phase 4, but they are build artefacts that should not be tracked, and they were in the way.Testing
tests/README_phase_4_validation.mddocuments each check, the three severities, the four ways Phase 3 can now run, and the staged-run runbook.Notes
Rebased onto
mainafter #13 and #14 merged; the two commits you reviewed there are gone from this diff. One question worth your eye is thespeciesTaxIdfallback above — everything else here is ours to get wrong.Summary by Sourcery
Validate the assembled pipeline outputs, connect milestone computation to production lineage columns, and script staged end-to-end verification.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests:
Chores: