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.
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
1 participant
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: