Repository navigation
Assembly version schema and path contracts (Phase 4, 1 of 3) - #13
Merged
rjchallis merged 5 commits intoSep 7, 2026
Merged
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 establishes configuration-driven TSV path/compression contracts, canonicalizes assembly-version schema access and output, standardizes missing-value semantics, and makes historical merges safe and non-destructive, with expanded realistic-schema tests and pull-request CI. Sequence diagram for safe historical version backfillsequenceDiagram
participant Backfill
participant Config
participant HistoricalTSV
participant NCBI
Backfill->>Config: resolve_output_path(config, work_dir)
Backfill->>HistoricalTSV: read_existing_output(output_path)
Backfill->>NCBI: fetch_and_parse_sequence_report()
NCBI-->>Backfill: parsed historical rows
alt historical TSV absent
Backfill->>HistoricalTSV: write_to_tsv(parsed, config)
else historical TSV exists
Backfill->>HistoricalTSV: append_to_tsv(header, new_rows, config.meta)
end
Sequence diagram for configuration-driven summary generationsequenceDiagram
participant SummaryFlow
participant Config
participant CurrentTSV
participant HistoricalTSV
participant SummaryOutput
SummaryFlow->>Config: load_config(config_file=yaml_path)
SummaryFlow->>Config: resolve_current_tsv_paths(work_dir, config)
SummaryFlow->>CurrentTSV: open_tsv(current_tsv)
SummaryFlow->>HistoricalTSV: open_tsv(historical_tsv)
SummaryFlow->>SummaryFlow: get_accession(row)
SummaryFlow->>SummaryFlow: get_version_status(row)
SummaryFlow->>SummaryFlow: cell(row, metric_columns)
SummaryFlow->>SummaryOutput: write summary TSV
Flow diagram for canonical assembly TSV schema handlingflowchart TD
ROW[TSV row]
CELL["cell(row, keys)"]
ACCESSION["get_accession(row)"]
ASSEMBLY["get_assembly_id(row)"]
STATUS["get_version_status(row)"]
CANONICAL[Canonical Phase 1 output columns]
UNION["merge_fieldnames(rows, preferred_order)"]
ROW --> CELL
CELL --> ACCESSION
CELL --> ASSEMBLY
CELL --> STATUS
ACCESSION --> CANONICAL
ASSEMBLY --> CANONICAL
STATUS --> CANONICAL
ROW --> UNION
UNION --> CANONICAL
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 3 issues
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" />
<code_context>
+ int: Number of rows actually written.
+ """
+ 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 containing rows without a recognized accession is treated as empty because `existing` remains empty, so the function calls `write_to_tsv` in overwrite mode and truncates those rows instead of preserving the existing file.
**Triggers:** When an existing historical file contains malformed, legacy, or otherwise accession-less rows.
**Suggested fix:** Use file existence (and the presence of a header) to choose append versus initial write; do not use the populated-accession set as the overwrite decision.
```suggestion
if not os.path.exists(output_path) or not header:
```
</issue_to_address>
### Comment 2
<location path="flows/parsers/parse_assembly_versions.py" line_range="282-286" />
<code_context>
+ with open_tsv(historical_tsv) as f:
+ reader = csv.DictReader(f, delimiter=DELIMITER)
+ for row in reader:
+ existing[get_assembly_id(row)] = dict(row)
+ file_order = list(reader.fieldnames or [])
for row in newly_superseded:
- existing[row["assembly_id"]] = row
+ existing[get_assembly_id(row)] = row
- fieldnames = list(next(iter(existing.values())).keys())
</code_context>
<issue_to_address>
**issue (bug_risk):** Rows lacking all three assembly-ID spellings are stored under the same empty-string key, so reading an existing historical file collapses every such row and a subsequent merge overwrites all but the last one.
**Triggers:** When a historical TSV contains multiple rows without `assemblyID`, `assemblyId`, or `assembly_id`.
**Suggested fix:** Skip or separately handle rows with no assembly ID rather than using an empty string as the deduplication key.
</issue_to_address>
### Comment 3
<location path="flows/parsers/parse_assembly_versions.py" line_range="127-129" />
<code_context>
"""
- base_acc, _ = parse_accession(previous_row["accession"])
+ base_acc, _ = parse_accession(get_accession(previous_row))
row = previous_row.copy()
- row["version_status"] = "superseded"
- row["assembly_id"] = f"{base_acc}_{previous_version}"
- row["superseded_by"] = new_accession
- row["superseded_by_version"] = new_version
- row["superseded_date"] = release_date
+ row[COL_VERSION_STATUS] = "superseded"
+ row[COL_ASSEMBLY_ID] = f"{base_acc}_{previous_version}"
+ row[COL_SUPERSEDED_BY] = new_accession
+ row[COL_SUPERSEDED_BY_VERSION] = new_version
</code_context>
<issue_to_address>
**issue (bug_risk):** `build_superseded_row` copies every column from the current TSV, including upstream's `assemblyId` spelling, and `append_superseded_to_tsv` unions those keys when no historical header is supplied; the resulting historical TSV therefore gains undeclared columns despite the YAML declaring only canonical `assemblyID`.
**Triggers:** When the current TSV contains the upstream `assemblyId` column and the incremental parser is invoked without `historical_headers`, as the wrapper does.
**Suggested fix:** Filter copied rows to the historical schema or pass the loaded historical YAML headers to `append_superseded_to_tsv`, while retaining only explicitly supported aliases for reads.
</issue_to_address>
Comment on lines
+282
to
+286
| existing[get_assembly_id(row)] = dict(row) | ||
| file_order = list(reader.fieldnames or []) | ||
|
|
||
| for row in newly_superseded: | ||
| existing[row["assembly_id"]] = row | ||
| existing[get_assembly_id(row)] = row |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Rows lacking all three assembly-ID spellings are stored under the same empty-string key, so reading an existing historical file collapses every such row and a subsequent merge overwrites all but the last one.
Triggers: When a historical TSV contains multiple rows without assemblyID, assemblyId, or assembly_id.
Suggested fix: Skip or separately handle rows with no assembly ID rather than using an empty string as the deduplication key.
Contributor
Author
|
Note on the red checks, since they look alarming and are not new — #12 failed the same two. flake8 Lint was already failing on Worth deciding separately whether to drop sourcery fails the same way it did on #12; the pytest is the new job this PR adds, and it passes. |
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.
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.
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
2 participants
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 1 of 3. A code review ahead of the Phase 4 full run found that Phase 1 was written and tested against a mock TSV schema that does not match what Phases 0 and 2 actually produce. Three of the defects raise
KeyErroron the first real production row, before any validation logic runs.This PR is deliberately behaviour-neutral: no change to which assemblies are detected or what the pipeline decides, only to the column names it reads and writes and where files land.
What it fixes
accessioncolumn that does not exist — real assembly TSVs usegenbankAccessionget_accession, reading eitherversion_statusvsversionStatus,assembly_idvsassemblyID)extrasaction="ignore", silently dropping any column that row lacksassembly_current.tsv; the name and the gzip mode were hardcodedconfig.meta["file_name"]via theworking_yamlthe wrappers already receive--work_dirwork_dir, mirroringrun_generic_tsv_parserwrite_to_tsvonly when the file is absent (that branch writes the header),append_to_tsvotherwise, with a dedup passTwo more turned up later, running the phases together rather than one at a time:
"None"was a value to Phase 2 and absent to Phase 3. The GenomeHubs write path stringifies a missing value, so on real dataassembly_version_summary.tsvwould claim an EBP metric thattaxon_milestone_summary.tsvdenies. Every phase now reads cells through one helper,assembly_versions_utils.cell.config.headers. That is correct 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 short. The pass that collects existing accessions now keeps the on-disk header too.Also here
configs/assembly_historical.types.yamldeclares the three supersession columns, so Phase 1's values survive the write path instead of being dropped.get_assembly_idreadsassemblyID,assemblyIdandassembly_id. The write side stays onassemblyID, which is what the YAML declares — a row emitted with the lowercase form would be dropped by the GenomeHubs write path.parse_ncbi_assemblieshas writtenassemblyIdsince5180090; nothing indata/reads it, so whatever consumes it lives elsewhere. Happy to repoint both toassemblyIdin a follow-up if that is the canonical spelling..github/workflows/pytest.ymlmakes that mechanical, on every PR. Plainpip install genomehubs pytest— the suite needs no network and nodatasetsCLI.tests/test_assembly_summary.py), including the cross-phase agreement on the sentinel.Testing
133 passing on this branch. The schema-conformance tests assert the columns Phase 1 writes are the ones the types YAML declares, and the non-destructive-write tests assert an existing historical TSV survives a gap fill — the two classes of defect that got us here.
Also exercised end to end against real NCBI data (a 207-assembly Malacostraca slice, 18 of them multi-version): the backfill fetched 19 superseded versions and the outputs reconcile.
Notes
Two follow-ups are stacked behind this one and will come as separate PRs: the daily diff logic (F5), and the Phase 4 validation suite and staged run.
Summary by Sourcery
Align assembly version phases with production schema and file-path contracts while preserving historical data and validating cross-phase behavior.
Bug Fixes:
Enhancements:
CI:
Documentation:
Tests: