Validate AnVIL metadata records at load (#161)#162
Closed
NoopDog wants to merge 2 commits into
Closed
Conversation
Nothing between the AnVIL API and the classifiers checked the shape of a record.
_load_input validated only the envelope. A renamed key or a newly-nullable column
flowed into 758,658 records and surfaced as a crash deep in a multi-hour run — or
as a silently wrong classification.
Two contracts, deliberately separate:
- ANVIL_RECORD_SCHEMA: the full 12-field record the download produces. Checked by
`make validate-metadata` before a run.
- validate_pipeline_records: the far smaller promise ClassifyPipeline relies on.
A derived input (a filtered subset, a test fixture) carries only a few fields,
so imposing the AnVIL contract there would be wrong.
Two constraints are measured, not assumed, and a naive validator gets both wrong:
* file_size may be 0 — three corpus records are. min_value=0, not 1.
* data_modality and reference_assembly are AnVIL's own declarations and are null
for ~99% of files. Nullable by design.
No coercion and no silent drops. A None is never turned into "": that hides the
drift this exists to catch, and is exactly what makes `rec.get(k, "")` dangerous —
the default fires only when the key is absent, so a present-but-null value returns
None and crashes the first `.lower()` downstream. A dropped record is
indistinguishable from a file that was never seen (#155), so _load_input raises.
Unknown fields are not a problem: the API may add a column. A renamed one shows up
as `missing:<old_name>`.
Verified against the real corpus: 758,658 records, no problems, 4.0s. Injecting a
renamed key, a null, a stringified size and an uppercase md5 produces all four
problem kinds with bounded samples. validate_pipeline_records adds 0.48s per run.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Adds explicit schema validation for AnVIL metadata records so record-shape drift (missing/renamed keys, nulls in required fields, wrong types, etc.) fails early with a summarized report, rather than causing late crashes or silent misclassification during long pipeline runs.
Changes:
- Introduces
metadata_schema.pywith a full AnVIL record schema (ANVIL_RECORD_SCHEMA) plus a smaller pipeline contract validator (validate_pipeline_records) and a structuredValidationReport. - Wires pipeline input loading to validate records on every run and raise with a bounded, human-readable problem summary.
- Adds a
scripts/validate_metadata.pyCLI and amake validate-metadatatarget, plus a new test suite covering key constraints and edge cases.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/meta_disco/metadata_schema.py |
Adds schema definitions and validators for AnVIL records and the pipeline contract, plus reporting utilities. |
src/meta_disco/pipeline.py |
Validates loaded input records using the pipeline contract validator and raises on invalid shapes. |
scripts/validate_metadata.py |
Adds a standalone validator script for downloaded metadata files (JSON/NDJSON). |
tests/test_metadata_schema.py |
Adds focused unit tests for schema validation behavior and edge cases. |
tests/test_pipeline.py |
Minor import-order adjustment (no functional change). |
Makefile |
Adds validate-metadata target and help text. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+169
to
+178
| data = json.load(f) | ||
| if not isinstance(data, dict): | ||
| raise TypeError( | ||
| f"Expected JSON object with 'results' key, got {type(data).__name__}" | ||
| ) | ||
| records = data.get("results") or data.get("files") | ||
| if records is None: | ||
| raise ValueError( | ||
| "JSON object must contain a 'results' or 'files' key" | ||
| ) |
Comment on lines
+37
to
+42
| if not isinstance(data, dict): | ||
| raise TypeError(f"Expected a JSON object, got {type(data).__name__}") | ||
| records = data.get("files") or data.get("results") | ||
| if records is None: | ||
| raise ValueError("JSON object must contain a 'files' or 'results' key") | ||
| return records |
Comment on lines
+10
to
+12
| * :data:`ANVIL_RECORD_SCHEMA` — the full 12-field record the download script | ||
| produces. Checked by ``scripts/validate_metadata.py`` and at download time. | ||
| * :func:`validate_pipeline_records` — the far smaller promise ``ClassifyPipeline`` |
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>
Comment on lines
+219
to
+228
| data = json.load(f) | ||
| if not isinstance(data, dict): | ||
| raise TypeError( | ||
| f"Expected JSON object with 'results' key, got {type(data).__name__}" | ||
| ) | ||
| records = data.get("results") or data.get("files") | ||
| if records is None: | ||
| raise ValueError( | ||
| "JSON object must contain a 'results' or 'files' key" | ||
| ) |
Comment on lines
+35
to
+42
| with open(path) as f: | ||
| data = json.load(f) | ||
| if not isinstance(data, dict): | ||
| raise TypeError(f"Expected a JSON object, got {type(data).__name__}") | ||
| records = data.get("files") or data.get("results") | ||
| if records is None: | ||
| raise ValueError("JSON object must contain a 'files' or 'results' key") | ||
| return records |
Comment on lines
+10
to
+14
| * :data:`ANVIL_RECORD_SCHEMA` — the full 12-field record the download script | ||
| produces. Checked by ``scripts/validate_metadata.py`` and at download time. | ||
| * :func:`validate_pipeline_records` — the far smaller promise ``ClassifyPipeline`` | ||
| relies on. Derived inputs (a filtered subset, a hand-built test fixture) carry | ||
| only a few fields, so requiring the full AnVIL record here would be wrong. |
Collaborator
Author
|
Closing in favor of the LinkML → gen-pydantic → validate-with-pydantic approach. #162's hand-rolled Rather than merge an interim and immediately replace it, dropping this. The requirement (validate AnVIL metadata at load, don't coerce, don't silently drop) is still tracked in #161; the implementation will follow the generated-pydantic path. |
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.
Closes #161.
Based on
main, independent of #151.Problem
Nothing between the AnVIL API and the classifiers checks the shape of a record.
_load_inputvalidated only the envelope — that the JSON is an object with aresultsorfileskey. Every record was then trusted.data/anvil/anvil_files_metadata.jsonis downloaded bymake download. A renamed key, a newly-nullable column, or afile_sizereturned as a string flows silently into 758,658 records and surfaces as a crash somewhere deep in a five-hour run — or worse, a wrong classification.The 26
f.get("file_name", "")sites acrossscripts/are symptoms, not the disease. That default fires only when the key is absent; a present-but-null value returnsNone, and six of those sites immediately call.lower()or.endswith().Two contracts, deliberately separate
ANVIL_RECORD_SCHEMAmake validate-metadatavalidate_pipeline_recordsClassifyPipelinereads_load_input, every runThey are not one schema because a derived input — a filtered subset, a hand-built fixture — legitimately carries only a few fields. Requiring
drs_uriinside the pipeline would be wrong.Two constraints a naive validator gets wrong
Measured against the corpus, not assumed:
file_size >= 0, not> 0. Three records have size 0.data_modalityandreference_assemblyare nullable by design — they are AnVIL's own declarations, null for 751,903 and 753,962 records respectively. Rejecting nulls there would reject 99% of the corpus.Also:
boolsubclassesint, soisinstance(True, int)isTrue— afile_sizeofTrueis caught explicitly.Deliberate non-features
Noneis never turned into"". That would hide the drift this exists to catch._load_inputraises, naming the problem kinds and counts.missing:<old_name>.Verification
Against the real corpus:
Injecting four kinds of drift into a 5,000-record slice:
Samples are bounded at 3 per kind, so a systematically broken download reports a summary rather than 758k lines.
validate_pipeline_recordsadds 0.48s to a run that takes hours.Changes
src/meta_disco/metadata_schema.py— field specs,validate_anvil_records,validate_pipeline_records,ValidationReport.src/meta_disco/pipeline.py—_load_inputvalidates and raises.scripts/validate_metadata.py+make validate-metadata.tests/test_metadata_schema.py— 28 tests, including the zero-size and nullable-declaration cases,bool-as-int, a renamed key, an added key, and bounded samples.485 tests pass;
make lintclean.Follow-up
download_anvil_metadata.pycould validate before writing, so a bad download never lands on disk. Left out to keep this reviewable — noted in #161.🤖 Generated with Claude Code