From d2f570fdd1b17e35d1ba4e07115a168340dd3727 Mon Sep 17 00:00:00 2001 From: Conrad Date: Thu, 13 Aug 2026 14:02:14 -0400 Subject: [PATCH 01/15] feat: Give bedpe and bigInteract their own file_format terms Both formats pair two loci per record, and both were aliased onto the EDAM term for the single-interval format they resemble -- bedpe to BED, bigInteract to bigBed. Processor lookup keys on the format name, so that alias handed them to the BED tabix pipeline, which sorted and indexed the first locus of each record and left the second unindexed. The result was a cached artifact that looked successful and was wrong, and the byte-sniff guard cannot catch it because both are plaintext or gzip. EDAM has no term for either, verified against OLS4, so the tokens are minted under a cfdb: prefix rather than borrowed. The distinct name is what does the work: neither appears in any processor's supported_formats, so a data request now streams the raw upstream file and an index request fails cleanly, instead of serving something silently incorrect. This reaches the .bedpe already published under type=Experiment, not only the annotation files that prompted it -- and no further. The format table has exactly one consumer, the ENCODE transform. 4DN and HuBMAP take file_format from their upstream C2M2 datapackage or portal API and never consult it, so a 4DN .bedpe still arrives declaring BED and is still claimed by the tabix pipeline. Routing every DCC through the same table, or refusing the filename at the processor regardless of declared format, is follow-up work. The two formats do not lose the same thing. A bedpe index was wrong: the second mate of every record was unindexed and invisible. A bigInteract index was range-coherent, since the leading three columns are a genuine interval, so it answered range queries correctly while degrading each interaction to a single locus. An index request now returns 404 for either, which means bigInteract goes from degraded-but-usable to unavailable, for files already in the experiment corpus as well as new ones. A 404 states the truth where the degraded index quietly answered a question the caller did not ask. Tileset endpoints that understand paired intervals are planned separately. The published contract is updated to match. FileFormat and the README type table both described the id as an EDAM term, which is what a consumer would resolve it as and now fail on; both name the minted prefix as the discriminator to test against instead. --- README.md | 4 +++- src/cfdb/models.py | 28 +++++++++++++++++----- src/cfdb/services/ontology_mappings.py | 33 ++++++++++++++++++++++++-- 3 files changed, 56 insertions(+), 9 deletions(-) diff --git a/README.md b/README.md index 06445b8..1452b2c 100644 --- a/README.md +++ b/README.md @@ -560,11 +560,13 @@ Anatomy, FileFormat, DataType, and AssayType share an identical schema: `id` (st | Entity | Ontology Source | |--------|----------------| | Anatomy | UBERON (Uber-anatomy ontology) | -| FileFormat | EDAM CV `format:` terms | +| FileFormat | EDAM CV `format:` terms, or a `cfdb:`-prefixed token minted where EDAM has none | | DataType | EDAM CV `data:` terms | | AssayType | OBI (Ontology for Biomedical Investigations) | | NcbiTaxonomy | NCBI Taxonomy Database | +Every `id` above resolves in its source ontology except the minted `FileFormat` tokens. EDAM has no term for a few formats cfdb ingests — `bedpe` and `bigInteract` at present — and aliasing them onto the nearest EDAM term would make each indistinguishable from the format it was aliased to, so a `cfdb:` token is minted instead. A client resolving `file_format.id` against EDAM should skip ids carrying that prefix rather than treat them as resolvable. + #### Subject A human or organism from which biosamples are derived. diff --git a/src/cfdb/models.py b/src/cfdb/models.py index 73ad107..ddcbb60 100644 --- a/src/cfdb/models.py +++ b/src/cfdb/models.py @@ -393,7 +393,9 @@ class FileMetadataModel(BaseModel): file_format: An EDAM CV term identifying the digital format of this file - (e.g., TSV or FASTQ). If compressed, this is the uncompressed format. + (e.g., TSV or FASTQ), or a ``cfdb:``-prefixed token where EDAM + has no term for it; see :class:`FileFormat`. If compressed, + this is the uncompressed format. compression_format: An EDAM CV term ID identifying compression that is extrinsic to @@ -571,20 +573,34 @@ class AssayType(BaseModel): class FileFormat(BaseModel): """ - An EDAM CV 'format:' term. + An EDAM CV 'format:' term, or a term minted here where EDAM has none. Describes the digital format of C2M2 files. + Most ids are EDAM CV ``format:`` terms and resolve at edamontology.org. + A few formats EDAM does not cover -- ``bedpe`` and ``bigInteract`` at + the time of writing -- carry a token minted here instead, prefixed + ``cfdb:`` (:data:`cfdb.services.ontology_mappings.MINTED_FORMAT_PREFIX`, + the stable discriminator to test against). Those ids resolve nowhere: + aliasing such a format onto the nearest EDAM term would make it + indistinguishable from the format it was aliased to, and the processor + claiming that term would pick it up and mangle it. A consumer resolving + ids against EDAM must therefore skip the minted prefix rather than + assume every id is resolvable. + Attributes: id: - An EDAM CV format term identifier. + An EDAM CV format term identifier, or a ``cfdb:``-prefixed + token minted where EDAM has no term for the format. name: - A short, human-readable, machine-read-friendly label for this EDAM - format term. + A short, human-readable, machine-read-friendly label for this + format term. Distinct per format even where the id is minted: + workflow processor routing keys on this field, so two formats + sharing a name share a pipeline. description: - A human-readable description of this EDAM format term. + A human-readable description of this format term. """ id: str = str() diff --git a/src/cfdb/services/ontology_mappings.py b/src/cfdb/services/ontology_mappings.py index 70fd217..be700ac 100644 --- a/src/cfdb/services/ontology_mappings.py +++ b/src/cfdb/services/ontology_mappings.py @@ -1,5 +1,14 @@ """Ontology mappings for ENCODE metadata transformation to C2M2 format.""" +# Prefix marking a CV term minted here rather than drawn from EDAM. EDAM has +# no term for every format ENCODE publishes, and the alternative -- aliasing +# an unrepresented format onto the nearest EDAM term -- is what produced the +# starch/BED conflation (#69, #72): the format becomes indistinguishable from +# the one it was aliased to, and the processor that claims that EDAM term +# picks it up and mangles it. A minted token says "no standard term exists" +# without lying about what the file is. +MINTED_FORMAT_PREFIX = "cfdb:" + # ENCODE file_format to EDAM format CV terms # Reference: https://edamontology.org/page/formats FILE_FORMAT_TO_EDAM = { @@ -12,7 +21,16 @@ "cram": {"id": "format:3462", "name": "CRAM"}, # Genomic interval formats "bed": {"id": "format:3003", "name": "BED"}, - "bedpe": {"id": "format:3003", "name": "BED"}, # BED paired-end + # Deliberately NOT format:3003/BED. BEDPE columns are chrom1/start1/end1/ + # chrom2/start2/end2, so the BED tabix pipeline would sort and index the + # first mate and leave the second unindexed -- a cached artifact that + # looks successful and is wrong. EDAM has no BEDPE term (OLS4: + # q=bedpe&ontology=edam returns nothing), so the token is minted. The + # distinct *name* is what matters operationally: processor lookup keys on + # ``file_format.name`` (``processors.tools.format_name``), and "bedpe" is + # in no processor's ``supported_formats``, so /data streams the raw + # upstream file instead of a mangled index until a real processor exists. + "bedpe": {"id": f"{MINTED_FORMAT_PREFIX}bedpe", "name": "bedpe"}, "broadpeak": {"id": "format:3614", "name": "BroadPeak"}, "narrowpeak": {"id": "format:3613", "name": "NarrowPeak"}, "gappedpeak": {"id": "format:3003", "name": "BED"}, # gappedPeak is BED variant @@ -52,7 +70,18 @@ "database": {"id": "format:2330", "name": "Plain text"}, "starch": {"id": "format:3003", "name": "BED"}, # BEDOPS compressed BED archive "tagalign": {"id": "format:3003", "name": "BED"}, # tagAlign is a BED variant - "biginteract": {"id": "format:3004", "name": "bigBed"}, # bigInteract is a bigBed variant + # Structurally a bigBed, but its trailing columns encode an interaction's + # source and target endpoints. Extracting it with bigBedToBed and indexing + # the leading three columns is range-coherent but silently degrades the + # interaction to an interval, so it is kept distinct for the same reason + # as "bedpe" above. EDAM has no bigInteract term either. + # + # Unlike bedpe, this trades a working capability for an honest one: + # the old index answered range queries correctly, so /index on these + # files goes from degraded-but-usable to 404 until the tileset + # endpoints land. That reaches bigInteract already in the experiment + # corpus, not only newly ingested annotation files. + "biginteract": {"id": f"{MINTED_FORMAT_PREFIX}biginteract", "name": "bigInteract"}, "csfasta": {"id": "format:1929", "name": "FASTA"}, # color-space FASTA (SOLiD) "csqual": {"id": "format:2330", "name": "Plain text"}, # color-space quality scores "h5ad": {"id": "format:3590", "name": "HDF5"}, # AnnData HDF5 format From 61bcb02595e02d86194c443d7dc45885cfc1accd Mon Sep 17 00:00:00 2001 From: Conrad Date: Thu, 13 Aug 2026 14:02:44 -0400 Subject: [PATCH 02/15] feat: Map the ENCODE annotation output types to EDAM data terms These five values are the complete Output type domain of the two annotation types the sync ingests, verified against the live metadata TSVs, and none of them appeared in the table. Without them every one of the ~29,000 annotation files would be stored with a null data_type. The element and gene link types resolve to the generic data term rather than to a features or track term. EDAM has nothing for a predicted regulatory relationship between two loci, and the pre-existing chromatin interactions entry already made that call the same way. A deliberately vague term is preferable to claiming the files are something they are not. --- src/cfdb/services/ontology_mappings.py | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/src/cfdb/services/ontology_mappings.py b/src/cfdb/services/ontology_mappings.py index be700ac..adebb0c 100644 --- a/src/cfdb/services/ontology_mappings.py +++ b/src/cfdb/services/ontology_mappings.py @@ -144,6 +144,24 @@ "transcription start sites": {"id": "data:1255", "name": "Sequence features"}, "enhancer predictions": {"id": "data:1255", "name": "Sequence features"}, "long range chromatin interactions": {"id": "data:0006", "name": "Data"}, + # Annotation datasets (type=Annotation ingest) + # + # Verified against the live annotation TSVs: these five values are the + # complete Output type domain of the two ingested annotation_types, and + # none of them appeared above, so without these entries every annotation + # file would carry data_type=None. + # + # The element/gene link types resolve to the generic "data:0006" rather + # than to a features or track term. EDAM has nothing for a predicted + # regulatory relationship between two loci, and the pre-existing + # "chromatin interactions" entry already made that call the same way. A + # deliberately vague term is preferable to claiming these files are + # something they are not. + "candidate Cis-Regulatory Elements": {"id": "data:1255", "name": "Sequence features"}, + "elements reference": {"id": "data:1255", "name": "Sequence features"}, + "element gene links": {"id": "data:0006", "name": "Data"}, + "thresholded element gene links": {"id": "data:0006", "name": "Data"}, + "thresholded links": {"id": "data:0006", "name": "Data"}, # Reference data "genome reference": {"id": "data:2340", "name": "Genome identifier"}, "sequence alignability": {"id": "data:0006", "name": "Data"}, From 2c4991215fe277063c533a1816240d07e62d1417 Mon Sep 17 00:00:00 2001 From: Conrad Date: Thu, 13 Aug 2026 14:03:10 -0400 Subject: [PATCH 03/15] feat: Add the annotation fields to the ENCODE models and inputs Eight fields the annotation metadata TSV publishes and the experiment one does not: annotation_type and organism on the file, annotation_type, software_used and encyclopedia_version on the dataset, and life_stage, age and age_units on the biosample. annotation_type is held on the file as well as the dataset. The dataset is where the property belongs, but filtering files by it is the actual use case, and routing that through a collection subdocument on every query is not worth avoiding one duplicated string. The age fields stay strings rather than becoming a number. The released corpus contains "2-4" and "unknown" alongside decimals, and it distinguishes "10.5" from "10.50". That is also why they do not go to Subject.age_at_sampling, which is a float in years and could represent neither the ranges nor the sentinels -- and an annotation row names no donor to build a Subject from in any case. The output types are derived from the models, so the SDL gains all eight on both the input and the output side. --- schema.graphql | 16 ++++++++ src/cfdb/api/gql/inputs.py | 8 ++++ src/cfdb/models.py | 75 +++++++++++++++++++++++++++++++++++++- 3 files changed, 97 insertions(+), 2 deletions(-) diff --git a/schema.graphql b/schema.graphql index 079dcaf..7497264 100644 --- a/schema.graphql +++ b/schema.graphql @@ -156,6 +156,9 @@ type EnrichedCollectionType { input EnrichedEncodeBiosampleInput { biosampleType: [String!] = null + lifeStage: [String!] = null + age: [String!] = null + ageUnits: [String!] = null biosampleTreatments: [String!] = null biosampleTreatmentsAmount: [String!] = null biosampleTreatmentsDuration: [String!] = null @@ -172,6 +175,9 @@ input EnrichedEncodeBiosampleInput { type EnrichedEncodeBiosampleType { biosampleType: String + lifeStage: String + age: String + ageUnits: String biosampleTreatments: String biosampleTreatmentsAmount: String biosampleTreatmentsDuration: String @@ -192,6 +198,9 @@ input EnrichedEncodeCollectionInput { platform: [String!] = null dbxrefs: [String!] = null rbnsProteinConcentration: [String!] = null + annotationType: [String!] = null + softwareUsed: [String!] = null + encyclopediaVersion: [String!] = null } type EnrichedEncodeCollectionType { @@ -199,10 +208,15 @@ type EnrichedEncodeCollectionType { platform: String dbxrefs: String rbnsProteinConcentration: String + annotationType: String + softwareUsed: String + encyclopediaVersion: String } input EnrichedEncodeFileInput { assembly: [String!] = null + annotationType: [String!] = null + organism: [String!] = null fileFormatType: [String!] = null outputType: [String!] = null genomeAnnotation: [String!] = null @@ -225,6 +239,8 @@ input EnrichedEncodeFileInput { type EnrichedEncodeFileType { assembly: String + annotationType: String + organism: String fileFormatType: String outputType: String genomeAnnotation: String diff --git a/src/cfdb/api/gql/inputs.py b/src/cfdb/api/gql/inputs.py index 2c1189d..fb9871a 100644 --- a/src/cfdb/api/gql/inputs.py +++ b/src/cfdb/api/gql/inputs.py @@ -70,6 +70,9 @@ class SubjectInput: @strawberry.input class EnrichedEncodeBiosampleInput: biosample_type: list[str] | None = None + life_stage: list[str] | None = None + age: list[str] | None = None + age_units: list[str] | None = None biosample_treatments: list[str] | None = None biosample_treatments_amount: list[str] | None = None biosample_treatments_duration: list[str] | None = None @@ -134,6 +137,9 @@ class EnrichedEncodeCollectionInput: platform: list[str] | None = None dbxrefs: list[str] | None = None rbns_protein_concentration: list[str] | None = None + annotation_type: list[str] | None = None + software_used: list[str] | None = None + encyclopedia_version: list[str] | None = None @strawberry.input @@ -216,6 +222,8 @@ class EnrichedFourdnFileInput: @strawberry.input class EnrichedEncodeFileInput: assembly: list[str] | None = None + annotation_type: list[str] | None = None + organism: list[str] | None = None file_format_type: list[str] | None = None output_type: list[str] | None = None genome_annotation: list[str] | None = None diff --git a/src/cfdb/models.py b/src/cfdb/models.py index ddcbb60..bb7d09d 100644 --- a/src/cfdb/models.py +++ b/src/cfdb/models.py @@ -130,9 +130,33 @@ class EnrichedFourdnFile(BaseModel): class EnrichedEncodeFile(BaseModel): - """ENCODE file-level metadata from metadata TSV.""" + """ENCODE file-level metadata from metadata TSV. + + Populated from either the Experiment or the Annotation metadata TSV. + The two share only part of their column sets, so a field sourced from a + column the other TSV does not publish is None on those documents rather + than derived from something else -- see the annotation mapping in + :mod:`cfdb.services.encode`. + + Attributes: + annotation_type: + The kind of annotation this file belongs to (e.g. "candidate + Cis-Regulatory Elements"). The field that gives an annotation + file its meaning, and the one a client filters on to ask for + cCREs without string-matching filenames. None on experiment + files, whose TSV has no such column. + + organism: + Scientific name of the source organism (e.g. "Homo sapiens"). + Also reaches ``subjects[].taxonomy`` on experiment files, but + annotation rows name no donor and so build no subject -- this is + the only place the organism of a multi-organism annotation + result set is queryable. + """ assembly: Optional[str] = None + annotation_type: Optional[str] = None + organism: Optional[str] = None file_format_type: Optional[str] = None output_type: Optional[str] = None genome_annotation: Optional[str] = None @@ -227,12 +251,37 @@ def empty_string_to_none(cls, v): class EnrichedEncodeCollection(BaseModel): - """ENCODE experiment-level metadata from metadata TSV.""" + """ENCODE dataset-level metadata from metadata TSV. + + A "dataset" is an Experiment or an Annotation depending on which TSV the + document came from; ``platform`` and ``rbns_protein_concentration`` are + experiment-only, ``annotation_type``, ``software_used`` and + ``encyclopedia_version`` annotation-only. + + Attributes: + annotation_type: + The annotation kind this dataset publishes, mirroring + :attr:`EnrichedEncodeFile.annotation_type`. Held on the dataset + as well as the file because that is the entity the property + actually describes. + + software_used: + Software that produced the annotation, as a comma-separated + list (e.g. "ABC-Enhancer-Gene-Prediction, Distal regulation + ENCODE-rE2G"). Blank for some annotation types. + + encyclopedia_version: + The ENCODE Encyclopedia release the annotation belongs to (e.g. + "ENCODE v4", "ENCODE v3, current"). + """ project: Optional[str] = None platform: Optional[str] = None dbxrefs: Optional[str] = None rbns_protein_concentration: Optional[str] = None + annotation_type: Optional[str] = None + software_used: Optional[str] = None + encyclopedia_version: Optional[str] = None class EnrichedFourdnCollection(BaseModel): @@ -298,9 +347,31 @@ class EnrichedEncodeBiosample(BaseModel): ENCODE biosample-level metadata from metadata TSV. Contains biosample classification, treatment, and library information. + + Attributes: + life_stage: + Developmental stage of the source organism when the biosample + was taken (e.g. "embryonic", "adult", "young adult", "unknown"). + + age: + Age of the source organism at sampling, in ``age_units``, kept + as the upstream string. NOT parsed to a number: the released + annotation corpus contains "2-4" and "unknown" alongside plain + decimals, and it distinguishes "10.5" from "10.50". This is also + why the value does not go to :attr:`Subject.age_at_sampling`, + which is a float in years -- that field could represent neither + the ranges nor the sentinels, and an annotation row names no + donor to build a Subject from in the first place. + + age_units: + Unit for :attr:`age` ("year", "month", "week", "day"). Blank + when ``age`` is absent or a sentinel. """ biosample_type: Optional[str] = None + life_stage: Optional[str] = None + age: Optional[str] = None + age_units: Optional[str] = None biosample_treatments: Optional[str] = None biosample_treatments_amount: Optional[str] = None biosample_treatments_duration: Optional[str] = None From ed465dc52d5512b8c22e1b118a9a63216e53f9ce Mon Sep 17 00:00:00 2001 From: Conrad Date: Thu, 13 Aug 2026 14:04:46 -0400 Subject: [PATCH 04/15] feat: Ingest released ENCODE annotations per configured type Everything ENCODE publishes as an Annotation -- candidate cis-regulatory elements, element gene regulatory interaction predictions, chromatin state, footprints -- was absent from cfdb, which fetched only type=Experiment. Annotation files are the interpretive layer over the raw experiments: a cCRE track is what a Gosling visualization usually wants alongside signal. The streaming loop is extracted so both TSVs share it, and each stream now names itself in its log lines. It takes a deadline rather than resolving its own budget, so several streams in one sync share a single allowance; a stream that finds the budget already spent refuses to open a request rather than passing the remainder through, since aiohttp reads a non-positive total as no timeout and would turn the exhausted case into an unbounded request. Which annotation types are ingested is read from ENCODE_ANNOTATION_TYPES rather than hardcoded: the full space is 580,910 datasets, 86% of it footprints, so the allowlist has to exist from the outset rather than be retrofitted once volume becomes a problem. Entries are trimmed and deduplicated, since files are written with insert_many into a collection with no unique key and a repeated token would load a whole type twice. One request per type, so a failure or a gateway timeout on one costs only that type. An explicitly empty value disables annotation ingest, and now says so. The neighbouring ENCODE_METADATA_TIMEOUT_SECONDS reads an empty value the other way, as unset, and empty values arrive by accident routinely -- an unset CloudFormation parameter, a docker-compose expansion that produced nothing. Turning the path off stays a legitimate operator choice, but without the warning the only trace of an accidental one is an absence in the per-phase log. The two TSVs do not share a column set -- Annotation publishes 32 columns to Experiment's 59, six of them renamed. Rather than a second transformation, the annotation columns are renamed to their experiment equivalents and run through the shared one, so a single mapping serves both and a column the TSV omits comes out unset instead of derived from something else. That is how an annotation document ends up with no subjects: there is no Donor(s) column to build one from, and inventing a donor would be worse than admitting there is none. Two deliberate departures from the experiment path. The dataset collection is built from the accession alone rather than requiring a biosample term, because 48 released cCRE files name no biosample and gating on one would leave 24 dataset accessions unqueryable. And the collection's persistent_id points at /annotations/, since ENCODE serves the two dataset kinds under different paths. Also populates extra.encode.assembly, which the schema has always published and nothing ever wrote -- a client filtering on it matched nothing at all. Multi-assembly cCREs, spanning GRCh38, mm10 and hg19, are what make that worth fixing now. --- src/cfdb/services/encode.py | 439 ++++++++++++++++++++++++++++++++---- 1 file changed, 394 insertions(+), 45 deletions(-) diff --git a/src/cfdb/services/encode.py b/src/cfdb/services/encode.py index 5dbafed..5a4c89c 100644 --- a/src/cfdb/services/encode.py +++ b/src/cfdb/services/encode.py @@ -106,8 +106,9 @@ import logging import os import re +from contextlib import aclosing from typing import AsyncGenerator, Optional -from urllib.parse import unquote, urlsplit +from urllib.parse import unquote, urlencode, urlsplit import aiohttp @@ -151,27 +152,121 @@ def _timeout_from_env(name: str, default: int) -> int: #: a malformed value should fail the sync that reads it, not the import of #: this module, which would take the whole API down over a knob only the #: sync uses. +#: +#: The budget covers *the whole sync*, not each stream in it. A sync runs one +#: stream per configured ``annotation_type`` plus one for experiments, all +#: inside a single cutover lock that gates the read surface, so a per-stream +#: budget would multiply the outage by the number of phases and put the worst +#: case past the sync lock's one-hour stale threshold -- at which point a +#: second sync is admitted and clears the corpus while the first is still +#: writing into it. Callers share one deadline; see +#: :func:`metadata_budget_seconds`. _METADATA_TIMEOUT_ENV = "ENCODE_METADATA_TIMEOUT_SECONDS" _METADATA_TIMEOUT_DEFAULT_SECONDS = 3600 -async def fetch_encode_metadata() -> AsyncGenerator[dict, None]: +def metadata_budget_seconds() -> int: + """Resolve the whole-sync metadata download budget, in seconds. + + Exposed so a sync running several streams resolves the budget once and + shares one deadline across them, rather than granting each stream its + own full allowance. """ - Fetch all released experiment files from ENCODE metadata TSV endpoint. + return _timeout_from_env( + _METADATA_TIMEOUT_ENV, _METADATA_TIMEOUT_DEFAULT_SECONDS + ) - Uses the /metadata/ endpoint which returns a single TSV file containing - all matching records, avoiding the need for paginated JSON API calls. - The response is streamed line-by-line to avoid loading the full TSV - (hundreds of MB) into memory. +#: Environment variable overriding which ``annotation_type`` values are +#: ingested, and the allowlist applied when it is unset. An allowlist rather +#: than a filter retrofitted later: ENCODE publishes 580,910 annotation +#: datasets and 86% of them are footprints, so ingesting the type space +#: wholesale and pruning afterwards would be a corpus-scale mistake to undo. +#: The two defaults are ~7,773 datasets, proportionate against the 27,043 +#: experiments already ingested. +_ANNOTATION_TYPES_ENV = "ENCODE_ANNOTATION_TYPES" +_ANNOTATION_TYPES_DEFAULT: tuple[str, ...] = ( + "candidate Cis-Regulatory Elements", + "element gene regulatory interaction predictions", +) - Yields: - Dicts keyed by TSV column names for each file row + +def annotation_types_from_env() -> tuple[str, ...]: + """Resolve the ``annotation_type`` allowlist to ingest. + + Reads a comma-separated ``ENCODE_ANNOTATION_TYPES``, stripping each + entry and dropping blanks. An unset variable yields the default + allowlist; a variable set to an empty (or all-blank) value yields an + empty tuple, which disables annotation ingest entirely. That distinction + is deliberate -- turning the annotation path off is a legitimate + operator choice, and having to edit code to make it is worse than + honoring an explicitly empty setting. It is also the opposite of what + :func:`_timeout_from_env` does with an empty value, so the empty case + logs a warning rather than leaving the operator to infer it from an + absence in the per-phase log. + + Repeats are collapsed, first occurrence winning, because the sync runs + one ingest phase per returned entry and ENCODE files are written with + ``insert_many`` into a collection with no unique key: a duplicated token + in a task definition would otherwise insert every file of that type + twice, with nothing to reject it and no way back short of a full + re-sync. + + Resolved per call rather than at import, matching + :func:`_timeout_from_env`: the value is only meaningful to the sync, so + a bad one should fail the sync rather than the API's import of this + module. """ - config = get_dcc_config("encode") - api_base = config["api_base"] + raw = os.getenv(_ANNOTATION_TYPES_ENV) + if raw is None: + return _ANNOTATION_TYPES_DEFAULT + entries = (entry.strip() for entry in raw.split(",")) + # dict.fromkeys rather than a set: order is the ingest order, and a + # reordered allowlist would shuffle the log and the phase sequence. + allowlist = tuple(dict.fromkeys(entry for entry in entries if entry)) + + if not allowlist: + # Warned about because the neighbouring :func:`_timeout_from_env` + # reads an empty value as "unset, use the default" and this one + # reads it as "ingest nothing" -- a difference an operator has no + # reason to expect between two variables of the same module. Empty + # values arrive by accident routinely: an unset CloudFormation + # parameter, a docker-compose ``${VAR}`` that expanded to nothing, + # an ECS task definition entry with an empty ``value``. Disabling + # the annotation ingest is a legitimate choice, so this stays a + # warning rather than an error -- but silently loading no + # annotations shows up only as an absence in the per-phase log, + # and an absence is what nobody notices. + logger.warning( + "%s is set but empty: annotation ingest is disabled and no " + "annotation files will be loaded. Unset it entirely to restore " + "the default allowlist (%s).", + _ANNOTATION_TYPES_ENV, + ", ".join(_ANNOTATION_TYPES_DEFAULT), + ) - metadata_url = f"{api_base}/metadata/?type=Experiment&status=released" + return allowlist + + +async def _stream_metadata_tsv( + metadata_url: str, label: str, deadline: float | None = None +) -> AsyncGenerator[dict, None]: + """Stream one ENCODE ``/metadata/`` TSV, yielding a dict per data row. + + ``label`` names the stream in log lines so that, with several streams + per sync, a progress or timeout message identifies which one it came + from. + Args: + metadata_url: Fully-formed ``/metadata/`` URL to stream. + label: Human-readable name for this stream, used in log messages. + deadline: Event-loop clock reading (:meth:`asyncio.AbstractEventLoop.time`) + by which this stream must finish, shared with every other stream + in the same sync. ``None`` grants a fresh full budget, which is + correct only for a stream running on its own. + + Yields: + Dicts keyed by TSV column names for each file row + """ headers = { "User-Agent": "cfdb/1.0", } @@ -184,9 +279,21 @@ async def fetch_encode_metadata() -> AsyncGenerator[dict, None]: # was not enough against DocumentDB -- the sync aborted around 230,000 of # ~810,000 rows and, because the DCC is cleared before reloading, left the # corpus smaller than it started. - timeout_seconds = _timeout_from_env( - _METADATA_TIMEOUT_ENV, _METADATA_TIMEOUT_DEFAULT_SECONDS - ) + timeout_seconds: float + if deadline is None: + timeout_seconds = metadata_budget_seconds() + else: + timeout_seconds = deadline - asyncio.get_running_loop().time() + if timeout_seconds <= 0: + # Refused rather than clamped to zero: aiohttp reads a + # non-positive total as "no timeout", so passing the exhausted + # remainder through would turn a spent budget into an unbounded + # request -- the exact failure the budget exists to prevent. + raise asyncio.TimeoutError( + f"ENCODE {label} metadata fetch not attempted: the " + f"{_METADATA_TIMEOUT_ENV} budget was already spent by an " + "earlier phase of this sync" + ) async with aiohttp.ClientSession() as session: try: @@ -196,9 +303,13 @@ async def fetch_encode_metadata() -> AsyncGenerator[dict, None]: timeout=aiohttp.ClientTimeout(total=timeout_seconds), ) as response: if response.status != 200: - logger.error(f"ENCODE metadata API error: HTTP {response.status}") + logger.error( + f"ENCODE {label} metadata API error: " + f"HTTP {response.status}" + ) raise Exception( - f"ENCODE metadata API error: HTTP {response.status}" + f"ENCODE {label} metadata API error: " + f"HTTP {response.status}" ) # Stream line-by-line to keep memory usage constant @@ -219,11 +330,13 @@ async def fetch_encode_metadata() -> AsyncGenerator[dict, None]: if total_fetched % 50000 == 0: logger.info( - f"Parsed {total_fetched} ENCODE metadata rows..." + f"Parsed {total_fetched} ENCODE {label} " + "metadata rows..." ) logger.info( - f"ENCODE metadata fetch complete: {total_fetched} rows" + f"ENCODE {label} metadata fetch complete: " + f"{total_fetched} rows" ) except asyncio.TimeoutError: @@ -233,16 +346,105 @@ async def fetch_encode_metadata() -> AsyncGenerator[dict, None]: # is the one fact that distinguishes "the budget is too small" # from "the endpoint is down". logger.error( - f"ENCODE metadata fetch timed out after {total_fetched} rows " - f"({timeout_seconds}s budget); the files collection is left " - f"partially loaded. Raise {_METADATA_TIMEOUT_ENV} and re-run " - "the sync." + f"ENCODE {label} metadata fetch timed out after " + f"{total_fetched} rows ({timeout_seconds:.0f}s of the " + f"{_METADATA_TIMEOUT_ENV} budget remained when it started); " + "the files collection is left partially loaded. Raise " + f"{_METADATA_TIMEOUT_ENV} and re-run the sync." ) raise except aiohttp.ClientError as e: - logger.error(f"ENCODE metadata API network error: {e}") - raise Exception(f"ENCODE metadata API network error: {e}") + logger.error(f"ENCODE {label} metadata API network error: {e}") + raise Exception(f"ENCODE {label} metadata API network error: {e}") + + +async def fetch_encode_metadata( + deadline: float | None = None, +) -> AsyncGenerator[dict, None]: + """ + Fetch all released experiment files from ENCODE metadata TSV endpoint. + + Uses the /metadata/ endpoint which returns a single TSV file containing + all matching records, avoiding the need for paginated JSON API calls. + The response is streamed line-by-line to avoid loading the full TSV + (hundreds of MB) into memory. + + Args: + deadline: Event-loop clock reading by which this fetch must finish, + shared with the other streams of the same sync. ``None`` grants + a fresh full budget; see :func:`metadata_budget_seconds`. + + Yields: + Dicts keyed by TSV column names for each file row + """ + config = get_dcc_config("encode") + api_base = config["api_base"] + + metadata_url = f"{api_base}/metadata/?type=Experiment&status=released" + + # ``aclosing`` because ``async for`` does not close the iterator it + # abandons: without it, closing this generator raises GeneratorExit here + # and leaves the inner one -- which owns the aiohttp session -- suspended + # at its yield with the session still open. + async with aclosing( + _stream_metadata_tsv(metadata_url, "experiment", deadline) + ) as stream: + async for row in stream: + yield row + + +async def fetch_encode_annotation_metadata( + annotation_type: str, + deadline: float | None = None, +) -> AsyncGenerator[dict, None]: + """ + Fetch all released annotation files of one ``annotation_type``. + + The same ``/metadata/`` endpoint as the experiment fetch, against + ``type=Annotation``. The two TSVs do not share a column set -- Annotation + publishes 32 columns to Experiment's 59, with six of the shared ones + renamed -- so rows from here must go through + :func:`transform_annotation_to_c2m2`, not :func:`transform_to_c2m2`. + + One request per annotation type, rather than one request carrying + repeated ``annotation_type`` parameters, so that a failure or a timeout + on one type costs only that type. It also keeps each response small + enough to come back: the portal returns 504 on requests that take too + long to assemble. + + Args: + annotation_type: An ENCODE ``annotation_type`` value, e.g. + "candidate Cis-Regulatory Elements". Passed through URL + encoding, so spaces and punctuation need no pre-escaping. + deadline: Event-loop clock reading by which this fetch must finish, + shared with the other streams of the same sync. ``None`` grants + a fresh full budget; see :func:`metadata_budget_seconds`. + + Yields: + Dicts keyed by TSV column names for each file row + """ + config = get_dcc_config("encode") + api_base = config["api_base"] + + query = urlencode( + { + "type": "Annotation", + "status": "released", + "annotation_type": annotation_type, + } + ) + metadata_url = f"{api_base}/metadata/?{query}" + + # See fetch_encode_metadata for why the inner stream is closed + # explicitly rather than left to ``async for``. + async with aclosing( + _stream_metadata_tsv( + metadata_url, f"annotation[{annotation_type}]", deadline + ) + ) as stream: + async for row in stream: + yield row def _nonempty(value: str | None) -> str | None: @@ -380,12 +582,45 @@ def _extract_donor_ids(donors_str: str | None) -> list[str]: return ids -def transform_to_c2m2(row: dict) -> Optional[dict]: +# Annotation TSV column -> the Experiment TSV column holding the same datum. +# Verified against the live headers of both ingested annotation types (both +# 32 columns, identical to each other). Applied by +# :func:`transform_annotation_to_c2m2` before the shared transformation, so +# that a rename is the only thing standing between the two TSVs and one +# mapping serves both. Every other shared column already agrees by name. +ANNOTATION_COLUMN_ALIASES = { + "Dataset accession": "Experiment accession", + "Assay term name": "Assay", + "Assembly": "File assembly", + "Dataset date released": "Experiment date released", + "S3 URL": "s3_uri", + "Organism": "Biosample organism", +} + + +def _transform_row( + row: dict, *, dataset_path: str, require_biosample: bool +) -> Optional[dict]: """ Transform an ENCODE metadata TSV row to C2M2-compatible document for MongoDB. + Shared by the experiment and annotation ingest paths. Every field is read + under its *experiment* column name; the annotation path renames its + columns to match before calling here (see + :data:`ANNOTATION_COLUMN_ALIASES`). A column the caller's TSV does not + publish is simply absent from ``row``, so the corresponding field comes + out unset rather than derived from something else. + Args: row: Dict keyed by TSV column names + dataset_path: Path segment for the collection's persistent_id -- + "experiments" or "annotations". ENCODE serves the two dataset + kinds under different URLs, and a link to the wrong one 404s. + require_biosample: When True, a row with no ``Biosample term name`` + produces no collection at all -- preserving the experiment + path's long-standing behavior. When False, the collection is + built from the dataset accession alone and simply carries no + biosamples, so that the accession stays queryable. Returns: C2M2-compatible dict for insertion into files collection, or None if invalid @@ -474,10 +709,19 @@ def transform_to_c2m2(row: dict) -> Optional[dict]: biosample_organism = _nonempty(row.get("Biosample organism")) donors_raw = _nonempty(row.get("Donor(s)")) - if biosample_term_name: + # A biosample can only be built from a biosample term. Whether its + # absence also suppresses the *collection* is the caller's call: 48 of + # the released cCRE files name no biosample term, and dropping their + # collection would make 24 dataset accessions unqueryable. + build_biosample = bool(biosample_term_name) + build_collection = build_biosample or ( + not require_biosample and bool(experiment_accession) + ) + + if build_collection: # Build anatomy from biosample term anatomy = None - if biosample_term_id: + if biosample_term_id and biosample_term_name: anatomy = {"id": biosample_term_id, "name": biosample_term_name} # Build subjects from donor(s) and organism @@ -501,6 +745,15 @@ def transform_to_c2m2(row: dict) -> Optional[dict]: if biosample_type: biosample_extra["biosample_type"] = biosample_type + # Donor characteristics, which ENCODE resolves onto the biosample + # upstream -- which is why the annotation TSV publishes them despite + # having no Donor(s) column. Kept as the upstream strings; see + # EnrichedEncodeBiosample for why they are not parsed into + # Subject.age_at_sampling. + _add_extra(biosample_extra, "life_stage", row.get("Life stage")) + _add_extra(biosample_extra, "age", row.get("Age")) + _add_extra(biosample_extra, "age_units", row.get("Age units")) + # Treatment fields treatments = _nonempty(row.get("Biosample treatments")) if treatments: @@ -555,19 +808,22 @@ def transform_to_c2m2(row: dict) -> Optional[dict]: ) # Build biosample - biosample = { - "id_namespace": id_namespace, - "local_id": f"biosample:{biosample_term_name}", - "project_id_namespace": id_namespace, - "project_local_id": "ENCODE", - "subjects": subjects, - } - if anatomy: - biosample["anatomy"] = anatomy - if biosample_extra: - biosample["extra"] = {"encode": biosample_extra} + biosamples = [] + if build_biosample: + biosample = { + "id_namespace": id_namespace, + "local_id": f"biosample:{biosample_term_name}", + "project_id_namespace": id_namespace, + "project_local_id": "ENCODE", + "subjects": subjects, + } + if anatomy: + biosample["anatomy"] = anatomy + if biosample_extra: + biosample["extra"] = {"encode": biosample_extra} + biosamples.append(biosample) - # Build collection extra (experiment-level fields) + # Build collection extra (dataset-level fields) collection_encode_extra = {} _add_extra(collection_encode_extra, "project", row.get("Project")) _add_extra(collection_encode_extra, "platform", row.get("Platform")) @@ -578,12 +834,13 @@ def transform_to_c2m2(row: dict) -> Optional[dict]: row.get("RBNS protein concentration"), ) - # Build collection — keyed by experiment accession, fallback to biosample + # Build collection — keyed by dataset accession, fallback to biosample if experiment_accession: collection_local_id = experiment_accession collection_name = experiment_accession collection_persistent_id = ( - f"https://www.encodeproject.org/experiments/{experiment_accession}/" + f"https://www.encodeproject.org/{dataset_path}/" + f"{experiment_accession}/" ) else: collection_local_id = f"biosample:{biosample_term_name}" @@ -594,12 +851,12 @@ def transform_to_c2m2(row: dict) -> Optional[dict]: "id_namespace": id_namespace, "local_id": collection_local_id, "name": collection_name, - "biosamples": [biosample], + "biosamples": biosamples, "subjects": subjects, } - # Only the experiment-keyed branch has an accession. The + # Only the dataset-keyed branch has an accession. The # ``biosample:``-keyed fallback collection is synthesized locally and - # names no ENCODE experiment, so it is left unset rather than given a + # names no ENCODE dataset, so it is left unset rather than given a # fabricated accession. if experiment_accession: collection["accession_id"] = normalize_accession(experiment_accession) @@ -650,6 +907,23 @@ def transform_to_c2m2(row: dict) -> Optional[dict]: _add_extra(extra, "s3_uri", row.get("s3_uri")) _add_extra(extra, "azure_url", row.get("Azure URL")) + # Mirrors the top-level genome_assembly under the DCC namespace, the way + # extra.fourdn.genome_assembly and extra.hubmap.genome_assembly mirror it + # for their DCCs. The field was declared and published in the SDL but + # never written, so a client reading the schema and reaching for + # extra.encode.assembly -- the natural move once cCREs made assembly a + # filter people actually use -- silently matched nothing. + _add_extra(extra, "assembly", row.get("File assembly")) + + # Organism, from the same column that feeds subjects[].taxonomy above. + # Recorded here as well because an annotation row names no donor, so it + # builds no subject -- without this, the organism of a multi-organism + # result set (the released cCREs span Homo sapiens and Mus musculus) + # would be recoverable only by inspecting filenames. Set on experiment + # rows too, from the identical datum, so the filter means the same thing + # across the whole corpus rather than only over annotations. + _add_extra(extra, "organism", row.get("Biosample organism")) + # Analysis metadata _add_extra(extra, "file_analysis_title", row.get("File analysis title")) _add_extra(extra, "file_analysis_status", row.get("File analysis status")) @@ -678,6 +952,81 @@ def transform_to_c2m2(row: dict) -> Optional[dict]: return doc +def transform_to_c2m2(row: dict) -> Optional[dict]: + """ + Transform an ENCODE *experiment* metadata TSV row to a C2M2 document. + + Args: + row: Dict keyed by TSV column names, as yielded by + :func:`fetch_encode_metadata` + + Returns: + C2M2-compatible dict for insertion into files collection, or None if invalid + """ + return _transform_row(row, dataset_path="experiments", require_biosample=True) + + +def transform_annotation_to_c2m2(row: dict) -> Optional[dict]: + """ + Transform an ENCODE *annotation* metadata TSV row to a C2M2 document. + + Renames the annotation TSV's columns to their experiment equivalents + (:data:`ANNOTATION_COLUMN_ALIASES`), runs the shared transformation, then + applies the seven annotation-only columns. Experiment-only fields are + left unset rather than derived: the aliased row simply does not carry + those keys, so, for instance, ``_extract_donor_ids`` receives nothing and + the document ends up with no subjects rather than an invented donor. + + Args: + row: Dict keyed by TSV column names, as yielded by + :func:`fetch_encode_annotation_metadata` + + Returns: + C2M2-compatible dict for insertion into files collection, or None if invalid + """ + aliased = dict(row) + for annotation_column, experiment_column in ANNOTATION_COLUMN_ALIASES.items(): + if annotation_column in aliased: + aliased[experiment_column] = aliased.pop(annotation_column) + + doc = _transform_row( + aliased, dataset_path="annotations", require_biosample=False + ) + if doc is None: + return None + + annotation_type = _nonempty(row.get("Annotation type")) + + # On the file as well as the dataset. The dataset is where the property + # belongs, but filtering files by annotation_type is the actual use case + # -- "give me the cCRE files" -- and routing that through a collection + # subdocument for every query is not worth the single duplicated string. + if annotation_type: + doc.setdefault("extra", {}).setdefault("encode", {})["annotation_type"] = ( + annotation_type + ) + + for collection in doc.get("collections", []): + collection_extra = collection.setdefault("extra", {}).setdefault("encode", {}) + if annotation_type: + collection_extra["annotation_type"] = annotation_type + _add_extra(collection_extra, "software_used", row.get("Software used")) + _add_extra( + collection_extra, "encyclopedia_version", row.get("Encyclopedia Version") + ) + if not collection_extra: + del collection["extra"] + + # Reuses the scalar experiment_target rather than adding a parallel + # list field: the TSV hands over a string either way. Note that + # ENCODE does not order it -- both "CTCF-human, H3K4me3-human" and + # "H3K4me3-human, CTCF-human" occur in the released corpus -- so an + # equality filter on the whole value matches one permutation only. + _add_extra(collection, "experiment_target", row.get("Targets")) + + return doc + + def _add_extra(extra: dict, key: str, value: str | None) -> None: """Add a non-empty value to the extra dict.""" v = _nonempty(value) From 4f05f3b84e69514bf82d8a3af012ef1f9f75a3cb Mon Sep 17 00:00:00 2001 From: Conrad Date: Thu, 13 Aug 2026 14:05:10 -0400 Subject: [PATCH 05/15] feat: Run the ENCODE ingest as isolated per-stream phases The sync now runs one phase for released experiments plus one per configured annotation type. A phase that raises is logged and recorded and the remaining phases still run, so a transient failure on one stream does not cost the corpus the others. The task still fails at the end naming the phases that broke -- reporting a clean sync over a partially loaded collection would be worse than a visible failure. Getting that right needed more than a try/except around each phase. A stream can die mid-flight, which is exactly what an asyncio.TimeoutError against the metadata budget does, and reporting a phase's contribution only on its clean return meant a failed phase contributed nothing to the total even though its completed batches were already committed. The tallies, mutated per row, kept counting the rows the partial batch then discarded. Three numbers in one log block disagreeing with each other and with the database. The row count is now reported by mutating an accumulator, and a batch left buffered by a dead stream is flushed on the way out, so a partial load reports itself accurately. The clean-path flush is deliberately not in that finally. A finally runs whether or not an exception is in flight, so suppressing the flush's failure there suppressed it on the success path too: a stream that drained cleanly and then failed to commit its trailing rows returned normally, and the sync reported success over a corpus short by up to a batch. Each insert also detaches its buffer before awaiting, because the driver stamps _id in place before sending, so a resubmitted batch collides on the prefix that already landed and silently drops the rest. Cancellation skips the flush entirely: the decision to stop has been made, and a CancelledError redelivered by that await would replace the original and skip the phase's tally. Isolation covers the destructive half too. Each phase owns a slice of files -- experiments are the documents carrying no annotation type, each annotation phase the documents carrying its own -- and deletes that slice only once it has replacement rows in hand. Clearing the whole DCC before the fan-out meant a failed experiment phase left the API serving the 29,000 annotation documents as the entire corpus. A stream that drains cleanly with no rows still clears, so a type ENCODE stops publishing stops being served. The download budget covers the whole sync rather than each stream in it. Every phase runs inside the one cutover lock that gates the read surface, so a per-stream budget multiplied the outage by the phase count and carried the worst case past the sync lock's one-hour stale threshold, at which point a second sync is admitted and clears the corpus while the first is still writing into it. The budget is resolved once and each phase gets what remains of it. Isolating phases also means abandoning a generator mid-iteration on every failure. The stream owns an aiohttp session inside its own context manager, released only when the generator is finalized, so the streams are closed explicitly rather than left to the garbage collector -- with one stream per sync that was academic, with N+1 it is not. The annotation tally is seeded from the configured allowlist so a type ENCODE stops publishing reports zero rather than vanishing from the log. An absent key is what nobody notices; a zero is what everybody does. The broad except stays narrower than BaseException on purpose: a CancelledError caught there would be logged as a phase failure and followed by every remaining phase, which is the opposite of cancelling. --- src/cfdb/services/sync.py | 352 ++++++++++++++++++++++++++++++++++---- 1 file changed, 319 insertions(+), 33 deletions(-) diff --git a/src/cfdb/services/sync.py b/src/cfdb/services/sync.py index 884e37a..732688a 100644 --- a/src/cfdb/services/sync.py +++ b/src/cfdb/services/sync.py @@ -8,10 +8,12 @@ import shutil import subprocess from collections import Counter +from contextlib import aclosing from copy import copy from dataclasses import dataclass, field from datetime import datetime from enum import Enum +from functools import partial from pathlib import Path from typing import Optional @@ -1000,16 +1002,195 @@ async def _enrich_hubmap_files(dataset_metadata: dict[str, dict]) -> None: ) +async def _ingest_encode_rows( + task: SyncTask, + label: str, + rows, + transform, + compression_counts: Counter[str | None], + annotation_type_counts: Counter[str], + counts_by_phase: dict[str, int], + stale_filter: dict, +) -> None: + """Transform and batch-insert one ENCODE metadata stream. + + Shared by the experiment and annotation phases so both use the same + batching, the same skip-on-None handling, and contribute to the same + corpus-wide tallies. + + The row count is reported by mutating ``counts_by_phase`` rather than by + returning, and a partial batch left buffered by a dead stream is flushed + in a ``finally``. Both exist for the same reason: a stream can die + *mid-flight* -- an ``asyncio.TimeoutError`` against the metadata budget + is the known ENCODE failure mode -- and a return value is lost when it + does. Reporting by return meant a failed phase contributed nothing to + the caller's total even though its completed batches were already + committed, so the sync under-reported the corpus by up to everything + that phase had loaded, while the tallies below (mutated per row, not per + batch) over-reported it by the discarded partial batch. Three numbers in + one log block that disagreed with each other and with the database. + + The *clean-path* flush is deliberately not in that ``finally``. A + ``finally`` runs whether or not an exception is in flight, so suppressing + the flush's failure there suppressed it on the success path too: a stream + that drained cleanly and then failed to commit its trailing rows returned + normally and the sync reported a clean load over a corpus short by up to + ``BATCH_SIZE - 1`` rows, against a DCC cleared before the load. + + Args: + task: Sync task whose ``progress`` is updated per batch. + label: Names this stream in progress and log lines. + rows: Async iterable of TSV row dicts. + transform: Row -> document callable; None means skip the row. + compression_counts: Mutated with each document's compression term. + annotation_type_counts: Mutated with each document's annotation type. + counts_by_phase: Mutated with this phase's committed row count, on + the failure path as well as the success path. + stale_filter: Selects the slice of ``files`` this phase owns, deleted + once replacement rows are in hand. See ``clear_stale_once``. + """ + batch: list[dict] = [] + count = 0 + stale_cleared = False + + async def clear_stale_once() -> None: + """Delete the slice this phase owns, once, before its first write. + + Deferred to the first batch rather than run before the stream opens. + The known ENCODE failures -- a 504 while the portal assembles the + response, an exhausted download budget -- strike before any row + arrives, and clearing up front would delete the slice and then have + nothing to put back, serving an empty one until the next successful + sync. Deferring means the previous rows keep being served instead. + A stream that dies *mid*-load still leaves a partial slice; only a + shadow-and-swap would fix that. + """ + nonlocal stale_cleared + if stale_cleared: + return + stale_cleared = True + deleted = await api.db.files.delete_many( + {"submission": "encode", **stale_filter} + ) + logger.info( + "ENCODE %s: cleared %d stale files", label, deleted.deleted_count + ) + + async def commit(pending: list[dict]) -> None: + """Send one batch, discounting it from the tally if it does not land.""" + nonlocal count + await clear_stale_once() + try: + await api.db.files.insert_many(pending) + except Exception: + count -= len(pending) + raise + + try: + # ``aclosing`` because the caller isolates phase failures with a + # broad ``except``, which abandons this generator mid-iteration. + # The stream owns an aiohttp session inside its own ``async with``, + # released only when the generator is finalized -- left to the + # garbage collector that is neither prompt nor guaranteed in a + # long-lived API process, and the annotation fan-out turned one + # stream per sync into N+1. + async with aclosing(rows): + async for row in rows: + doc = transform(row) + if doc is None: + continue + + compression_counts[doc.get("compression_format")] += 1 + annotation_type = ( + doc.get("extra", {}).get("encode", {}).get("annotation_type") + ) + if annotation_type: + annotation_type_counts[annotation_type] += 1 + + batch.append(doc) + count += 1 + + # Insert in batches. The buffer is detached *before* the + # await rather than cleared after it, so an insert that + # raises leaves nothing behind for the flush below to submit + # a second time. Re-submitting is not harmless: the driver + # stamps ``_id`` in place on these dicts before sending, so + # a retry collides on the committed prefix and silently + # drops the batch's uncommitted suffix. + if len(batch) >= BATCH_SIZE: + pending, batch = batch, [] + await commit(pending) + task.progress = f"Inserted {count} ENCODE {label} files..." + logger.info(task.progress) + + # The stream drained without error, so its result is authoritative + # even when it is empty: clear here too, or a type ENCODE stopped + # publishing would keep serving last sync's rows indefinitely. + await clear_stale_once() + + # The clean-path flush: inside the ``try``, so a sink failure with no + # exception in flight fails the phase rather than being swallowed. + if batch: + pending, batch = batch, [] + await commit(pending) + except asyncio.CancelledError: + # Never write on the way out of a cancellation. The decision to stop + # has already been made, and the flush below could not protect the + # unwind anyway: its ``except Exception`` does not catch a + # re-delivered CancelledError, which would replace the original and + # skip the ``counts_by_phase`` record. + count -= len(batch) + batch.clear() + raise + finally: + # Only reachable with rows still buffered when the stream itself died + # mid-flight -- the detach above guarantees they were never + # submitted. They are already counted in the tallies, and the DCC was + # cleared before the load, so discarding them would lose data for no + # gain. + if batch: + try: + await commit(batch) + except Exception: + # Never let the flush replace the exception that caused it. + logger.exception( + "ENCODE %s: failed to commit the final %d-row batch", + label, + len(batch), + ) + counts_by_phase[label] = count + logger.info(f"ENCODE {label} ingest complete: {count} files") + + async def _sync_encode(task: SyncTask) -> None: """ Sync ENCODE data from REST API. Unlike C2M2 ZIP sources, ENCODE data is fetched from the REST API and pre-materialized directly into the files collection. + + Runs one phase for released experiments plus one per configured + ``annotation_type``. Each phase is isolated: a phase that raises is + logged and recorded, and the remaining phases still run, so a transient + failure on one stream does not cost the corpus the others. The task is + still failed at the end if any phase failed -- the alternative would + report a clean sync over a partially loaded collection. + + Isolation covers the destructive half too. Each phase owns a slice of + ``files`` and deletes only that slice, and only once it has rows to put + back, so a phase that fails before delivering any -- the shape a portal + 504 or an exhausted download budget takes -- leaves its previous rows + being served rather than emptying them. A phase that dies partway + through still leaves its own slice partially loaded; nothing short of + loading into a shadow collection and swapping would change that. """ from cfdb.services.encode import ( + annotation_types_from_env, build_encode_dcc_record, + fetch_encode_annotation_metadata, fetch_encode_metadata, + metadata_budget_seconds, + transform_annotation_to_c2m2, transform_to_c2m2, ) @@ -1021,8 +1202,28 @@ async def _sync_encode(task: SyncTask) -> None: task.progress = "Clearing existing ENCODE data..." logger.info(task.progress) + count = 0 + counts_by_phase: dict[str, int] = {} + failures: list[tuple[str, Exception]] = [] + # Tally the derived compression terms. The derivation depends on the + # shape of ENCODE's download URLs, which nobody here controls, so a + # corpus-wide flip (an upstream column rename, a redirect stub) would + # otherwise be invisible behind an unchanged row count. + compression_counts: Counter[str | None] = Counter() + # Same reasoning for annotation_type, and seeded from the configured + # allowlist below so a type that returns nothing is reported as zero + # rather than simply missing from the log. + annotation_type_counts: Counter[str] = Counter() + async with locks.CutoverLock("encode"): - await _clear_dcc_data_async("encode") + # ``files`` is deliberately not cleared here. One corpus-wide delete + # before a fan-out designed to survive a phase failure means a failed + # phase serves an absent slice rather than a stale one -- the + # experiment phase is the ~295k-file bulk and the likeliest to time + # out, and losing it leaves the API serving the ~29k annotation + # documents as the whole corpus. Each phase clears only what it is + # about to reload, immediately before reloading it. + await _clear_dcc_data_async("encode", skip_collections={"files"}) # Step 2: Upsert DCC record task.current_step = "dcc_record" @@ -1036,49 +1237,111 @@ async def _sync_encode(task: SyncTask) -> None: upsert=True, ) - # Step 3: Fetch and transform files from ENCODE API + # Step 3: Fetch and transform files from ENCODE API. The experiment + # stream first, then one per configured annotation type. task.current_step = "fetching" task.progress = "Fetching files from ENCODE API..." logger.info(task.progress) - batch = [] - count = 0 - # Tally the derived compression terms. The derivation depends on the - # shape of ENCODE's download URLs, which nobody here controls, so a - # corpus-wide flip (an upstream column rename, a redirect stub) would - # otherwise be invisible behind an unchanged row count. - compression_counts: Counter[str | None] = Counter() - - async for encode_file in fetch_encode_metadata(): - # Transform to C2M2 format - doc = transform_to_c2m2(encode_file) - if doc is None: - continue + # One deadline for the whole fan-out, resolved here rather than per + # stream. Every phase runs inside the cutover lock held above, which + # gates the read surface, so a per-stream budget would multiply the + # outage by the number of phases and carry the worst case past the + # sync lock's one-hour stale threshold -- admitting a second sync + # that clears the corpus while this one is still writing to it. A + # phase that spends what is left fails as an ordinary TimeoutError, + # so the isolation below still applies and its rows are still + # committed; the phases after it fail immediately rather than + # opening a request they have no time to finish. + deadline = asyncio.get_running_loop().time() + metadata_budget_seconds() + + # Streams are built by a factory rather than constructed up front, so + # a phase that is never reached never opens a request. Each phase + # carries the filter selecting the slice of ``files`` it owns, so it + # replaces exactly what it reloads. The experiment slice is defined + # by absence: an annotation row published with a blank ``Annotation + # type`` produces no such key, so the experiment phase sweeps it and + # its own phase reinserts it -- it is lost only if that phase also + # fails. + phases = [ + ( + "experiment", + partial(fetch_encode_metadata, deadline=deadline), + transform_to_c2m2, + {"extra.encode.annotation_type": {"$exists": False}}, + ) + ] + for annotation_type in annotation_types_from_env(): + # Seeded at zero so a configured type that returns nothing logs + # ": 0" rather than vanishing from the distribution. An absent + # key is what nobody notices; a zero is what everybody does. + annotation_type_counts[annotation_type] = 0 + phases.append( + ( + f"annotation[{annotation_type}]", + partial( + fetch_encode_annotation_metadata, + annotation_type, + deadline=deadline, + ), + transform_annotation_to_c2m2, + {"extra.encode.annotation_type": annotation_type}, + ) + ) - compression_counts[doc.get("compression_format")] += 1 - batch.append(doc) - count += 1 + for label, open_stream, transform, stale_filter in phases: + try: + await _ingest_encode_rows( + task, + label, + open_stream(), + transform, + compression_counts, + annotation_type_counts, + counts_by_phase, + stale_filter, + ) + except Exception as exc: + # Deliberately broad: the point is that no failure mode of + # one stream -- timeout, HTTP error, malformed TSV -- may + # stop the others from being attempted. Narrower than + # BaseException on purpose, so a CancelledError still + # cancels the sync rather than being logged as a phase + # failure and followed by N more phases. + logger.exception( + "ENCODE %s ingest failed; continuing with the remaining " + "phases", + label, + ) + failures.append((label, exc)) + continue - # Insert in batches - if len(batch) >= BATCH_SIZE: - await api.db.files.insert_many(batch) - task.progress = f"Inserted {count} ENCODE files..." - logger.info(task.progress) - batch.clear() - - # Insert remaining batch - if batch: - await api.db.files.insert_many(batch) - logger.info(f"Inserted final batch, total: {count} ENCODE files") + # Summed from the accumulator rather than tracked alongside it, so + # a phase that failed after committing rows still contributes what + # it actually loaded. + count = sum(counts_by_phase.values()) # ENCODE writes straight into the materialized collection and never runs # the materializer, which is the only other creator of files indexes. On # a database where ENCODE is the only DCC synced, that leaves files with - # no indexes at all and makes every accession lookup a full scan. + # no indexes at all and makes every accession lookup a full scan. Run + # unconditionally: whatever a partially failed sync did load still has to + # be servable without a full collection scan. await ensure_indexes(api.db, materialized_files_index_specs()) - task.progress = f"ENCODE sync complete: {count} files" + if failures: + # counts_by_phase carries an entry for every phase that started, + # failed ones included -- they may have committed rows before + # dying -- so the succeeded tally comes from the phase list, not + # from the accumulator's length. + task.progress = ( + f"ENCODE sync incomplete: {count} files from " + f"{len(phases) - len(failures)} of {len(phases)} phases" + ) + else: + task.progress = f"ENCODE sync complete: {count} files" logger.info(task.progress) + logger.info("ENCODE files inserted per phase: %s", counts_by_phase) logger.info( "ENCODE compression_format distribution: %s", { @@ -1086,6 +1349,17 @@ async def _sync_encode(task: SyncTask) -> None: for term, tally in compression_counts.most_common() }, ) + logger.info( + "ENCODE annotation_type distribution: %s", dict(annotation_type_counts) + ) + + if failures: + failed_labels = ", ".join(label for label, _ in failures) + raise RuntimeError( + f"ENCODE sync completed with {len(failures)} failed phase(s) " + f"({failed_labels}); {count} files were loaded from the phases " + "that succeeded" + ) from failures[0][1] async def _materialize_files(submission: str) -> None: @@ -1119,14 +1393,26 @@ async def _materialize_files(submission: str) -> None: raise -async def _clear_dcc_data_async(submission: str) -> None: - """Clear DCC data using async Motor client.""" +async def _clear_dcc_data_async( + submission: str, skip_collections: set[str] | None = None +) -> None: + """Clear DCC data using async Motor client. + + Args: + submission: DCC whose documents are removed. + skip_collections: Collections left untouched. The ENCODE sync passes + ``files`` here because it clears that collection one phase-slice + at a time instead; see :func:`_sync_encode`. + """ if api.db is None: raise RuntimeError("Database not initialized") collection_names = await api.db.list_collection_names() + skip = skip_collections or set() for collection_name in collection_names: + if collection_name in skip: + continue try: result = await api.db[collection_name].delete_many( {"submission": submission} From 7160828f80b238ec7e9eda6c7d38f0a36a7291f1 Mon Sep 17 00:00:00 2001 From: Conrad Date: Thu, 13 Aug 2026 14:05:29 -0400 Subject: [PATCH 06/15] feat: Index and expose the ENCODE annotation facets annotation_type is the field the whole annotation corpus is meant to be reached through, organism narrows a result set spanning human and mouse, and assembly narrows one spanning GRCh38, mm10 and hg19. All three needed two things the schema alone does not give them. An index, because the ENCODE sync writes the files collection directly and never reaches the Rust materializer that owns the rest of its keys -- so nothing else would create these, and the flagship filter would be a full scan of a ~300,000 document collection on an unauthenticated endpoint. The core genome_assembly field is indexed here for the same reason, despite the materializer owning it. A database where ENCODE is the only DCC synced never runs the materializer, and genome_assembly is the field the schema publishes and a client narrowing to GRCh38 actually filters on -- indexing only the extra.encode mirror would have left the documented path scanning. Repeating a key the materializer also creates costs nothing, because identical keys derive identical default names. A place in the distinct-values allowlist, because filtering by annotation type is only useful if the vocabulary is discoverable. Otherwise a client has to already know ENCODE's exact spelling, which is the string matching the annotation ingest set out to replace. All three are small closed vocabularies, which is what that allowlist is for -- unlike the accessions, which are excluded from it deliberately because enumerating one would return the whole corpus. --- src/cfdb/api/gql/schema.py | 12 ++++++++++++ src/cfdb/indexes.py | 20 ++++++++++++++++++++ 2 files changed, 32 insertions(+) diff --git a/src/cfdb/api/gql/schema.py b/src/cfdb/api/gql/schema.py index e5eef05..8402a38 100644 --- a/src/cfdb/api/gql/schema.py +++ b/src/cfdb/api/gql/schema.py @@ -78,6 +78,18 @@ def process_errors( "output_type", "status", "data_access_level", + # ENCODE annotation facets. All three are small closed vocabularies + # -- a handful of annotation types, organisms and assemblies -- + # which is what this allowlist is for; unlike accession_id, + # deliberately absent because enumerating it would return the whole + # corpus. Without annotation_type here a client can discover which + # assemblies exist but not which annotation types do, which is the + # facet the annotation ingest exists to expose (issue #94). + # ``assembly`` mirrors the core ``genome_assembly`` above; both are + # written by the ENCODE ingest and either may be filtered on. + "extra.encode.annotation_type", + "extra.encode.organism", + "extra.encode.assembly", } ) diff --git a/src/cfdb/indexes.py b/src/cfdb/indexes.py index 74f534f..5e0cedd 100644 --- a/src/cfdb/indexes.py +++ b/src/cfdb/indexes.py @@ -173,10 +173,30 @@ def materialized_files_index_specs() -> list[IndexSpec]: collection on a public endpoint. Ensuring just the accession keys there costs nothing when the materializer has already created them: identical keys derive identical default names, so the create is a no-op. + + The ``extra.encode`` keys are here for the same reason and are likewise + not the materializer's: they are written only by the ENCODE ingest, and + ``annotation_type`` in particular is the filter the whole annotation + corpus is meant to be reached through (issue #94). Without an index that + is a full scan of ~300k documents on an unauthenticated endpoint. + + ``genome_assembly`` is the exception to "not the materializer's": it is + a core field every DCC populates and the materializer does index it. + It is repeated here because an ENCODE-only database never runs the + materializer, and assembly is the other half of the same acceptance + criterion as organism -- narrowing to GRCh38 is the first move a client + makes. The `extra.encode.assembly` mirror is indexed alongside it since + the ENCODE ingest writes both and either may be filtered on. Repeating + a key the materializer also creates costs nothing: identical keys derive + identical default names, so the create is a no-op. """ return [ IndexSpec("files", [("accession_id", 1)]), IndexSpec("files", [("collections.accession_id", 1)]), + IndexSpec("files", [("extra.encode.annotation_type", 1)]), + IndexSpec("files", [("extra.encode.organism", 1)]), + IndexSpec("files", [("extra.encode.assembly", 1)]), + IndexSpec("files", [("genome_assembly", 1)]), ] From 35e8ea12c6a97da92767376262c686b7bc74213a Mon Sep 17 00:00:00 2001 From: Conrad Date: Thu, 13 Aug 2026 14:06:03 -0400 Subject: [PATCH 07/15] docs: Document the ENCODE annotation mapping The module docstring is the reference for this service and described only half of it once the annotation path landed. It now carries the second metadata URL, the six renamed columns, the seven annotation-only ones, and -- as importantly -- the list of columns the annotation TSV does not publish, which are left unset rather than derived. The supplement gains the same mapping, the allowlist and its configured types with their real dataset and file counts, the two minted format terms and why they are minted, and the annotation output types. Three properties of the fan-out are recorded because a reader would otherwise have to reconstruct each of them from the code. The download budget bounds the whole sync rather than each stream in it, and the reason is the cutover lock the phases run inside and the sync lock's one-hour stale threshold beyond it. Each phase replaces only the slice it reloads, and only once it has rows to put back, so a phase that dies before delivering any leaves its previous rows being served. And the paired-interval fix reaches ENCODE alone, since the format table has one consumer and the other two DCCs never touch it. What the mint costs is recorded alongside what it buys: the incorrect tabix artifacts left orphaned in the cache for the re-typed files, and that bigInteract goes from a degraded index to none at all until the tileset endpoints land. Two details are called out in both the docstring and the supplement. An annotation document carries no subjects, since the TSV names no donor to build one from, and its dataset is built from the accession alone so that the 24 datasets whose files name no biosample stay queryable. The donor traits are the one mapping the tables overstated. Life stage, age and age units land on the biosample, so a row naming no biosample term drops them -- which is exactly the row the annotation transform relaxed its biosample requirement to serve. Nothing was lost against the corpus as last measured, and inventing a biosample to hold them would be worse than dropping them, but the mapping was written as unconditional and it is not. --- ENCODE-SUPPLEMENT.md | 125 +++++++++++++++++++++++++++++++++--- src/cfdb/services/encode.py | 77 ++++++++++++++++++++-- 2 files changed, 190 insertions(+), 12 deletions(-) diff --git a/ENCODE-SUPPLEMENT.md b/ENCODE-SUPPLEMENT.md index ce87a61..e309fe7 100644 --- a/ENCODE-SUPPLEMENT.md +++ b/ENCODE-SUPPLEMENT.md @@ -1,14 +1,30 @@ # ENCODE Metadata Mapping -Field mapping from the ENCODE metadata TSV to the CFDB data model. ENCODE does not use C2M2 — all data is fetched from the ENCODE REST API and pre-materialized directly into the `files` collection, bypassing the C2M2 load and Rust materializer steps. +Field mapping from the ENCODE metadata TSVs to the CFDB data model. ENCODE does not use C2M2 — all data is fetched from the ENCODE REST API and pre-materialized directly into the `files` collection, bypassing the C2M2 load and Rust materializer steps. + +Two TSVs are ingested: released **experiments** and released **annotations** of configured types. They share most of their mapping; the sections below describe the experiment TSV, and [Annotation Mapping](#annotation-mapping) states every way an annotation row differs. ## Data Source | Source | URL | What It Provides | |--------|-----|------------------| -| ENCODE metadata TSV | `GET /metadata/?type=Experiment&status=released` | Streaming TSV of all released experiment files (~700k rows, hundreds of MB) | +| ENCODE experiment metadata TSV | `GET /metadata/?type=Experiment&status=released` | Streaming TSV of all released experiment files (~700k rows, hundreds of MB), 59 columns | +| ENCODE annotation metadata TSV | `GET /metadata/?type=Annotation&status=released&annotation_type=` | Streaming TSV of all released files of one annotation type, 32 columns. Requested once per configured type | + +The TSV is streamed line-by-line to keep memory usage constant. Each row represents one file with its experiment/dataset, biosample, library, and donor metadata denormalized inline. + +### Which annotation types are ingested + +`ENCODE_ANNOTATION_TYPES` — comma-separated `annotation_type` values. Unset yields the default allowlist below; set to an empty value to disable annotation ingest entirely, which logs a warning since `ENCODE_METADATA_TIMEOUT_SECONDS` reads an empty value the other way, as "unset, use the default". Entries are trimmed, blanks dropped, and repeats collapsed — one ingest phase runs per distinct entry, and since ENCODE files are written with `insert_many` into a collection with no unique key, a duplicated token would otherwise load every file of that type twice. + +| `annotation_type` | Datasets | Files | Formats | Assemblies | +|---|---|---|---|---| +| `candidate Cis-Regulatory Elements` | 6,230 | 12,448 | `bed bed9+`, `bigBed bed9+`, `bed bed3+`, `bigBed bed3+`, `bigBed bed9` | GRCh38, mm10, hg19 | +| `element gene regulatory interaction predictions` | 1,543 | 16,692 | `bed bed3+`, `bedpe`, `bigInteract`, `bed bed3` | GRCh38 | -The TSV is streamed line-by-line to keep memory usage constant. Each row represents one file with its experiment, biosample, library, and donor metadata denormalized inline. +This is an allowlist rather than a filter applied after the fact. ENCODE publishes 580,910 annotation datasets, 86% of them footprints; ingesting the type space wholesale and pruning later would be a corpus-scale mistake to undo. + +One request per type, not one request carrying repeated `annotation_type` parameters: a failure or timeout on one type then costs only that type, and each response stays small enough for the portal to assemble without a gateway timeout. ## Ontology Mappings @@ -28,11 +44,25 @@ ENCODE uses human-readable strings for file formats, assay types, output types, | `broadPeak` | `format:3614` | BroadPeak | | `bigWig` | `format:3006` | bigWig | | `bigBed` | `format:3004` | bigBed | +| `bedpe` | `cfdb:bedpe` | bedpe | +| `bigInteract` | `cfdb:biginteract` | bigInteract | | `vcf` | `format:3016` | VCF | | `gtf` | `format:2306` | GTF | | `tsv` | `format:3475` | TSV | | `hdf5` | `format:3590` | HDF5 | +#### Minted (non-EDAM) format terms + +`cfdb:` marks a term minted here because EDAM has none (verified against OLS4). The alternative — aliasing an unrepresented format onto the nearest EDAM term — is what produced the starch/BED conflation behind #69 and #72: the format becomes indistinguishable from the one it was aliased to, and the processor claiming that term picks it up and mangles it. + +`bedpe` and `bigInteract` both pair two loci per record. Routed into the BED tabix pipeline they would be sorted and indexed by the first locus alone, committing a cached artifact that looks successful and is wrong — and the byte-sniff guard from #71 does not catch it, since both are plaintext or gzip. The distinct **name** is what does the work: processor lookup keys on `file_format.name`, and neither name is in any processor's `supported_formats`, so `GET /data/...` streams the raw upstream file rather than a mangled index. Tileset endpoints that understand these formats are planned separately. + +**`bigInteract` loses a working index in the meantime.** The two formats were not equally broken before this change. A `bedpe` routed through tabix produced an index that was simply wrong — the second mate of every record was unindexed and invisible. A `bigInteract` routed through `bigBedToBed` produced one that was *range-coherent*: the leading three columns are a genuine interval, so the index answered range queries correctly while silently degrading each interaction to a single locus. Re-typing both means `GET /index/...` now returns 404 for either, so `bigInteract` files that previously returned a usable-if-degraded index return nothing at all, and that applies to `bigInteract` already in the experiment corpus as well as to newly ingested annotation files. The tradeoff is accepted deliberately: a 404 states the truth, where the degraded index quietly answered a question the caller did not ask. The tileset work is what restores the capability. + +This applies to `.bedpe` already in the ENCODE experiment corpus, not only to annotation files — but it reaches no further than ENCODE. `FILE_FORMAT_TO_EDAM` and `get_file_format` have exactly one consumer, the ENCODE transform; 4DN and HuBMAP take `file_format` from their upstream C2M2 datapackage or portal API and never consult the table. A 4DN `.bedpe` whose upstream declares BED therefore still carries `file_format.name == "BED"`, is still claimed by `TabixIntervalProcessor`, and still gets an index built from its first mate. Closing that means routing incoming formats from every DCC through the same table, or refusing a `.bedpe` filename at the processor regardless of declared format; both are follow-up work. + +**Cache artifacts left behind.** Files of these two formats that were indexed before the re-typing still have their incorrect `.tbi` artifacts in the workflow cache. Nothing reads them any more — `lookup_for` returns `None` and the router bails before probing the cache — and nothing purges them either, so they are unreachable storage cost until someone sweeps them. They also make a latent cache-key hazard concrete: `cache_key` identifies a processor only by its `processor_version`, not by which processor it is, so a future paired-interval processor sharing a version number with `TabixIntervalProcessor` would derive the same key and read those stale artifacts back as cache hits. A processor identity has to be folded into the key before that processor lands; see the warning on `cfdb.workflows.keys.cache_key`. + ### Output Type -> EDAM Data 53 mappings from ENCODE `Output type` strings to EDAM data CV terms. @@ -47,6 +77,13 @@ ENCODE uses human-readable strings for file formats, assay types, output types, | `methylation state at CpG` | `data:1772` | Methylation data | | `variant calls` | `data:3498` | Sequence variations | | `contact matrix` | `data:2082` | Matrix | +| `candidate Cis-Regulatory Elements` | `data:1255` | Sequence features | +| `elements reference` | `data:1255` | Sequence features | +| `element gene links` | `data:0006` | Data | +| `thresholded element gene links` | `data:0006` | Data | +| `thresholded links` | `data:0006` | Data | + +The last five are the complete `Output type` domain of the two ingested annotation types, verified against the live TSVs. Without them every annotation file would carry `data_type: null`. The element/gene link types resolve to the generic `data:0006` because EDAM has no term for a predicted regulatory relationship between two loci — the pre-existing `chromatin interactions` entry already made that call the same way. ### Assay -> OBI @@ -150,6 +187,9 @@ Experiment-level fields stored on `collection.extra.encode` (`EnrichedEncodeColl | `extra.encode.platform` | `Platform` | | `extra.encode.dbxrefs` | `dbxrefs` | | `extra.encode.rbns_protein_concentration` | `RBNS protein concentration` | +| `extra.encode.annotation_type` | `Annotation type` (annotation TSV only) | +| `extra.encode.software_used` | `Software used` (annotation TSV only) | +| `extra.encode.encyclopedia_version` | `Encyclopedia Version` (annotation TSV only) | ### Biosample @@ -161,6 +201,9 @@ One biosample per file, nested inside the collection: | `anatomy` | `Biosample term id` + `Biosample term name` | `{id, name}` object | | `subjects[]` | `Donor(s)` | Same subjects as collection | | `extra.encode.biosample_type` | `Biosample type` | e.g., `"primary cell"`, `"tissue"`, `"cell line"` | +| `extra.encode.life_stage` | `Life stage` | Annotation TSV only. e.g., `"embryonic"`, `"adult"`, `"young adult"`, `"unknown"` | +| `extra.encode.age` | `Age` | Annotation TSV only. Kept as the upstream string — the released corpus contains `"2-4"` and `"unknown"` alongside decimals, and distinguishes `"10.5"` from `"10.50"` | +| `extra.encode.age_units` | `Age units` | Annotation TSV only. `year` / `month` / `week` / `day`; blank when `age` is absent or a sentinel | | `extra.encode.biosample_treatments` | `Biosample treatments` | Treatment details | | `extra.encode.biosample_treatments_amount` | `Biosample treatments amount` | Dosage | | `extra.encode.biosample_treatments_duration` | `Biosample treatments duration` | Duration | @@ -215,8 +258,10 @@ All stored on `file.extra.encode` (`EnrichedEncodeFile`). Every field is `Option |------------|-------------------| | `extra.encode.genome_annotation` | `Genome annotation` | | `extra.encode.controlled_by` | `Controlled by` | -| `extra.encode.s3_uri` | `s3_uri` | +| `extra.encode.s3_uri` | `s3_uri` (annotation TSV: `S3 URL`) | | `extra.encode.azure_url` | `Azure URL` | +| `extra.encode.organism` | `Biosample organism` (annotation TSV: `Organism`) | +| `extra.encode.annotation_type` | `Annotation type` (annotation TSV only) | ### Analysis Metadata @@ -233,6 +278,52 @@ All stored on `file.extra.encode` (`EnrichedEncodeFile`). Every field is `Option | `extra.encode.audit_not_compliant` | `Audit NOT_COMPLIANT` | | `extra.encode.audit_error` | `Audit ERROR` | +## Annotation Mapping + +Everything above applies to an annotation row too, except as stated here. `transform_annotation_to_c2m2` renames the annotation TSV's columns to their experiment equivalents, runs the shared transformation, then applies the annotation-only columns — so one mapping serves both TSVs. + +### Renamed columns + +Applied via `ANNOTATION_COLUMN_ALIASES` in `encode.py`. Verified against the live headers of both ingested types (both 32 columns, identical to each other). Every other shared column already agrees by name. + +| Annotation TSV | Experiment TSV | Lands on | +|---|---|---| +| `Dataset accession` | `Experiment accession` | `collections[].local_id`, `accession_id`, `name`, `persistent_id` | +| `Assay term name` | `Assay` | `assay_type`, `collections[].experiment_type` | +| `Assembly` | `File assembly` | `genome_assembly` | +| `Dataset date released` | `Experiment date released` | `creation_time` | +| `S3 URL` | `s3_uri` | `extra.encode.s3_uri` | +| `Organism` | `Biosample organism` | `extra.encode.organism`, `subjects[].taxonomy` | + +### Annotation-only columns + +| Annotation TSV | Lands on | +|---|---| +| `Annotation type` | `extra.encode.annotation_type` **and** `collections[].extra.encode.annotation_type` | +| `Software used` | `collections[].extra.encode.software_used` | +| `Encyclopedia Version` | `collections[].extra.encode.encyclopedia_version` | +| `Targets` | `collections[].experiment_target` | +| `Life stage` | `…biosamples[].extra.encode.life_stage`, when the row names a biosample | +| `Age` | `…biosamples[].extra.encode.age`, when the row names a biosample | +| `Age units` | `…biosamples[].extra.encode.age_units`, when the row names a biosample | + +`annotation_type` is stored on the file as well as the dataset. The dataset is where the property belongs, but filtering *files* by it is the actual use case — "give me the cCRE files" — and routing that through a collection subdocument on every query is not worth avoiding one duplicated string. + +`Targets` reuses the scalar `experiment_target` rather than adding a parallel list field; the TSV hands over a string either way. Note that ENCODE does not order it — both `"CTCF-human, H3K4me3-human"` and `"H3K4me3-human, CTCF-human"` occur in the released corpus — so an equality filter on the whole value matches one permutation only. + +`Life stage` / `Age` / `Age units` go to the biosample, not to `Subject.age_at_sampling`. They are biosample-scoped in ENCODE's own model (resolved from the donor upstream), which is why the annotation TSV publishes them despite having no `Donor(s)` column — and `age_at_sampling` is a float in years, which could represent neither the `"2-4"` ranges nor the `"unknown"` sentinels the corpus contains. + +The biosample is the destination, so a row that names no `Biosample term name` has nowhere to put them and they are dropped. That is exactly the row the annotation transform relaxed `require_biosample` for, so the caveat matters more here than on the experiment path: an annotation row with donor traits but no biosample loses them. Nothing was lost against the corpus as last checked — of the biosample-less rows in the two configured annotation types, none published any of the three — and inventing a biosample to hold them would be worse than dropping them. `test_transform_annotation_to_c2m2_should_drop_donor_traits_with_no_biosample` pins the behavior. + +### Experiment-only columns + +Absent from the annotation TSV, and therefore **unset** on annotation documents rather than derived: all eight `Library *`, all six `Biosample genetic modifications *`, `Biosample treatments *`, `Biological`/`Technical replicate(s)`, `Donor(s)`, `Experiment target`, `File analysis title`/`status`, `File format type`, `Genome annotation`, `Index of`, `Read length`, `Mapped read length`, `Run type`, `Paired end`, `Paired with`, `Platform`, `RBNS protein concentration`, `Controlled by`. + +Two consequences worth naming: + +- **No subjects.** With no `Donor(s)` column there is nothing to key a `Subject` on, so annotation documents carry `collections[].subjects == []` and no donor is fabricated. The organism that would have reached `subjects[].taxonomy` is on `extra.encode.organism` instead — which is the only way the organism of a multi-organism result set (the released cCREs span *Homo sapiens* and *Mus musculus*) is recoverable without inspecting filenames. +- **The dataset does not require a biosample.** Unlike the experiment path, the annotation collection is built from `Dataset accession` alone. 48 released cCRE files name no biosample term; gating on one would leave 24 dataset accessions unqueryable. Its `persistent_id` points at `/annotations/`, not `/experiments/`. + ## Sync Flow ENCODE sync bypasses the C2M2 ZIP pipeline entirely. Files are pre-materialized during ingest. @@ -243,19 +334,37 @@ ENCODE sync bypasses the C2M2 ZIP pipeline entirely. Files are pre-materialized ### Data Flow +`_sync_encode` runs one phase for released experiments plus one per configured `annotation_type`: + ```text -fetch_encode_metadata() # Streaming TSV from ENCODE API - │ # Yields one dict per row +_sync_encode() + ├─ clear ENCODE data + upsert the DCC record (inside the cutover lock) + │ + ├─ phase "experiment" + │ fetch_encode_metadata() # Streaming TSV from ENCODE API + │ └─> transform_to_c2m2(row) # Yields one dict per row + │ + ├─ phase "annotation[]" × one per configured type + │ fetch_encode_annotation_metadata(type) + │ └─> transform_annotation_to_c2m2(row) + │ └─ rename columns, then the shared transformation │ - └─> transform_to_c2m2(row) # Per-row transformation + └─ each transformation: ├─ Map File format -> EDAM # ontology_mappings.get_file_format() ├─ Derive compression -> EDAM # from the download URL's suffix ├─ Map Output type -> EDAM # ontology_mappings.get_data_type() ├─ Map Assay -> OBI # ontology_mappings.get_assay_type() ├─ Map Organism -> NCBI # ontology_mappings.get_taxonomy() ├─ Build collection + biosample + subjects inline - ├─ Build extra.encode dict (21 file fields) └─ Insert into files collection (batches of 1000) ``` +**Phase isolation.** A phase that raises is logged and recorded, and the remaining phases still run — a transient failure on one stream does not cost the corpus the others. A stream that dies *mid-flight* (the shape an `asyncio.TimeoutError` against the metadata budget takes) has its trailing partial batch committed rather than discarded, and still reports the rows it loaded, so the per-phase counts, the summary tallies and the collection all agree. The indexes are ensured unconditionally afterwards, so whatever did load is servable without a full collection scan. The sync then fails, naming the phases that broke: reporting a clean sync over a partially loaded collection would be worse than a visible failure. + +**Each phase replaces only its own slice.** `files` is not cleared corpus-wide before the fan-out. Each phase owns a slice — experiments are the documents with no `extra.encode.annotation_type`, each annotation phase the documents carrying its type — and deletes that slice only once it has replacement rows in hand. A phase that fails before delivering any row, the shape a portal 504 or an exhausted download budget takes, therefore leaves its previous rows being served instead of emptying them; clearing up front meant a failed experiment phase left the API serving the ~29k annotation documents as the entire corpus. A stream that drains cleanly with no rows is the opposite case and does clear, so a type ENCODE stopped publishing stops being served. A phase that dies partway through still leaves its own slice partially loaded — only loading into a shadow collection and swapping on success would close that, and it is not part of this change. + +**One budget for the fan-out.** `ENCODE_METADATA_TIMEOUT_SECONDS` (default 3600) bounds the whole sync, not each stream within it. The budget is resolved once and every phase shares a single deadline, each receiving whatever remains; a phase that finds the budget spent fails immediately rather than opening a request it has no time to finish. Per-stream budgets would have multiplied the ceiling by the number of phases — three by default, and unbounded as the allowlist grows — while the whole fan-out sits inside the cutover lock that gates the read surface. That matters beyond availability: the sync lock treats itself as abandoned after one hour (`STALE_LOCK_THRESHOLD`) with nothing refreshing `started_at`, so a run that outlives the threshold admits a second sync, which clears the ENCODE corpus while the first is still inserting into it. + +**Queryability.** `extra.encode.annotation_type` and `extra.encode.organism` are indexed by `materialized_files_index_specs()` — the ENCODE sync writes `files` directly and never reaches the Rust materializer that owns the rest, so nothing else would create them — and both are in `ALLOWED_DISTINCT_FIELDS`, so a client can enumerate the available annotation types rather than having to know ENCODE's exact spelling in advance. + No post-ingest enrichment pass. All metadata is captured during the initial TSV transformation. diff --git a/src/cfdb/services/encode.py b/src/cfdb/services/encode.py index 5a4c89c..f591aca 100644 --- a/src/cfdb/services/encode.py +++ b/src/cfdb/services/encode.py @@ -1,11 +1,15 @@ """ENCODE metadata TSV client and CFDB transformation service. -Fetches the released-experiment metadata TSV from ENCODE and transforms -each row into a CFDB file document. +Fetches the released-experiment and released-annotation metadata TSVs from +ENCODE and transforms each row into a CFDB file document. -Metadata URL ------------- +Metadata URLs +------------- https://www.encodeproject.org/metadata/?type=Experiment&status=released +https://www.encodeproject.org/metadata/?type=Annotation&status=released&annotation_type= + +The annotation URL is requested once per configured ``annotation_type`` +(``ENCODE_ANNOTATION_TYPES``; see :func:`annotation_types_from_env`). Field Mapping (ENCODE TSV → CFDB) ---------------------------------- @@ -43,6 +47,7 @@ Controlled by → extra.encode.controlled_by s3_uri → extra.encode.s3_uri Azure URL → extra.encode.azure_url +Biosample organism → extra.encode.organism File analysis title → extra.encode.file_analysis_title File analysis status → extra.encode.file_analysis_status Audit WARNING → extra.encode.audit_warning @@ -100,6 +105,63 @@ dcc.dcc_description, dcc.contact_email, dcc.contact_name, dcc.dcc_url, dcc.project_id_namespace, dcc.project_local_id + + +Annotation TSV +-------------- + +The Annotation TSV publishes 32 columns to the Experiment TSV's 59, and the +overlap is partial. Everything above applies to an annotation row too, except +as stated here. + +Renamed columns (annotation name → the experiment name the mapping uses). +Applied by ``transform_annotation_to_c2m2`` via ANNOTATION_COLUMN_ALIASES +before the shared transformation, so the table above covers both TSVs: + +Dataset accession → Experiment accession +Assay term name → Assay +Assembly → File assembly +Dataset date released → Experiment date released +S3 URL → s3_uri +Organism → Biosample organism + +Annotation-only columns: + +Annotation type → extra.encode.annotation_type, and + collections[].extra.encode.annotation_type +Software used → collections[].extra.encode.software_used +Encyclopedia Version → collections[].extra.encode.encyclopedia_version +Targets → collections[].experiment_target (reuses the + scalar; ENCODE does not order the value, so + an equality filter matches one permutation) +Life stage → …biosamples[].extra.encode.life_stage +Age → …biosamples[].extra.encode.age +Age units → …biosamples[].extra.encode.age_units + +The three donor traits land on the biosample, so a row naming no +``Biosample term name`` -- the row the annotation transform relaxed +``require_biosample`` for -- has nowhere to put them and drops them. +No biosample-less row in the configured annotation types publishes any +of the three today, and inventing a biosample to hold them would be +worse than dropping them. + +Columns the Annotation TSV does not publish, left unset rather than derived: +all eight ``Library *``, all six ``Biosample genetic modifications *``, +``Biosample treatments *``, ``Biological/Technical replicate(s)``, +``Donor(s)``, ``Experiment target``, ``File analysis title``/``status``, +``File format type``, ``Genome annotation``, ``Index of``, ``Read length``, +``Mapped read length``, ``Run type``, ``Paired end``, ``Paired with``, +``Platform``, ``RBNS protein concentration``, ``Controlled by``. + +Two consequences worth naming: + +* No ``Donor(s)`` column means no subjects are built, so an annotation + document has ``collections[].subjects == []`` and no donor is invented. + The organism it would have carried is on ``extra.encode.organism``. +* The dataset collection is built from ``Dataset accession`` alone, without + requiring a biosample term -- 48 released cCRE files name no biosample, + and gating on one would make 24 dataset accessions unqueryable. Its + ``persistent_id`` points at ``/annotations/``, not ``/experiments/``. """ import asyncio @@ -750,6 +812,13 @@ def _transform_row( # having no Donor(s) column. Kept as the upstream strings; see # EnrichedEncodeBiosample for why they are not parsed into # Subject.age_at_sampling. + # + # Note these are attached only under ``if build_biosample`` below, + # so a row naming no biosample term parses them and drops them. + # Deliberate: the biosample is their destination and inventing one + # to hold them would be worse. No biosample-less row in the + # configured annotation types published any of the three when the + # released corpus was last checked. _add_extra(biosample_extra, "life_stage", row.get("Life stage")) _add_extra(biosample_extra, "age", row.get("Age")) _add_extra(biosample_extra, "age_units", row.get("Age units")) From e691ac34a1df8d22aa98b46a145f6c50aef06ca3 Mon Sep 17 00:00:00 2001 From: Conrad Date: Thu, 13 Aug 2026 14:06:22 -0400 Subject: [PATCH 08/15] test: Cover the minted formats and annotation data terms The minted prefix is the mechanism that stops an unrepresented format being aliased onto a term meaning something else, and nothing tied the exported constant to the table it governs. Three property tests do that, each asserting through get_file_format rather than against the table directly so the property covers what production reads: every id is either an EDAM term or a minted one, a minted id is derived from its own key, and a minted name is shared with no EDAM entry and claimed by no processor. That last one is the load-bearing case -- since routing keys on the name, a minted id paired with the name "BED" would drop the file straight back into the pipeline the minting exists to keep it out of. Each property's example budget is derived from the table it samples rather than left at the default, so a closed domain is exhausted exactly and grows with the table. The routing assertion now goes through a registry wired the way the API wires it, rather than two hand-picked classes, so it tracks what is actually served. A positive control pins that plain BED still reaches the tabix processor, since "no processor claims it" would also hold if lookup were simply broken. The annotation output types are asserted against their exact terms. The previous test only checked for a non-null data term, so all five could have pointed at the wrong one and passed. --- tests/test_ontology_mappings.py | 252 +++++++++++++++++++++++++++++++- 1 file changed, 245 insertions(+), 7 deletions(-) diff --git a/tests/test_ontology_mappings.py b/tests/test_ontology_mappings.py index 2835153..18abcc8 100644 --- a/tests/test_ontology_mappings.py +++ b/tests/test_ontology_mappings.py @@ -2,10 +2,29 @@ from __future__ import annotations -from hypothesis import given +import pytest +from hypothesis import given, settings from hypothesis import strategies as st -from cfdb.services.ontology_mappings import get_file_format +from cfdb.services.ontology_mappings import ( + FILE_FORMAT_TO_EDAM, + MINTED_FORMAT_PREFIX, + get_data_type, + get_file_format, +) +from cfdb.workflows.processors.bam import BamIndexProcessor +from cfdb.workflows.processors.registry import default_registry +from cfdb.workflows.processors.tabix import TabixIntervalProcessor + +#: The minted subset of the format table. Extracted so the property tests +#: below share one definition and can size their example budgets from it -- +#: these domains are small closed tables, so exhausting them is both cheaper +#: and stricter than the default budget. +MINTED_ENTRIES = [ + (key, value) + for key, value in FILE_FORMAT_TO_EDAM.items() + if value["id"].startswith(MINTED_FORMAT_PREFIX) +] def test_get_file_format_with_simple_format(): @@ -113,19 +132,196 @@ def test_get_file_format_with_tagalign_case_insensitive(): assert result == {"id": "format:3003", "name": "BED"} -def test_get_file_format_with_biginteract_format(): - """Test bigInteract format maps to bigBed. +def test_get_file_format_should_return_a_minted_term_for_biginteract(): + """Test bigInteract resolves to its own term rather than to bigBed. Given: - The format string "bigInteract" (a bigBed variant). + The format string "bigInteract", a bigBed variant whose trailing + columns encode interaction endpoints. When: get_file_format is called. Then: - It should return the EDAM bigBed term. + It should return a minted term distinct from the EDAM bigBed term, + so the interaction is not silently served as a plain interval. """ + # Act result = get_file_format("bigInteract") - assert result == {"id": "format:3004", "name": "bigBed"} + # Assert + assert result == {"id": "cfdb:biginteract", "name": "bigInteract"} + assert result != get_file_format("bigBed") + + +def test_get_file_format_should_return_a_minted_term_for_bedpe(): + """Test bedpe resolves to its own term rather than to BED. + + Given: + The format string "bedpe", whose columns are chrom1/start1/end1/ + chrom2/start2/end2 rather than a single interval. + When: + get_file_format is called. + Then: + It should return a minted term distinct from the EDAM BED term. + """ + # Act + result = get_file_format("bedpe") + + # Assert + assert result == {"id": "cfdb:bedpe", "name": "bedpe"} + assert result != get_file_format("bed") + + +def _wired_registry(): + """Build the registry the API serves from. + + Mirrors the registrations in ``cfdb.api.main``'s lifespan, so an + assertion about what is claimed holds against the real deployment + rather than a hand-picked pair of classes. + """ + registry = default_registry() + registry.register(BamIndexProcessor()) + registry.register(TabixIntervalProcessor()) + return registry + + +@pytest.mark.parametrize("encode_format", ["bedpe", "bigInteract"]) +def test_get_file_format_should_keep_paired_formats_out_of_the_tabix_pipeline( + encode_format, +): + """Test the paired-interval formats are claimed by no processor. + + Given: + A format whose records pair two loci, which the BED tabix pipeline + would index by the first locus alone -- committing an artifact that + looks successful and is wrong -- and the registry the API wires. + When: + A file document carrying that format is looked up. + Then: + No processor should claim it, so a request streams the raw + upstream file instead of a mangled index. + """ + # Arrange + file_meta = {"file_format": get_file_format(encode_format)} + + # Act + processor = _wired_registry().lookup_for(file_meta) + + # Assert + assert processor is None + + +def test_get_file_format_should_still_route_plain_bed_to_the_tabix_pipeline(): + """Test the minting did not cost the formats that were routed correctly. + + A positive control for the assertion above: "no processor claims it" + would also hold if the registry were empty or lookup were broken. + + Given: + A plain BED format and the registry the API wires. + When: + A file document carrying it is looked up. + Then: + The tabix interval processor should claim it. + """ + # Arrange + file_meta = {"file_format": get_file_format("bed")} + + # Act + processor = _wired_registry().lookup_for(file_meta) + + # Assert + assert isinstance(processor, TabixIntervalProcessor) + + +@settings(max_examples=len(MINTED_ENTRIES)) +@given(entry=st.sampled_from(MINTED_ENTRIES)) +def test_get_file_format_should_derive_a_minted_id_from_its_key(entry): + """Test a minted term's id matches the format it is keyed under. + + A typo'd mint -- "cfdb:biginterract" -- is invisible to every other + assertion, since nothing else compares the id to anything. Asserted + through the accessor rather than against the table directly, so the + property covers what production reads. + + Given: + Any format whose table entry carries a minted id. + When: + get_file_format is called with its key. + Then: + The returned id should be the prefix followed by the key. + """ + # Arrange + key, _ = entry + + # Act + term = get_file_format(key) + + # Assert + assert term["id"] == f"{MINTED_FORMAT_PREFIX}{key}" + + +@settings(max_examples=len(FILE_FORMAT_TO_EDAM)) +@given(entry=st.sampled_from(sorted(FILE_FORMAT_TO_EDAM.items()))) +def test_get_file_format_should_return_only_edam_or_minted_ids(entry): + """Test the accessor admits only two kinds of identifier. + + The minted prefix is the mechanism that keeps an unrepresented format + from being aliased onto a term that means something else. A third id + shape would sit outside it silently. + + Given: + Any format in the table. + When: + get_file_format is called with its key. + Then: + The returned id should be an EDAM format term or a minted token, + and its name should be non-empty. + """ + # Arrange + key, _ = entry + + # Act + term = get_file_format(key) + + # Assert + assert term["id"].startswith("format:") or term["id"].startswith( + MINTED_FORMAT_PREFIX + ) + assert term["name"] + + +@settings(max_examples=len(MINTED_ENTRIES)) +@given(entry=st.sampled_from(MINTED_ENTRIES)) +def test_get_file_format_should_not_let_a_minted_term_borrow_an_edam_name(entry): + """Test a minted term cannot be defeated by reusing an EDAM name. + + Routing keys on the format *name*, not the id, so + ``{"id": "cfdb:x", "name": "BED"}`` would mint an id and still drop + the file into the BED pipeline -- the exact outcome minting exists to + prevent. + + Given: + Any format whose table entry carries a minted id. + When: + get_file_format is called with its key. + Then: + The returned name should be shared with no EDAM entry and claimed + by no processor. + """ + # Arrange + key, _ = entry + edam_names = { + other["name"] + for other in FILE_FORMAT_TO_EDAM.values() + if not other["id"].startswith(MINTED_FORMAT_PREFIX) + } + + # Act + term = get_file_format(key) + + # Assert + assert term["name"] not in edam_names + assert _wired_registry().lookup_for({"file_format": term}) is None def test_get_file_format_with_h5ad_format(): @@ -188,6 +384,48 @@ def test_get_file_format_with_unknown_compound(): assert result is None +@pytest.mark.parametrize( + "output_type, expected", + [ + ( + "candidate Cis-Regulatory Elements", + {"id": "data:1255", "name": "Sequence features"}, + ), + ("elements reference", {"id": "data:1255", "name": "Sequence features"}), + ("element gene links", {"id": "data:0006", "name": "Data"}), + ( + "thresholded element gene links", + {"id": "data:0006", "name": "Data"}, + ), + ("thresholded links", {"id": "data:0006", "name": "Data"}), + ], +) +def test_get_data_type_should_map_every_annotation_output_type(output_type, expected): + """Test the annotation Output type domain resolves to its documented term. + + These five are the complete Output type domain of the two ingested + annotation types, verified against the live TSVs; without them every + annotation file would carry a null data_type. Asserted by exact term + rather than merely non-null, so a copy-paste that pointed cCREs at the + generic Data term fails here. + + Given: + An Output type value published by an ingested annotation type. + When: + get_data_type is called. + Then: + It should return that value's documented CV term. + """ + # Act + result = get_data_type(output_type) + + # Assert + assert result == expected + + +# Unlike the sampled-table properties above, this domain is unbounded, so +# the budget is doing real work rather than exhausting a closed set. +@settings(max_examples=200) @given(st.text()) def test_get_file_format_return_type_invariant(format_string): """Test return type is always None or a dict with id and name keys. From a2877f33ab149444dd9e632d88d783c9080586e9 Mon Sep 17 00:00:00 2001 From: Conrad Date: Thu, 13 Aug 2026 14:06:49 -0400 Subject: [PATCH 09/15] test: Cover the ENCODE annotation fetch and transform The fixture carries the full 32-column annotation header with values from a real released cCRE file, so the tests exercise the shapes the corpus actually contains rather than an idealized row: an empty Assay term name, which every one of the 12,448 cCRE rows has, a multi-valued Targets, and a non-human organism. On the fetch side: the URL is asserted by parsing its query rather than by substring, which would pass against a stray or duplicated parameter, and a type containing URL metacharacters is pinned as unable to smuggle one in. Each failure mode -- HTTP status, timeout, network error -- is checked to name its own stream, which is the entire reason the label was threaded through, and to release its session. That last one caught a real leak: async for does not close the iterator it abandons, so the delegating wrapper left the session-owning inner generator suspended. The shared deadline gets both of its cases. A stream given one bounds its request by the time remaining rather than by a fresh full budget, and a stream given a spent one refuses without opening a request at all -- aiohttp reads a non-positive total as no timeout, so passing the remainder through would make the exhausted case unbounded. Allowlist parsing gets its adversarial cases: all-blank values disable ingest rather than widening it to an unfiltered query, repeats collapse, and a property test pins that no configuration at all can produce a blank entry. An explicitly empty value is pinned to warn, with a negative control that the unset path stays silent, since the warning is what separates a deliberate disable from a templating accident. The default-when-unset test now deletes the variable first -- it previously asserted against whatever the ambient environment held and passed with an override exported. On the transform side: the renames, the annotation-only fields, the absence of the experiment-only ones, and the two behaviors that differ by design -- no subjects without a donor column, and a dataset built from the accession alone. The experiment path gets the pins it was missing: its persistent_id, which was asserted nowhere in the repository despite the URL path becoming a caller-supplied argument, and anatomy, which the diff's guard change touched and no test read. The assembly mirror is asserted once per transform rather than through one parametrized case. The two read different columns, File assembly against Assembly, which a shared parameter hid behind an argument name. One document is validated through FileMetadataModel end to end. The ingest writes plain dicts and the resolver reads them back through the model on every row, so a key the model does not declare is dropped silently -- the filter would exist and match nothing. --- tests/test_encode.py | 1589 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 1578 insertions(+), 11 deletions(-) diff --git a/tests/test_encode.py b/tests/test_encode.py index ff33c31..5c1f08c 100644 --- a/tests/test_encode.py +++ b/tests/test_encode.py @@ -4,20 +4,25 @@ import asyncio import logging +from urllib.parse import parse_qs, urlsplit import aiohttp import pytest -from hypothesis import given, settings +from hypothesis import HealthCheck, assume, given, settings from hypothesis import strategies as st from cfdb.accessions import normalize_accession +from cfdb.models import FileMetadataModel from cfdb.services import encode as encode_module from cfdb.services.encode import ( COMPRESSION_SUFFIX_TO_EDAM, UNCOMPRESSED, UNMAPPABLE_COMPRESSION_SUFFIXES, + annotation_types_from_env, derive_compression_format, + fetch_encode_annotation_metadata, fetch_encode_metadata, + transform_annotation_to_c2m2, transform_to_c2m2, ) @@ -26,6 +31,10 @@ #: the module should fail these tests, not silently follow along. TIMEOUT_ENV = "ENCODE_METADATA_TIMEOUT_SECONDS" +#: The variable bounding which annotation types are ingested. Named here for +#: the same reason as TIMEOUT_ENV. +ANNOTATION_TYPES_ENV = "ENCODE_ANNOTATION_TYPES" + DOWNLOAD_URL = "https://www.encodeproject.org/files/ENCFF123ABC/@@download/{name}" # Every value the derivation is allowed to produce, for closed-domain @@ -59,7 +68,10 @@ class _FakeContent: modelling a stream that dies partway. """ - def __init__(self, lines: list[str], fail_after: int | None = None): + def __init__(self, lines, fail_after: int | None = None): + # Any iterable, not just a list: a body large enough to trip the + # per-50000-row progress log would otherwise have to be + # materialized in memory just to be thrown away line by line. self._lines = lines self._fail_after = fail_after @@ -87,10 +99,10 @@ async def __aexit__(self, *exc): class _StreamingSession: """Fake aiohttp session that serves one streamed TSV body. - Records the kwargs of every ``get`` -- unlike the 4DN fakes, which - discard them -- because the request's timeout is the behavior under - test. ``error`` raises instead of responding, modelling a network - failure before any body arrives. + Records the URL and kwargs of every ``get`` -- unlike the 4DN fakes, + which discard them -- because the request's timeout and the query it + carries are both behaviors under test. ``error`` raises instead of + responding, modelling a network failure before any body arrives. """ def __init__(self, lines=None, status=200, fail_after=None, error=None): @@ -99,14 +111,22 @@ def __init__(self, lines=None, status=200, fail_after=None, error=None): self._fail_after = fail_after self._error = error self.get_kwargs: list[dict] = [] + self.get_urls: list[str] = [] + #: Times ``__aexit__`` ran. The stream owns its session inside an + #: ``async with``, so this is the only observable signal that an + #: abandoned or failed stream released it rather than leaving it to + #: the garbage collector. + self.exit_count = 0 async def __aenter__(self): return self async def __aexit__(self, *exc): + self.exit_count += 1 return False def get(self, url, **kwargs): + self.get_urls.append(url) self.get_kwargs.append(kwargs) if self._error is not None: raise self._error @@ -211,6 +231,70 @@ async def test_fetch_encode_metadata_should_fall_back_to_the_default_when_unset( assert session.get_kwargs[0]["timeout"].total == 3600 +@pytest.mark.asyncio +async def test_fetch_encode_metadata_should_request_only_the_deadline_that_remains( + mocker, monkeypatch +): + """Test a shared deadline bounds the request, not a fresh full budget. + + A sync runs one stream per configured annotation type plus one for + experiments, all inside the cutover lock that gates the read surface. + Granting each its own hour would multiply the outage by the phase count. + + Given: + A deadline half a minute out, against an hour-long budget. + When: + fetch_encode_metadata drains a response. + Then: + It should bound the request by the remaining time, not the budget. + """ + # Arrange + monkeypatch.delenv(TIMEOUT_ENV, raising=False) + session = _StreamingSession(lines=_tsv("ENCFF1\tbed")) + mocker.patch.object( + encode_module.aiohttp, "ClientSession", return_value=session + ) + deadline = asyncio.get_running_loop().time() + 30 + + # Act + [row async for row in fetch_encode_metadata(deadline=deadline)] + + # Assert + assert session.get_kwargs[0]["timeout"].total <= 30 + + +@pytest.mark.asyncio +async def test_fetch_encode_metadata_should_refuse_a_spent_deadline( + mocker, monkeypatch +): + """Test an exhausted budget stops the fetch instead of going unbounded. + + aiohttp reads a non-positive total as "no timeout", so handing it the + remainder of a spent budget would turn the exhausted case into an + unbounded request -- the failure the budget exists to prevent. + + Given: + A deadline that has already passed. + When: + fetch_encode_metadata is drained. + Then: + It should raise TimeoutError without opening a request. + """ + # Arrange + monkeypatch.delenv(TIMEOUT_ENV, raising=False) + session = _StreamingSession(lines=_tsv("ENCFF1\tbed")) + mocker.patch.object( + encode_module.aiohttp, "ClientSession", return_value=session + ) + deadline = asyncio.get_running_loop().time() - 1 + + # Act & assert + with pytest.raises(asyncio.TimeoutError, match="budget was already spent"): + [row async for row in fetch_encode_metadata(deadline=deadline)] + + assert session.get_kwargs == [] + + @pytest.mark.asyncio @pytest.mark.parametrize( "value", ["not-a-number", "0", "-1"], ids=["malformed", "zero", "negative"] @@ -398,6 +482,9 @@ async def test_fetch_encode_metadata_should_report_how_far_it_got_when_it_times_ [row async for row in fetch_encode_metadata()] assert "timed out after 2 rows" in caplog.text + # Names its stream: with several streams per sync, a timeout message + # that does not say which one died is not actionable. + assert "experiment" in caplog.text @pytest.mark.asyncio @@ -1104,11 +1191,13 @@ def test_transform_to_c2m2_should_fold_the_experiment_collection_accession(): def test_transform_to_c2m2_should_build_no_collection_without_a_biosample_term(): """Test that an experiment accession alone yields no collection. - The whole collection block is gated on the biosample term name, so a - row shaped like an ENCODE annotation or reference contributes no - collection and its experiment accession is not queryable anywhere. - Pre-existing behavior, pinned here because the accession field turns it - from a cosmetic gap into a data-completeness question. + On the experiment path the whole collection block is gated on the + biosample term name, so such a row contributes no collection and its + accession is queryable nowhere. Long-standing behavior, pinned here as + the other half of ``require_biosample``: the annotation path passes + False and deliberately does build the collection (see + ...should_build_a_dataset_without_a_biosample), so this test is what + stops that choice from silently leaking onto experiments. Given: A row carrying an Experiment accession but no Biosample term name. @@ -1163,3 +1252,1481 @@ def test_transform_to_c2m2_should_store_the_accession_case_folded(accession, pad # Assert assert doc["accession_id"] == normalize_accession(doc["local_id"]) + + +# --------------------------------------------------------------------------- +# Annotation ingest +# --------------------------------------------------------------------------- + +#: Download URL of the real released cCRE file the annotation fixture is +#: modelled on. +ANNOTATION_DOWNLOAD_URL = ( + "https://www.encodeproject.org/files/ENCFF845ITU/@@download/ENCFF845ITU.bigBed" +) + +#: Text an environment variable can actually hold: ``os.environ`` rejects an +#: embedded NUL outright and cannot encode a lone surrogate. Generating +#: either tests the harness rather than the allowlist parser. +ENV_SAFE_TEXT = st.text( + alphabet=st.characters(exclude_characters="\x00", codec="utf-8"), + max_size=80, +) + + +def _annotation_row(**overrides) -> dict: + """Return a realistic ENCODE annotation metadata TSV row. + + The full 32-column annotation header, verified against the live TSVs of + both ingested types, with values from a released cCRE file -- so the + fixture carries the shapes the corpus actually contains: an empty Assay + term name, a multi-valued Targets, a non-human organism. + """ + row = { + "File accession": "ENCFF845ITU", + "File format": "bigBed bed9+", + "Output type": "candidate Cis-Regulatory Elements", + "Assay term name": "", + "Dataset accession": "ENCSR026KJM", + "Annotation type": "candidate Cis-Regulatory Elements", + "Software used": "", + "Encyclopedia Version": "ENCODE v1", + "Biosample term id": "UBERON:0002048", + "Biosample term name": "lung", + "Biosample type": "tissue", + "Life stage": "postnatal", + "Age": "0", + "Age units": "day", + "Organism": "Mus musculus", + "Targets": "H3K4me3-mouse, CTCF-mouse", + "Dataset date released": "2017-09-12", + "Project": "ENCODE", + "Lab": "Zhiping Weng, UMass", + "md5sum": "5ff392dcde69f8ec512ea381928674d9", + "dbxrefs": "", + "File download URL": ANNOTATION_DOWNLOAD_URL, + "Assembly": "mm10", + "Controlled by": "", + "File Status": "released", + "Derived from": "/files/ENCFF728HFF/", + "S3 URL": "https://encode-public.s3.amazonaws.com/2017/ENCFF845ITU.bigBed", + "Azure URL": "", + "Size": "13743825", + "Audit WARNING": "", + "Audit NOT_COMPLIANT": "", + "Audit ERROR": "", + } + row.update(overrides) + return row + + +def test_annotation_types_from_env_should_return_a_bounded_default_when_unset( + monkeypatch, +): + """Test the allowlist is never implicitly the whole annotation space. + + ENCODE publishes 580,910 annotation datasets, 86% of them footprints. + Defaulting to all of them would be a corpus-scale mistake to undo, so + the default has to be an explicit, small set -- asserted by equality, + not by "non-empty and not footprints", which would also pass if a third + type were quietly added to the default. + + Given: + No ENCODE_ANNOTATION_TYPES in the environment. + When: + annotation_types_from_env is called. + Then: + It should return exactly the two documented default types. + """ + # Arrange + monkeypatch.delenv(ANNOTATION_TYPES_ENV, raising=False) + + # Act + types = annotation_types_from_env() + + # Assert + assert types == ( + "candidate Cis-Regulatory Elements", + "element gene regulatory interaction predictions", + ) + + +def test_annotation_types_from_env_should_honor_an_override(monkeypatch): + """Test which types are ingested is configuration, not a code change. + + Given: + ENCODE_ANNOTATION_TYPES naming two types. + When: + annotation_types_from_env is called. + Then: + It should return exactly those two, in order. + """ + # Arrange + monkeypatch.setenv(ANNOTATION_TYPES_ENV, "chromatin state,footprints") + + # Act + types = annotation_types_from_env() + + # Assert + assert types == ("chromatin state", "footprints") + + +def test_annotation_types_from_env_should_strip_padding_and_drop_blanks(monkeypatch): + """Test a human-written list is read the way it was meant. + + Given: + An override written with spaces after the commas and a trailing + comma. + When: + annotation_types_from_env is called. + Then: + It should return the trimmed values with no empty entry. + """ + # Arrange + monkeypatch.setenv(ANNOTATION_TYPES_ENV, " chromatin state , footprints ,") + + # Act + types = annotation_types_from_env() + + # Assert + assert types == ("chromatin state", "footprints") + + +def test_annotation_types_from_env_should_disable_ingest_when_set_empty(monkeypatch): + """Test an operator can turn the annotation path off without a deploy. + + Given: + ENCODE_ANNOTATION_TYPES set to an empty value -- distinct from + unset, which yields the default allowlist. + When: + annotation_types_from_env is called. + Then: + It should return no types at all. + """ + # Arrange + monkeypatch.setenv(ANNOTATION_TYPES_ENV, "") + + # Act + types = annotation_types_from_env() + + # Assert + assert types == () + + +def test_annotation_types_from_env_should_warn_when_set_empty(monkeypatch, caplog): + """Test an accidentally empty allowlist is visible rather than inferable. + + The neighbouring timeout variable reads an empty value as "unset, use + the default" and this one reads it as "ingest nothing" -- a difference + an operator has no reason to expect. Empty values arrive by accident + routinely, from an unset CloudFormation parameter to a docker-compose + expansion, and the only other trace is an absence in the per-phase log. + + Given: + ENCODE_ANNOTATION_TYPES set to an empty value. + When: + annotation_types_from_env is called. + Then: + It should warn, naming the variable and the disabled ingest. + """ + # Arrange + monkeypatch.setenv(ANNOTATION_TYPES_ENV, "") + + # Act + with caplog.at_level(logging.WARNING, logger=encode_module.logger.name): + annotation_types_from_env() + + # Assert + assert ANNOTATION_TYPES_ENV in caplog.text + assert "annotation ingest is disabled" in caplog.text + + +def test_annotation_types_from_env_should_not_warn_when_unset(monkeypatch, caplog): + """Test the default allowlist is not reported as a disabled ingest. + + Given: + No ENCODE_ANNOTATION_TYPES in the environment. + When: + annotation_types_from_env is called. + Then: + It should emit no warning. + """ + # Arrange + monkeypatch.delenv(ANNOTATION_TYPES_ENV, raising=False) + + # Act + with caplog.at_level(logging.WARNING, logger=encode_module.logger.name): + annotation_types_from_env() + + # Assert + assert caplog.text == "" + + +@pytest.mark.asyncio +async def test_fetch_encode_annotation_metadata_should_query_the_requested_type( + mocker, +): + """Test the request selects annotations of one type, not experiments. + + Given: + A streamed metadata response and an annotation type containing the + spaces and capitals ENCODE's vocabulary actually uses. + When: + fetch_encode_annotation_metadata drains it. + Then: + It should have requested released Annotations with the type + URL-encoded rather than interpolated raw, and carry no other + parameter. + """ + # Arrange + session = _StreamingSession(lines=_tsv("ENCFF1\tbed")) + mocker.patch.object( + encode_module.aiohttp, "ClientSession", return_value=session + ) + + # Act + [ + row + async for row in fetch_encode_annotation_metadata( + "candidate Cis-Regulatory Elements" + ) + ] + + # Assert + url = session.get_urls[0] + parsed = urlsplit(url) + assert parsed.path == "/metadata/" + # Equality, not substring containment: a substring check passes against + # a URL that also carries a stray or duplicated parameter. + assert parse_qs(parsed.query) == { + "type": ["Annotation"], + "status": ["released"], + "annotation_type": ["candidate Cis-Regulatory Elements"], + } + assert " " not in url + + +@pytest.mark.asyncio +async def test_fetch_encode_annotation_metadata_should_yield_a_row_per_data_line( + mocker, +): + """Test the annotation stream parses the way the experiment one does. + + Given: + A streamed annotation TSV with a header and two data lines. + When: + fetch_encode_annotation_metadata drains it. + Then: + It should yield one dict per data line, keyed by column name. + """ + # Arrange + session = _StreamingSession(lines=_tsv("ENCFF1\tbed", "ENCFF2\tbigBed")) + mocker.patch.object( + encode_module.aiohttp, "ClientSession", return_value=session + ) + + # Act + rows = [row async for row in fetch_encode_annotation_metadata("chromatin state")] + + # Assert + assert rows == [ + {"File accession": "ENCFF1", "File format": "bed"}, + {"File accession": "ENCFF2", "File format": "bigBed"}, + ] + + +@pytest.mark.asyncio +@pytest.mark.parametrize( + "session_kwargs", + [ + {"status": 503}, + {"error": aiohttp.ClientError("connection reset")}, + {"lines": _tsv("ENCFF1\tbed"), "fail_after": 1}, + ], + ids=["http-error", "network-error", "timeout"], +) +async def test_fetch_encode_annotation_metadata_should_release_the_session_on_failure( + mocker, session_kwargs +): + """Test a failed stream does not leave its HTTP session open. + + The stream owns an aiohttp session inside its own context manager, and + the sync isolates phase failures with a broad except that abandons the + generator mid-iteration. With one stream per sync that was academic; + with one per annotation type plus the experiment, a leak per failure + accumulates in a long-lived API process. + + Given: + An annotation stream that fails by HTTP status, by network error, + or by timeout. + When: + It is drained and the error escapes. + Then: + The session should have been exited in every case. + """ + # Arrange + session = _StreamingSession(**session_kwargs) + mocker.patch.object( + encode_module.aiohttp, "ClientSession", return_value=session + ) + + # Act + with pytest.raises((Exception, asyncio.TimeoutError)): + [row async for row in fetch_encode_annotation_metadata("chromatin state")] + + # Assert + assert session.exit_count == 1 + + +@pytest.mark.asyncio +async def test_fetch_encode_annotation_metadata_should_release_the_session_when_abandoned( + mocker, +): + """Test a stream abandoned part-way still releases its session. + + This is the shape the sync's phase isolation produces: the consumer + stops iterating because something else raised, not because the stream + ended. + + Given: + A streamed annotation body of several rows. + When: + The consumer takes one row and closes the generator. + Then: + The session should have been exited. + """ + # Arrange + session = _StreamingSession(lines=_tsv("ENCFF1\tbed", "ENCFF2\tbed")) + mocker.patch.object( + encode_module.aiohttp, "ClientSession", return_value=session + ) + stream = fetch_encode_annotation_metadata("chromatin state") + + # Act + async for _ in stream: + break + await stream.aclose() + + # Assert + assert session.exit_count == 1 + + +@pytest.mark.asyncio +@pytest.mark.parametrize( + "session_kwargs, expectation", + [ + ({"status": 503}, "503"), + ({"error": aiohttp.ClientError("connection reset")}, "network error"), + ], + ids=["http-error", "network-error"], +) +async def test_fetch_encode_annotation_metadata_should_name_its_stream_when_it_fails( + mocker, session_kwargs, expectation +): + """Test a failure message identifies which stream produced it. + + With several streams per sync, a message that does not say which one + died leaves an operator to guess between the experiment corpus and any + of the configured annotation types. + + Given: + An annotation stream for a named type that fails. + When: + It is drained. + Then: + The error should name both the failure and the annotation type. + """ + # Arrange + session = _StreamingSession(**session_kwargs) + mocker.patch.object( + encode_module.aiohttp, "ClientSession", return_value=session + ) + + # Act & assert + with pytest.raises(Exception, match=r"annotation\[chromatin state\]"): + [row async for row in fetch_encode_annotation_metadata("chromatin state")] + + with pytest.raises(Exception, match=expectation): + [row async for row in fetch_encode_annotation_metadata("chromatin state")] + + +@pytest.mark.asyncio +async def test_fetch_encode_annotation_metadata_should_name_its_stream_on_timeout( + mocker, caplog +): + """Test a timed-out annotation stream names its type and its progress. + + Given: + An annotation stream that times out after one data row. + When: + It is drained. + Then: + The log should name the annotation type alongside the row count. + """ + # Arrange + session = _StreamingSession( + lines=_tsv("ENCFF1\tbed", "ENCFF2\tbed"), fail_after=2 + ) + mocker.patch.object( + encode_module.aiohttp, "ClientSession", return_value=session + ) + + # Act & assert + with caplog.at_level(logging.ERROR), pytest.raises(asyncio.TimeoutError): + [row async for row in fetch_encode_annotation_metadata("chromatin state")] + + assert "annotation[chromatin state]" in caplog.text + assert "timed out after 1 rows" in caplog.text + + +@pytest.mark.asyncio +async def test_fetch_encode_annotation_metadata_should_not_let_a_type_inject_parameters( + mocker, +): + """Test a configured value cannot smuggle extra query parameters. + + The annotation type is operator-supplied configuration interpolated + into a URL. Were it not encoded, a value containing an ampersand could + append parameters -- widening status past released, or flipping the + type back to Experiment. + + Given: + An annotation type containing URL metacharacters. + When: + The stream is drained. + Then: + The query should still carry exactly three parameters with the + intended type and status. + """ + # Arrange + hostile = "x&status=deleted&type=Experiment" + session = _StreamingSession(lines=_tsv("ENCFF1\tbed")) + mocker.patch.object( + encode_module.aiohttp, "ClientSession", return_value=session + ) + + # Act + [row async for row in fetch_encode_annotation_metadata(hostile)] + + # Assert + assert parse_qs(urlsplit(session.get_urls[0]).query) == { + "type": ["Annotation"], + "status": ["released"], + "annotation_type": [hostile], + } + + +@pytest.mark.asyncio +async def test_fetch_encode_metadata_should_request_the_released_experiment_corpus( + mocker, +): + """Test the experiment URL is unchanged by the annotation work. + + The annotation fetch builds its query with urlencode while this one + keeps a hand-built literal. The obvious future tidy-up is to unify + them, which would change escaping or parameter order with nothing else + to catch it. + + Given: + A streamed metadata response. + When: + fetch_encode_metadata drains it. + Then: + It should have requested exactly the released-experiment URL. + """ + # Arrange + session = _StreamingSession(lines=_tsv("ENCFF1\tbed")) + mocker.patch.object( + encode_module.aiohttp, "ClientSession", return_value=session + ) + + # Act + [row async for row in fetch_encode_metadata()] + + # Assert + assert session.get_urls[0] == ( + "https://www.encodeproject.org/metadata/?type=Experiment&status=released" + ) + + +@pytest.mark.parametrize("raw", [" ", ",,,", " , , ", "\t"]) +def test_annotation_types_from_env_should_disable_ingest_when_all_entries_blank( + monkeypatch, raw +): + """Test a whitespace-only allowlist disables ingest rather than widening it. + + A blank entry surviving into the allowlist would request + ``annotation_type=``, which the portal is liable to read as unfiltered + -- 580,910 datasets, the exact outcome the allowlist exists to prevent. + + Given: + An override consisting only of separators and whitespace. + When: + annotation_types_from_env is called. + Then: + It should return no types at all. + """ + # Arrange + monkeypatch.setenv(ANNOTATION_TYPES_ENV, raw) + + # Act + types = annotation_types_from_env() + + # Assert + assert types == () + + +def test_annotation_types_from_env_should_collapse_a_repeated_type(monkeypatch): + """Test a repeated entry yields one ingest phase, not two. + + Given: + An override naming the same type twice with differing padding. + When: + annotation_types_from_env is called. + Then: + It should return that type once, keeping first-occurrence order. + """ + # Arrange + monkeypatch.setenv(ANNOTATION_TYPES_ENV, "alpha, beta ,alpha") + + # Act + types = annotation_types_from_env() + + # Assert + assert types == ("alpha", "beta") + + +@given(annotation_type=ENV_SAFE_TEXT.filter(lambda s: s.strip())) +# monkeypatch is function-scoped and so is not reset between generated +# examples. Safe here: every example overwrites the same single variable +# rather than accumulating state, and the fixture still restores the real +# environment once at the end. +@settings( + max_examples=100, + suppress_health_check=[HealthCheck.function_scoped_fixture], +) +def test_annotation_types_from_env_should_round_trip_any_single_type( + monkeypatch, annotation_type +): + """Test any comma-free type survives configuration unchanged. + + ENCODE's vocabulary is case- and space-significant, so a well-meaning + normalization would silently stop matching upstream. + + Given: + Any non-blank text containing no comma. + When: + It is set as the allowlist and read back. + Then: + It should come back as its own stripped form, unaltered otherwise. + """ + # Arrange + assume("," not in annotation_type) + monkeypatch.setenv(ANNOTATION_TYPES_ENV, annotation_type) + + # Act + types = annotation_types_from_env() + + # Assert + assert types == (annotation_type.strip(),) + + +@given(raw=ENV_SAFE_TEXT) +@settings( + max_examples=200, + suppress_health_check=[HealthCheck.function_scoped_fixture], +) +def test_annotation_types_from_env_should_never_yield_a_blank_entry(monkeypatch, raw): + """Test no configuration can produce an unfiltered annotation query. + + Given: + Any text at all, including comma-and-whitespace soup. + When: + annotation_types_from_env is called. + Then: + Every returned entry should be non-blank and already stripped. + """ + # Arrange + monkeypatch.setenv(ANNOTATION_TYPES_ENV, raw) + + # Act + types = annotation_types_from_env() + + # Assert + assert all(entry and entry == entry.strip() for entry in types) + + +def test_transform_annotation_to_c2m2_should_map_the_renamed_columns(): + """Test the renamed columns reach the fields their twins do. + + The annotation TSV publishes the same data as the experiment TSV under + different column names. A missed rename is silent -- the field simply + ends up unset -- so each one is pinned. + + Given: + An annotation row carrying the renamed columns. + When: + transform_annotation_to_c2m2 is called. + Then: + Each value should land where its experiment-named twin would. + """ + # Arrange + row = _annotation_row() + + # Act + doc = transform_annotation_to_c2m2(row) + + # Assert + assert doc["collections"][0]["local_id"] == "ENCSR026KJM" # Dataset accession + assert doc["genome_assembly"] == "mm10" # Assembly + assert doc["creation_time"] == "2017-09-12" # Dataset date released + assert doc["extra"]["encode"]["s3_uri"].endswith("ENCFF845ITU.bigBed") # S3 URL + assert doc["extra"]["encode"]["organism"] == "Mus musculus" # Organism + + +def test_transform_annotation_to_c2m2_should_map_the_assay_term_name_rename(): + """Test Assay term name is read as the experiment path reads Assay. + + Given: + An annotation row whose Assay term name is populated, as the + interaction-prediction type's rows are. + When: + transform_annotation_to_c2m2 is called. + Then: + The dataset should carry it as its experiment_type. + """ + # Arrange + row = _annotation_row(**{"Assay term name": "DNase-seq"}) + + # Act + doc = transform_annotation_to_c2m2(row) + + # Assert + assert doc["collections"][0]["experiment_type"] == "DNase-seq" + + +def test_transform_annotation_to_c2m2_should_make_annotation_type_queryable(): + """Test the field that gives an annotation its meaning is stored. + + Without it a client can only find cCRE files by string-matching + filenames, which is the gap the annotation ingest exists to close. + + Given: + A cCRE annotation row. + When: + transform_annotation_to_c2m2 is called. + Then: + annotation_type should be set on both the file and its dataset. + """ + # Arrange + row = _annotation_row() + + # Act + doc = transform_annotation_to_c2m2(row) + + # Assert + expected = "candidate Cis-Regulatory Elements" + assert doc["extra"]["encode"]["annotation_type"] == expected + assert doc["collections"][0]["extra"]["encode"]["annotation_type"] == expected + + +def test_transform_annotation_to_c2m2_should_store_the_dataset_only_fields(): + """Test the dataset-scoped annotation-only columns are preserved. + + Given: + An annotation row naming its encyclopedia version and the software + that produced it. + When: + transform_annotation_to_c2m2 is called. + Then: + Both should be stored on the dataset. + """ + # Arrange + row = _annotation_row( + **{ + "Software used": "ABC-Enhancer-Gene-Prediction", + "Encyclopedia Version": "ENCODE v4", + } + ) + + # Act + doc = transform_annotation_to_c2m2(row) + + # Assert + dataset_extra = doc["collections"][0]["extra"]["encode"] + assert dataset_extra["software_used"] == "ABC-Enhancer-Gene-Prediction" + assert dataset_extra["encyclopedia_version"] == "ENCODE v4" + + +def test_transform_annotation_to_c2m2_should_carry_targets_as_experiment_target(): + """Test Targets reuses the existing scalar rather than a parallel field. + + Given: + An annotation row whose Targets column lists several targets. + When: + transform_annotation_to_c2m2 is called. + Then: + The dataset's experiment_target should hold the value verbatim. + """ + # Arrange + row = _annotation_row(**{"Targets": "H3K4me3-mouse, CTCF-mouse"}) + + # Act + doc = transform_annotation_to_c2m2(row) + + # Assert + assert doc["collections"][0]["experiment_target"] == "H3K4me3-mouse, CTCF-mouse" + + +def test_transform_annotation_to_c2m2_should_carry_donor_traits_on_the_biosample(): + """Test the age columns are preserved as published. + + They are kept as strings, not parsed: the released corpus contains + "2-4" and "unknown" alongside decimals, which is also why they cannot + go to Subject.age_at_sampling. + + Given: + An annotation row whose Age is a range rather than a number. + When: + transform_annotation_to_c2m2 is called. + Then: + All three should sit on the biosample, verbatim. + """ + # Arrange + row = _annotation_row( + **{"Life stage": "embryonic", "Age": "2-4", "Age units": "week"} + ) + + # Act + doc = transform_annotation_to_c2m2(row) + + # Assert + biosample_extra = doc["collections"][0]["biosamples"][0]["extra"]["encode"] + assert biosample_extra["life_stage"] == "embryonic" + assert biosample_extra["age"] == "2-4" + assert biosample_extra["age_units"] == "week" + + +def test_transform_annotation_to_c2m2_should_leave_experiment_only_fields_unset(): + """Test fields the annotation TSV cannot supply are absent, not invented. + + Given: + An annotation row, whose TSV publishes none of the library, + replicate, genetic-modification or analysis columns. + When: + transform_annotation_to_c2m2 is called. + Then: + Those fields should be absent rather than filled with a derived or + default value. + """ + # Arrange + row = _annotation_row() + + # Act + doc = transform_annotation_to_c2m2(row) + + # Assert + for absent in ( + "genome_annotation", + "output_type_detail", + "biological_replicates", + "technical_replicates", + ): + assert absent not in doc + file_extra = doc["extra"]["encode"] + for absent in ( + "read_length", + "mapped_read_length", + "run_type", + "paired_end", + "paired_with", + "index_of", + "file_analysis_title", + "file_analysis_status", + ): + assert absent not in file_extra + biosample_extra = doc["collections"][0]["biosamples"][0]["extra"]["encode"] + assert not any(key.startswith("library_") for key in biosample_extra) + assert "biosample_genetic_modifications" not in biosample_extra + assert "analyte_class" not in doc["collections"][0] + + +def test_transform_annotation_to_c2m2_should_build_no_subject_without_a_donor(): + """Test no donor is fabricated for a TSV that names none. + + The annotation TSV has no Donor(s) column at all, so there is nothing + to key a Subject on. + + Given: + An annotation row. + When: + transform_annotation_to_c2m2 is called. + Then: + Both the dataset and its biosample should carry no subjects. + """ + # Arrange + row = _annotation_row() + + # Act + doc = transform_annotation_to_c2m2(row) + + # Assert + collection = doc["collections"][0] + assert collection["subjects"] == [] + assert collection["biosamples"][0]["subjects"] == [] + + +def test_transform_annotation_to_c2m2_should_link_to_the_annotations_path(): + """Test the dataset's persistent_id resolves. + + ENCODE serves annotations and experiments under different paths, so + reusing the experiment path would mint a link that 404s. + + Given: + An annotation row with a dataset accession. + When: + transform_annotation_to_c2m2 is called. + Then: + The dataset's persistent_id should point at /annotations/. + """ + # Arrange + row = _annotation_row() + + # Act + doc = transform_annotation_to_c2m2(row) + + # Assert + assert doc["collections"][0]["persistent_id"] == ( + "https://www.encodeproject.org/annotations/ENCSR026KJM/" + ) + + +def test_transform_annotation_to_c2m2_should_build_a_dataset_without_a_biosample(): + """Test a dataset accession stays queryable with no biosample term. + + 48 of the released cCRE files name no biosample term. Gating the + dataset on one -- as the experiment path does -- would leave 24 dataset + accessions unreachable. + + Given: + An annotation row with a dataset accession but no biosample term. + When: + transform_annotation_to_c2m2 is called. + Then: + It should still build the dataset -- addressable, linkable and + labelled -- carrying no biosamples. + """ + # Arrange + row = _annotation_row( + **{"Biosample term name": "", "Biosample term id": "", "Biosample type": ""} + ) + + # Act + doc = transform_annotation_to_c2m2(row) + + # Assert + collection = doc["collections"][0] + assert collection["local_id"] == "ENCSR026KJM" + assert collection["accession_id"] == "ENCSR026KJM" + assert collection["biosamples"] == [] + # Addressable and labelled, not merely present: a collection a client + # cannot resolve or filter is not what those 24 datasets needed. + assert collection["persistent_id"] == ( + "https://www.encodeproject.org/annotations/ENCSR026KJM/" + ) + assert ( + collection["extra"]["encode"]["annotation_type"] + == "candidate Cis-Regulatory Elements" + ) + + +def test_transform_annotation_to_c2m2_should_fold_the_dataset_accession(): + """Test the dataset accession is folded the way filters are. + + Given: + An annotation row whose dataset accession is lower-cased. + When: + transform_annotation_to_c2m2 is called. + Then: + accession_id should be folded while local_id keeps the raw value. + """ + # Arrange + row = _annotation_row(**{"Dataset accession": "encsr026kjm"}) + + # Act + doc = transform_annotation_to_c2m2(row) + + # Assert + collection = doc["collections"][0] + assert collection["local_id"] == "encsr026kjm" + assert collection["accession_id"] == normalize_accession("encsr026kjm") + + +def test_transform_annotation_to_c2m2_should_return_none_without_an_accession(): + """Test a row with no file accession is skipped rather than inserted. + + Given: + An annotation row whose File accession is blank. + When: + transform_annotation_to_c2m2 is called. + Then: + It should return None. + """ + # Arrange + row = _annotation_row(**{"File accession": " "}) + + # Act + doc = transform_annotation_to_c2m2(row) + + # Assert + assert doc is None + + +def test_transform_annotation_to_c2m2_should_keep_bedpe_out_of_the_bed_format(): + """Test a paired-interval annotation file is not labelled plain BED. + + A bedpe labelled BED is routed into the tabix BED pipeline, which + indexes the first mate and silently drops the second. + + Given: + An annotation row published as bedpe. + When: + transform_annotation_to_c2m2 is called. + Then: + Its file_format should name bedpe rather than BED. + """ + # Arrange + row = _annotation_row(**{"File format": "bedpe"}) + + # Act + doc = transform_annotation_to_c2m2(row) + + # Assert + assert doc["file_format"]["name"] == "bedpe" + + +def test_transform_annotation_to_c2m2_should_populate_the_required_c2m2_fields(): + """Test annotation documents are as complete as experiment documents. + + Given: + An annotation row. + When: + transform_annotation_to_c2m2 is called. + Then: + The fields a client needs to locate and fetch the file should all + be populated, on the same code paths the experiment ingest uses. + """ + # Arrange + row = _annotation_row() + + # Act + doc = transform_annotation_to_c2m2(row) + + # Assert + assert doc["dcc"]["dcc_abbreviation"] == "ENCODE" + assert doc["file_format"] == {"id": "format:3004", "name": "bigBed"} + # The exact term, so deleting the annotation entries from + # OUTPUT_TYPE_TO_EDAM fails at the ingest boundary too, not only in the + # ontology unit test. + assert doc["data_type"] == {"id": "data:1255", "name": "Sequence features"} + assert doc["access_url"] == ANNOTATION_DOWNLOAD_URL + assert doc["md5"] == "5ff392dcde69f8ec512ea381928674d9" + assert doc["size_in_bytes"] == 13743825 + + +@given(name=st.text(min_size=0, max_size=40)) +@settings(max_examples=100) +def test_transform_annotation_to_c2m2_should_not_raise_for_any_download_url(name): + """Test the annotation path is total over download URLs. + + Given: + Any download filename, including empty and punctuation-only ones. + When: + transform_annotation_to_c2m2 is called. + Then: + It should return a document rather than raising. + """ + # Arrange + row = _annotation_row(**{"File download URL": DOWNLOAD_URL.format(name=name)}) + + # Act + doc = transform_annotation_to_c2m2(row) + + # Assert + assert doc is not None + + +def test_transform_to_c2m2_should_link_to_the_experiments_path(): + """Test an experiment dataset's persistent_id resolves. + + The URL path became a caller-supplied argument when the annotation + ingest landed. ENCODE serves the two dataset kinds under different + paths, so passing the wrong one would give all ~27,000 experiment + collections a link that 404s -- and the annotation side of the same + interpolation is pinned while this one was not. + + Given: + An experiment row with an accession and a biosample term. + When: + transform_to_c2m2 is called. + Then: + The collection's persistent_id should point at /experiments/. + """ + # Arrange + row = _encode_row( + **{"Experiment accession": "ENCSR918ZSJ", "Biosample term name": "K562"} + ) + + # Act + doc = transform_to_c2m2(row) + + # Assert + assert doc["collections"][0]["persistent_id"] == ( + "https://www.encodeproject.org/experiments/ENCSR918ZSJ/" + ) + + +@pytest.mark.parametrize( + "term_id, expected", + [ + ("UBERON:0002048", {"id": "UBERON:0002048", "name": "lung"}), + ("", None), + ], + ids=["with-term-id", "without-term-id"], +) +def test_transform_to_c2m2_should_derive_anatomy_from_the_biosample_term( + term_id, expected +): + """Test anatomy tracks the term id while the collection tracks the name. + + The guard producing anatomy was rewritten when the annotation path + landed, and nothing asserted the field on either path -- deleting the + anatomy block outright would have failed no test. + + Given: + An experiment row with a biosample term name, with and without a + term id. + When: + transform_to_c2m2 is called. + Then: + Anatomy should appear on both the biosample and the collection + when the term id is present and on neither when it is not, with + the collection built either way. + """ + # Arrange + row = _encode_row( + **{ + "Experiment accession": "ENCSR918ZSJ", + "Biosample term name": "lung", + "Biosample term id": term_id, + } + ) + + # Act + doc = transform_to_c2m2(row) + + # Assert + collection = doc["collections"][0] + assert collection["biosamples"][0].get("anatomy") == expected + assert collection.get("anatomy") == ([expected] if expected else None) + + +def test_transform_to_c2m2_should_not_stamp_annotation_fields_on_an_experiment(): + """Test the annotation-only post-processing stays on its own path. + + Given: + An experiment row that happens to carry an Annotation type column. + When: + transform_to_c2m2 is called. + Then: + No annotation_type should appear on the file or its collection. + """ + # Arrange + row = _encode_row( + **{ + "Experiment accession": "ENCSR918ZSJ", + "Biosample term name": "K562", + "Annotation type": "candidate Cis-Regulatory Elements", + } + ) + + # Act + doc = transform_to_c2m2(row) + + # Assert + assert "annotation_type" not in doc.get("extra", {}).get("encode", {}) + assert "annotation_type" not in doc["collections"][0].get("extra", {}).get( + "encode", {} + ) + + +def test_transform_to_c2m2_should_keep_bedpe_out_of_the_bed_format(): + """Test the remap reaches the experiment corpus, not only annotations. + + ENCODE already publishes .bedpe under type=Experiment, and those files + were the original motivation for not calling a paired-interval format + BED. + + Given: + An experiment row published as bedpe. + When: + transform_to_c2m2 is called. + Then: + Its file_format should name bedpe rather than BED. + """ + # Arrange + row = _encode_row(**{"File format": "bedpe"}) + + # Act + doc = transform_to_c2m2(row) + + # Assert + assert doc["file_format"]["name"] == "bedpe" + + +def test_transform_to_c2m2_should_mirror_the_assembly_under_the_dcc_namespace(): + """Test extra.encode.assembly carries the value the schema promises. + + The field was declared and published in the SDL but never written, so + a client reaching for it -- the natural move once multi-assembly cCREs + made assembly a filter people use -- matched nothing at all. + + Given: + An experiment row naming an assembly. + When: + transform_to_c2m2 is called. + Then: + extra.encode.assembly should mirror the top-level genome_assembly. + """ + # Arrange + row = _encode_row(**{"File assembly": "GRCh38"}) + + # Act + doc = transform_to_c2m2(row) + + # Assert + assert doc["genome_assembly"] == "GRCh38" + assert doc["extra"]["encode"]["assembly"] == "GRCh38" + + +def test_transform_annotation_to_c2m2_should_mirror_the_assembly_under_the_dcc_namespace(): + """Test extra.encode.assembly carries the value the schema promises. + + The annotation TSV names the column "Assembly" rather than "File + assembly", so the two transforms reach the same field by different + routes and each needs its own pin. + + Given: + An annotation row naming an assembly. + When: + transform_annotation_to_c2m2 is called. + Then: + extra.encode.assembly should mirror the top-level genome_assembly. + """ + # Arrange + row = _annotation_row(**{"Assembly": "GRCh38"}) + + # Act + doc = transform_annotation_to_c2m2(row) + + # Assert + assert doc["genome_assembly"] == "GRCh38" + assert doc["extra"]["encode"]["assembly"] == "GRCh38" + + +def test_transform_annotation_to_c2m2_should_produce_a_valid_file_document(): + """Test the emitted document is one the read path can deserialize. + + The ingest writes plain dicts and the GraphQL resolver reads them back + through FileMetadataModel on every row, so a key the model does not + declare is dropped silently -- the filter would exist and match + nothing. Nothing else joins the two halves. + + Given: + A full annotation row. + When: + The emitted document is validated as a FileMetadataModel. + Then: + It should validate and expose all the annotation fields on their + enriched models. + """ + # Arrange + row = _annotation_row(**{"Software used": "ABC-Enhancer-Gene-Prediction"}) + + # Act + model = FileMetadataModel(**transform_annotation_to_c2m2(row)) + + # Assert + assert model.extra.encode.annotation_type == "candidate Cis-Regulatory Elements" + assert model.extra.encode.organism == "Mus musculus" + assert model.extra.encode.assembly == "mm10" + dataset = model.collections[0] + assert dataset.extra.encode.annotation_type == ( + "candidate Cis-Regulatory Elements" + ) + assert dataset.extra.encode.software_used == "ABC-Enhancer-Gene-Prediction" + assert dataset.extra.encode.encyclopedia_version == "ENCODE v1" + biosample = dataset.biosamples[0] + assert biosample.extra.encode.life_stage == "postnatal" + assert biosample.extra.encode.age == "0" + assert biosample.extra.encode.age_units == "day" + + +def test_transform_annotation_to_c2m2_should_omit_an_empty_dataset_extra(): + """Test a dataset with nothing to enrich carries no empty extra dict. + + An empty ``{"encode": {}}`` would surface as a null facet in any + distinct-value enumeration over the dataset fields. + + Given: + An annotation row whose every dataset-level source is blank. + When: + transform_annotation_to_c2m2 is called. + Then: + The dataset should carry no extra key at all. + """ + # Arrange + row = _annotation_row( + **{ + "Project": "", + "dbxrefs": "", + "Annotation type": "", + "Software used": "", + "Encyclopedia Version": "", + } + ) + + # Act + doc = transform_annotation_to_c2m2(row) + + # Assert + assert "extra" not in doc["collections"][0] + + +def test_transform_annotation_to_c2m2_should_skip_a_blank_annotation_type(): + """Test a blank annotation type is absent rather than empty. + + Given: + An annotation row with no Annotation type but a populated + Encyclopedia Version. + When: + transform_annotation_to_c2m2 is called. + Then: + Neither the file nor the dataset should carry annotation_type, + while the encyclopedia version still lands. + """ + # Arrange + row = _annotation_row(**{"Annotation type": " "}) + + # Act + doc = transform_annotation_to_c2m2(row) + + # Assert + assert "annotation_type" not in doc["extra"]["encode"] + dataset_extra = doc["collections"][0]["extra"]["encode"] + assert "annotation_type" not in dataset_extra + assert dataset_extra["encyclopedia_version"] == "ENCODE v1" + + +def test_transform_annotation_to_c2m2_should_enrich_a_biosample_keyed_dataset(): + """Test the locally synthesized dataset still carries its annotation fields. + + Reachable on the annotation path because the collection is no longer + gated on the biosample term: a row with a biosample but no dataset + accession falls back to a ``biosample:``-keyed collection. + + Given: + An annotation row with a biosample term but no dataset accession. + When: + transform_annotation_to_c2m2 is called. + Then: + The fallback dataset should carry annotation_type and + experiment_target, and no fabricated accession or persistent id. + """ + # Arrange + row = _annotation_row(**{"Dataset accession": ""}) + + # Act + doc = transform_annotation_to_c2m2(row) + + # Assert + dataset = doc["collections"][0] + assert dataset["local_id"] == "biosample:lung" + assert dataset["extra"]["encode"]["annotation_type"] == ( + "candidate Cis-Regulatory Elements" + ) + assert dataset["experiment_target"] == "H3K4me3-mouse, CTCF-mouse" + assert "accession_id" not in dataset + assert "persistent_id" not in dataset + + +def test_transform_annotation_to_c2m2_should_still_label_a_file_with_no_dataset(): + """Test a file with nothing to group it by is still classified. + + Given: + An annotation row with neither a dataset accession nor a biosample + term. + When: + transform_annotation_to_c2m2 is called. + Then: + It should build no collection while the file still carries its + annotation type. + """ + # Arrange + row = _annotation_row( + **{ + "Dataset accession": "", + "Biosample term name": "", + "Biosample term id": "", + } + ) + + # Act + doc = transform_annotation_to_c2m2(row) + + # Assert + assert doc["collections"] == [] + assert doc["extra"]["encode"]["annotation_type"] == ( + "candidate Cis-Regulatory Elements" + ) + + +def test_transform_annotation_to_c2m2_should_not_assay_type_an_empty_assay(): + """Test the shape every released cCRE row actually has. + + All 12,448 of them publish an empty Assay term name, so this is the + dominant annotation shape rather than an edge case. + + Given: + An annotation row whose Assay term name is empty. + When: + transform_annotation_to_c2m2 is called. + Then: + The dataset should be built with no experiment_type and the file + with no assay_type, rather than either being defaulted. + """ + # Arrange + row = _annotation_row() + + # Act + doc = transform_annotation_to_c2m2(row) + + # Assert + assert "assay_type" not in doc + assert "experiment_type" not in doc["collections"][0] + + +def test_transform_annotation_to_c2m2_should_pass_a_multi_valued_assay_through(): + """Test a comma-joined assay list is preserved rather than dropped. + + Some interaction-prediction rows name several assays in one column. + No OBI term matches the joined string, which is correct -- but the + string itself is still the best available description. + + Given: + An annotation row whose Assay term name lists four assays. + When: + transform_annotation_to_c2m2 is called. + Then: + experiment_type should hold it verbatim while assay_type stays + absent. + """ + # Arrange + row = _annotation_row( + **{"Assay term name": "ChIP-seq, RNA-seq, HiC, ATAC-seq"} + ) + + # Act + doc = transform_annotation_to_c2m2(row) + + # Assert + assert doc["collections"][0]["experiment_type"] == ( + "ChIP-seq, RNA-seq, HiC, ATAC-seq" + ) + assert "assay_type" not in doc + + +def test_transform_annotation_to_c2m2_should_drop_donor_traits_with_no_biosample(): + """Test the donor traits go nowhere when there is no biosample to hold them. + + They are biosample-scoped, so a row with no biosample term has no + honest destination for them short of inventing one. Pinned rather than + fixed: no released annotation row carries both, so nothing is lost + today -- but if ENCODE starts publishing that combination this test is + what makes the loss visible instead of silent. + + Given: + An annotation row naming a life stage and age but no biosample + term. + When: + transform_annotation_to_c2m2 is called. + Then: + The dataset should be built with no biosamples and the traits + should appear nowhere in the document. + """ + # Arrange + row = _annotation_row( + **{ + "Biosample term name": "", + "Biosample term id": "", + "Life stage": "embryonic", + "Age": "10.5", + "Age units": "week", + } + ) + + # Act + doc = transform_annotation_to_c2m2(row) + + # Assert + assert doc["collections"][0]["biosamples"] == [] + assert "embryonic" not in repr(doc) + assert "10.5" not in repr(doc) + + +def test_transform_annotation_to_c2m2_should_keep_biginteract_out_of_bigbed(): + """Test a bigInteract annotation file is not labelled plain bigBed. + + Extracting it as a bigBed and indexing its leading columns is + range-coherent but degrades the interaction to an interval. + + Given: + An annotation row published as bigInteract. + When: + transform_annotation_to_c2m2 is called. + Then: + Its file_format should name bigInteract rather than bigBed. + """ + # Arrange + row = _annotation_row(**{"File format": "bigInteract"}) + + # Act + doc = transform_annotation_to_c2m2(row) + + # Assert + assert doc["file_format"]["name"] == "bigInteract" + + +def test_transform_to_c2m2_should_store_the_organism_on_the_file(): + """Test the experiment path records organism where annotations do. + + Both TSVs publish the same datum under different names. Populating it + from both keeps the filter meaning the same thing corpus-wide rather + than matching annotation files only. + + Given: + An experiment row naming its Biosample organism. + When: + transform_to_c2m2 is called. + Then: + The file should carry that organism. + """ + # Arrange + row = _encode_row( + **{"Biosample organism": "Homo sapiens", "Biosample term name": "K562"} + ) + + # Act + doc = transform_to_c2m2(row) + + # Assert + assert doc["extra"]["encode"]["organism"] == "Homo sapiens" From 8621063fabf1802aee2b90839bc2195cb99b66d6 Mon Sep 17 00:00:00 2001 From: Conrad Date: Thu, 13 Aug 2026 14:07:11 -0400 Subject: [PATCH 10/15] test: Cover the isolated ingest phases and their accounting The central pin is that the reported total always equals the documents actually committed. It is asserted for a stream that dies mid-flight -- the shape a metadata timeout takes -- and then generalized by a property test over arbitrary fan-out, row counts, batch sizes and failure patterns, since the example cases only fix a handful of shapes. Those assertions are anchored to the whole progress line rather than matched as substrings: "6 files" is contained in "16 files", so containment passes on an order-of-magnitude error in the one number this design exists to get right. The failure cases now exercise the sink as well as the stream. Every earlier one failed the metadata fetch, so nothing covered a stream that drains cleanly and then cannot commit its last batch, which returned normally and reported a clean sync over a short corpus. A batch the sink rejects is pinned as never resubmitted, and a cancellation with rows still buffered as never flushed. Around those: the partial batch of a failed phase is committed rather than discarded, a repeated allowlist entry loads its type once, a configured type that returns nothing is reported as zero, a cancellation propagates instead of being swallowed as a phase failure, and the error chains the first failure so the traceback points at what started the cascade. Every phase is pinned to receive one shared deadline, which no test counting rows could otherwise observe. The clearing tests follow the slice. A phase clears only what it reloads, so the total-failure path leaves last sync's corpus intact rather than emptying it, and a phase whose stream drains cleanly with no rows empties its own slice rather than serving withdrawn documents indefinitely. Batching gets its boundaries -- exactly one batch, one plus a remainder, and none -- plus the skip path for a row that does not transform. The fake collection's $exists now resolves dotted paths. It tested for a top-level key, so a filter on extra.encode.annotation_type reported every document as lacking it and matched them all; the per-slice clearing tests would have passed against a clear that deleted the corpus. The fixture that disables annotation phases moves from module scope to the classes that drive the sync. It governs a network boundary -- without it a unit test reaches the real portal once per configured type -- so it belongs next to the tests that depend on it rather than silently over the 4DN and HuBMAP ones. A new test opts out of it deliberately, because nothing otherwise joined the default allowlist to the phases that run. --- tests/conftest.py | 22 +- tests/test_sync.py | 1218 +++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 1226 insertions(+), 14 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index a711076..24769c1 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -15,14 +15,19 @@ ISSUE_83_SIZE = 6262125716 -def _resolve(doc: dict, key: str): +#: Distinguishes "no such path" from "a path holding None", which ``$exists`` +#: has to tell apart and a bare ``None`` default cannot. +_MISSING = object() + + +def _resolve(doc: dict, key: str, default=None): """Resolve a possibly dot-notated key against a nested dict.""" value = doc for part in key.split("."): - if isinstance(value, dict): - value = value.get(part) + if isinstance(value, dict) and part in value: + value = value[part] else: - return None + return default return value @@ -53,9 +58,12 @@ def _match(doc: dict, query: dict) -> bool: if value == operand: return False elif op == "$exists": - if operand and key not in doc: - return False - if not operand and key in doc: + # Resolved rather than tested with ``in doc``, which sees + # only top-level keys and so reports every dotted path as + # absent -- making {"a.b": {"$exists": False}} match + # everything, including the documents that do have it. + present = _resolve(doc, key, _MISSING) is not _MISSING + if bool(operand) is not present: return False elif op == "$regex": import re diff --git a/tests/test_sync.py b/tests/test_sync.py index 46e36a6..6311488 100644 --- a/tests/test_sync.py +++ b/tests/test_sync.py @@ -6,7 +6,10 @@ import logging import pytest +from hypothesis import HealthCheck, given, settings +from hypothesis import strategies as st +from cfdb import api from cfdb.services import encode as encode_module from cfdb.services import fourdn as fourdn_module from cfdb.services import sync as sync_module @@ -24,6 +27,33 @@ _sync_encode, ) from cfdb.indexes import data_index_specs +from tests.conftest import FakeDB + + +class _EncodeSyncTestBase: + """Base for the classes that drive ``_sync_encode``. + + ``_sync_encode`` fans out over ``annotation_types_from_env()``, whose + default allowlist is non-empty, so a test that mocks only the experiment + fetch would reach the real ENCODE portal once per annotation type and + stream tens of thousands of live rows into its assertions. The autouse + fixture below turns the annotation phases off by default. + + Scoped to these classes rather than the module: nothing in the 4DN or + HuBMAP tests reads the variable, and a module-wide autouse fixture that + silently governs a network boundary is worth keeping close to the tests + that depend on it. + + To opt back in, either set ``ENCODE_ANNOTATION_TYPES`` to the types + under test or ``monkeypatch.delenv`` it to exercise the default + allowlist -- the fixture is function-scoped, so a test's own + monkeypatching wins. + """ + + @pytest.fixture(autouse=True) + def no_annotation_phases(self, monkeypatch): + """Turn the annotation phases off unless a test asks for them.""" + monkeypatch.setenv("ENCODE_ANNOTATION_TYPES", "") def _encode_metadata_row(accession: str, filename: str) -> dict: @@ -37,12 +67,65 @@ def _encode_metadata_row(accession: str, filename: str) -> dict: } +def _encode_annotation_row(accession: str, dataset: str, filename: str) -> dict: + """Return a minimal ENCODE annotation TSV row, using annotation column names.""" + return { + "File accession": accession, + "File format": "bed", + "Dataset accession": dataset, + "Annotation type": "candidate Cis-Regulatory Elements", + "Assembly": "GRCh38", + "Organism": "Homo sapiens", + "File download URL": ( + f"https://www.encodeproject.org/files/{accession}/@@download/{filename}" + ), + } + + async def _async_iter(rows): """Yield the given rows, standing in for the streaming metadata fetch.""" for row in rows: yield row +async def _async_raise(exc): + """Stand in for a metadata stream that fails before yielding anything.""" + raise exc + yield # pragma: no cover - unreachable, marks this an async generator + + +async def _async_iter_then_raise(rows, exc): + """Stand in for a stream that dies partway through. + + The production failure shape: the metadata fetch's timeout bounds the + whole streamed body, so it fires with rows already delivered and + inserted, not before the first one. + """ + for row in rows: + yield row + raise exc + + +def _fail_insert_on_call(collection, failing_call: int, exc: Exception) -> None: + """Make one of a fake collection's insert_many calls raise. + + Fails the *sink* rather than the stream, which is the half of the ingest + the stream helpers above cannot reach. Keyed by call ordinal so a test can + choose a batch boundary without inspecting the rows. + """ + real = collection.insert_many + calls = 0 + + async def insert_many(docs): + nonlocal calls + calls += 1 + if calls == failing_call: + raise exc + return await real(docs) + + collection.insert_many = insert_many + + class TestSyncDccsDataIndexing: @pytest.mark.asyncio async def test__sync_dccs_should_ensure_data_indexes_after_load( @@ -392,7 +475,7 @@ async def test__load_dataset_async_should_not_add_compression_format_when_absent # --------------------------------------------------------------------------- -class TestSyncEncode: +class TestSyncEncode(_EncodeSyncTestBase): @pytest.mark.asyncio async def test__sync_encode_should_populate_accession_id_on_inserted_docs( self, mock_db, mocker @@ -417,7 +500,7 @@ async def test__sync_encode_should_populate_accession_id_on_inserted_docs( row["Experiment accession"] = "encsr918zsj" row["Biosample term name"] = "K562" mocker.patch.object( - encode_module, "fetch_encode_metadata", lambda: _async_iter([row]) + encode_module, "fetch_encode_metadata", lambda deadline=None: _async_iter([row]) ) # Act @@ -451,7 +534,7 @@ async def test__sync_encode_should_populate_compression_format_on_inserted_docs( _encode_metadata_row("ENCFF003CCC", "ENCFF003CCC.bed.starch"), ] mocker.patch.object( - encode_module, "fetch_encode_metadata", lambda: _async_iter(rows) + encode_module, "fetch_encode_metadata", lambda deadline=None: _async_iter(rows) ) # Act @@ -491,7 +574,7 @@ async def test__sync_encode_should_log_the_compression_format_distribution( _encode_metadata_row("ENCFF003CCC", "ENCFF003CCC.bed.starch"), ] mocker.patch.object( - encode_module, "fetch_encode_metadata", lambda: _async_iter(rows) + encode_module, "fetch_encode_metadata", lambda deadline=None: _async_iter(rows) ) # Act @@ -536,7 +619,7 @@ async def test__sync_encode_should_leave_other_dcc_documents_unchanged( mocker.patch.object( encode_module, "fetch_encode_metadata", - lambda: _async_iter([_encode_metadata_row("ENCFF001AAA", "x.bed.gz")]), + lambda deadline=None: _async_iter([_encode_metadata_row("ENCFF001AAA", "x.bed.gz")]), ) # Act @@ -547,6 +630,1127 @@ async def test__sync_encode_should_leave_other_dcc_documents_unchanged( assert survivors == others +class TestSyncEncodeAnnotationPhases(_EncodeSyncTestBase): + @pytest.mark.asyncio + async def test__sync_encode_should_ingest_annotations_alongside_experiments( + self, mock_db, mocker, monkeypatch + ): + """Test that both metadata streams reach the files collection. + + Given: + One configured annotation type, plus an experiment stream and an + annotation stream each yielding one row. + When: + _sync_encode runs. + Then: + Both documents should be inserted, and the annotation one should + carry the annotation_type that makes it findable. + """ + # Arrange + monkeypatch.setenv( + "ENCODE_ANNOTATION_TYPES", "candidate Cis-Regulatory Elements" + ) + mocker.patch.object( + encode_module, + "fetch_encode_metadata", + lambda deadline=None: _async_iter([_encode_metadata_row("ENCFF001AAA", "x.bed.gz")]), + ) + mocker.patch.object( + encode_module, + "fetch_encode_annotation_metadata", + lambda annotation_type, deadline=None: _async_iter( + [_encode_annotation_row("ENCFF002BBB", "ENCSR001AAA", "y.bed.gz")] + ), + ) + + # Act + await _sync_encode(SyncTask(id="t1", dcc_names=["encode"])) + + # Assert + by_id = {d["local_id"]: d for d in mock_db.files.docs} + assert set(by_id) == {"ENCFF001AAA", "ENCFF002BBB"} + annotation = by_id["ENCFF002BBB"] + assert ( + annotation["extra"]["encode"]["annotation_type"] + == "candidate Cis-Regulatory Elements" + ) + assert annotation["collections"][0]["accession_id"] == "ENCSR001AAA" + + @pytest.mark.asyncio + async def test__sync_encode_should_request_only_the_configured_types( + self, mock_db, mocker, monkeypatch + ): + """Test that the allowlist bounds which annotation types are fetched. + + Given: + Two annotation types configured out of the far larger space + ENCODE publishes. + When: + _sync_encode runs. + Then: + The annotation fetch should be called once per configured type + and with no other type. + """ + # Arrange + monkeypatch.setenv("ENCODE_ANNOTATION_TYPES", "chromatin state, footprints") + mocker.patch.object( + encode_module, "fetch_encode_metadata", lambda deadline=None: _async_iter([]) + ) + fetch_annotations = mocker.patch.object( + encode_module, + "fetch_encode_annotation_metadata", + side_effect=lambda annotation_type, deadline=None: _async_iter([]), + ) + + # Act + await _sync_encode(SyncTask(id="t1", dcc_names=["encode"])) + + # Assert + requested = [call.args[0] for call in fetch_annotations.call_args_list] + assert requested == ["chromatin state", "footprints"] + + @pytest.mark.asyncio + async def test__sync_encode_should_ingest_annotations_when_experiments_fail( + self, mock_db, mocker, monkeypatch + ): + """Test that a failed experiment stream does not cost the annotations. + + Given: + An experiment stream that raises, and a healthy annotation + stream. + When: + _sync_encode runs. + Then: + The annotation document should still be inserted, and the sync + should fail afterwards naming the phase that broke. + """ + # Arrange + monkeypatch.setenv( + "ENCODE_ANNOTATION_TYPES", "candidate Cis-Regulatory Elements" + ) + mocker.patch.object( + encode_module, + "fetch_encode_metadata", + lambda deadline=None: _async_raise(RuntimeError("experiment stream died")), + ) + mocker.patch.object( + encode_module, + "fetch_encode_annotation_metadata", + lambda annotation_type, deadline=None: _async_iter( + [_encode_annotation_row("ENCFF002BBB", "ENCSR001AAA", "y.bed.gz")] + ), + ) + + # Act & assert + with pytest.raises(RuntimeError, match="experiment"): + await _sync_encode(SyncTask(id="t1", dcc_names=["encode"])) + + assert [d["local_id"] for d in mock_db.files.docs] == ["ENCFF002BBB"] + + @pytest.mark.asyncio + async def test__sync_encode_should_ingest_experiments_when_an_annotation_fails( + self, mock_db, mocker, monkeypatch + ): + """Test that one failed annotation type does not cost the others. + + Given: + Two configured annotation types where the first stream raises, + alongside a healthy experiment stream. + When: + _sync_encode runs. + Then: + The experiment document and the surviving annotation type's + document should both be inserted. + """ + # Arrange + monkeypatch.setenv("ENCODE_ANNOTATION_TYPES", "broken, healthy") + mocker.patch.object( + encode_module, + "fetch_encode_metadata", + lambda deadline=None: _async_iter([_encode_metadata_row("ENCFF001AAA", "x.bed.gz")]), + ) + + def _annotation_stream(annotation_type, deadline=None): + if annotation_type == "broken": + return _async_raise(RuntimeError("annotation stream died")) + return _async_iter( + [_encode_annotation_row("ENCFF002BBB", "ENCSR001AAA", "y.bed.gz")] + ) + + mocker.patch.object( + encode_module, + "fetch_encode_annotation_metadata", + side_effect=_annotation_stream, + ) + + # Act & assert + with pytest.raises(RuntimeError, match="annotation\\[broken\\]"): + await _sync_encode(SyncTask(id="t1", dcc_names=["encode"])) + + assert {d["local_id"] for d in mock_db.files.docs} == { + "ENCFF001AAA", + "ENCFF002BBB", + } + + @pytest.mark.asyncio + async def test__sync_encode_should_ensure_the_indexes_when_a_phase_failed( + self, mock_db, mocker, monkeypatch + ): + """Test that a partial load is still left queryable. + + Given: + An experiment stream that raises and a healthy annotation + stream, so the sync ends in failure with rows loaded. + When: + _sync_encode runs. + Then: + The accession indexes should still be ensured, so what did load + is not left behind a full collection scan. + """ + # Arrange + monkeypatch.setenv( + "ENCODE_ANNOTATION_TYPES", "candidate Cis-Regulatory Elements" + ) + mocker.patch.object( + encode_module, + "fetch_encode_metadata", + lambda deadline=None: _async_raise(RuntimeError("experiment stream died")), + ) + mocker.patch.object( + encode_module, + "fetch_encode_annotation_metadata", + lambda annotation_type, deadline=None: _async_iter( + [_encode_annotation_row("ENCFF002BBB", "ENCSR001AAA", "y.bed.gz")] + ), + ) + ensure = mocker.patch.object( + sync_module, "ensure_indexes", mocker.AsyncMock(return_value=0) + ) + + # Act & assert + with pytest.raises(RuntimeError): + await _sync_encode(SyncTask(id="t1", dcc_names=["encode"])) + + assert ensure.await_count == 1 + + @pytest.mark.asyncio + async def test__sync_encode_should_report_the_rows_a_failed_phase_committed( + self, mock_db, mocker, monkeypatch + ): + """Test the reported total matches what is actually in the database. + + A stream bounded by a wall-clock budget fails mid-flight, with rows + already committed. Reporting a phase's contribution only on its + clean return dropped every one of those rows from the total while + leaving them in the collection, so the sync under-reported the + corpus it had just written. + + Given: + A batch size of 2 and an experiment stream that yields 5 rows + and then dies, alongside a healthy annotation phase. + When: + _sync_encode runs. + Then: + The count in the final progress should equal the ENCODE + documents actually present. + """ + # Arrange + monkeypatch.setenv( + "ENCODE_ANNOTATION_TYPES", "candidate Cis-Regulatory Elements" + ) + monkeypatch.setattr(sync_module, "BATCH_SIZE", 2) + rows = [ + _encode_metadata_row(f"ENCFF00{i}AAA", f"f{i}.bed.gz") for i in range(5) + ] + mocker.patch.object( + encode_module, + "fetch_encode_metadata", + lambda deadline=None: _async_iter_then_raise(rows, TimeoutError("budget exhausted")), + ) + mocker.patch.object( + encode_module, + "fetch_encode_annotation_metadata", + lambda annotation_type, deadline=None: _async_iter( + [_encode_annotation_row("ENCFF999ZZZ", "ENCSR001AAA", "y.bed.gz")] + ), + ) + task = SyncTask(id="t1", dcc_names=["encode"]) + + # Act & assert + with pytest.raises(RuntimeError): + await _sync_encode(task) + + inserted = [d for d in mock_db.files.docs if d["submission"] == "encode"] + # Anchored rather than a substring: "6 files" is contained in + # "16 files", so containment passes on an order-of-magnitude error. + assert task.progress.startswith( + f"ENCODE sync incomplete: {len(inserted)} files" + ) + + @pytest.mark.asyncio + async def test__sync_encode_should_commit_the_partial_batch_of_a_failed_phase( + self, mock_db, mocker, monkeypatch + ): + """Test rows transformed before a stream died are not thrown away. + + The DCC is cleared before the load, so a row discarded here is a + row the corpus loses outright until the next full re-sync. + + Given: + A batch size of 2 and a stream that yields 5 rows and then + dies, leaving one row in an uncommitted partial batch. + When: + _sync_encode runs. + Then: + All 5 rows should be committed, not just the two full batches. + """ + # Arrange + monkeypatch.setattr(sync_module, "BATCH_SIZE", 2) + rows = [ + _encode_metadata_row(f"ENCFF00{i}AAA", f"f{i}.bed.gz") for i in range(5) + ] + mocker.patch.object( + encode_module, + "fetch_encode_metadata", + lambda deadline=None: _async_iter_then_raise(rows, TimeoutError("budget exhausted")), + ) + + # Act & assert + with pytest.raises(RuntimeError): + await _sync_encode(SyncTask(id="t1", dcc_names=["encode"])) + + assert len(mock_db.files.docs) == 5 + + @pytest.mark.asyncio + async def test__sync_encode_should_fail_when_the_trailing_batch_fails_to_commit( + self, mock_db, mocker, monkeypatch + ): + """Test a sink failure on a cleanly drained stream fails the sync. + + The flush of the trailing partial batch used to sit in a ``finally`` + that suppressed its own failure. A ``finally`` also runs when nothing + is propagating, so this reported a clean sync over a corpus short by + the trailing batch -- against a DCC cleared before the load. + + Given: + A batch size of 2 and a stream of 3 rows that drains cleanly, + whose trailing one-row batch fails to insert. + When: + _sync_encode runs. + Then: + It should raise and report only the two rows that committed. + """ + # Arrange + monkeypatch.setattr(sync_module, "BATCH_SIZE", 2) + rows = [ + _encode_metadata_row(f"ENCFF00{i}AAA", f"f{i}.bed.gz") for i in range(3) + ] + mocker.patch.object( + encode_module, "fetch_encode_metadata", lambda deadline=None: _async_iter(rows) + ) + _fail_insert_on_call(mock_db.files, 2, RuntimeError("mongo failover")) + task = SyncTask(id="t1", dcc_names=["encode"]) + + # Act & assert + with pytest.raises(RuntimeError): + await _sync_encode(task) + + assert len(mock_db.files.docs) == 2 + assert task.progress.startswith("ENCODE sync incomplete: 2 files") + + @pytest.mark.asyncio + async def test__sync_encode_should_not_resubmit_a_batch_the_sink_rejected( + self, mock_db, mocker, monkeypatch + ): + """Test a batch that failed to insert is not sent a second time. + + The buffer used to be cleared only after a successful insert, so a + failed one left the same list for the trailing flush to submit again. + + Given: + A batch size of 2 and a stream of 4 rows whose first batch fails + to insert. + When: + _sync_encode runs. + Then: + The rejected batch should never reach the collection, and the + reported count should exclude it. + """ + # Arrange + monkeypatch.setattr(sync_module, "BATCH_SIZE", 2) + rows = [ + _encode_metadata_row(f"ENCFF00{i}AAA", f"f{i}.bed.gz") for i in range(4) + ] + mocker.patch.object( + encode_module, "fetch_encode_metadata", lambda deadline=None: _async_iter(rows) + ) + _fail_insert_on_call(mock_db.files, 1, RuntimeError("mongo failover")) + task = SyncTask(id="t1", dcc_names=["encode"]) + + # Act & assert + with pytest.raises(RuntimeError): + await _sync_encode(task) + + assert mock_db.files.docs == [] + assert task.progress.startswith("ENCODE sync incomplete: 0 files") + + @pytest.mark.asyncio + async def test__sync_encode_should_give_every_phase_the_same_deadline( + self, mock_db, mocker, monkeypatch + ): + """Test the download budget bounds the sync rather than each stream. + + Every phase runs inside the one cutover lock that gates the read + surface, so a per-stream budget would multiply the outage by the + phase count and carry the worst case past the sync lock's one-hour + stale threshold -- at which point a second sync is admitted and + clears the corpus while this one is still writing to it. + + Given: + Two configured annotation types, so three phases run. + When: + _sync_encode runs. + Then: + Every phase should be handed one and the same deadline. + """ + # Arrange + monkeypatch.setenv("ENCODE_ANNOTATION_TYPES", "alpha,beta") + deadlines = [] + + def _experiment_stream(deadline=None): + deadlines.append(deadline) + return _async_iter([]) + + def _annotation_stream(annotation_type, deadline=None): + deadlines.append(deadline) + return _async_iter([]) + + mocker.patch.object( + encode_module, "fetch_encode_metadata", _experiment_stream + ) + mocker.patch.object( + encode_module, + "fetch_encode_annotation_metadata", + side_effect=_annotation_stream, + ) + + # Act + await _sync_encode(SyncTask(id="t1", dcc_names=["encode"])) + + # Assert + assert len(deadlines) == 3 + assert None not in deadlines + assert len(set(deadlines)) == 1 + + @pytest.mark.asyncio + async def test__sync_encode_should_ingest_a_repeated_type_only_once( + self, mock_db, mocker, monkeypatch + ): + """Test a duplicated allowlist entry does not double-load its type. + + ENCODE documents are written with insert_many into a collection + carrying no unique key, so a repeated token would insert every file + of that type twice with nothing to reject it. + + Given: + ENCODE_ANNOTATION_TYPES naming the same type twice. + When: + _sync_encode runs. + Then: + The annotation fetch should run once and each document should + appear once. + """ + # Arrange + monkeypatch.setenv("ENCODE_ANNOTATION_TYPES", "chromatin state,chromatin state") + mocker.patch.object( + encode_module, "fetch_encode_metadata", lambda deadline=None: _async_iter([]) + ) + fetch_annotations = mocker.patch.object( + encode_module, + "fetch_encode_annotation_metadata", + side_effect=lambda annotation_type, deadline=None: _async_iter( + [_encode_annotation_row("ENCFF002BBB", "ENCSR001AAA", "y.bed.gz")] + ), + ) + + # Act + await _sync_encode(SyncTask(id="t1", dcc_names=["encode"])) + + # Assert + assert fetch_annotations.call_count == 1 + assert [d["local_id"] for d in mock_db.files.docs] == ["ENCFF002BBB"] + + @pytest.mark.asyncio + async def test__sync_encode_should_report_a_configured_type_that_returned_nothing( + self, mock_db, mocker, monkeypatch, caplog + ): + """Test an empty annotation type is logged as zero, not omitted. + + A type ENCODE stops publishing is the failure this tally exists to + surface. An absent key is what nobody notices when diffing logs; a + zero is what everybody does. + + Given: + Two configured types, one returning rows and one returning + none. + When: + _sync_encode runs. + Then: + The annotation_type distribution should carry the empty type + with a count of zero. + """ + # Arrange + monkeypatch.setenv("ENCODE_ANNOTATION_TYPES", "populated,vanished") + mocker.patch.object( + encode_module, "fetch_encode_metadata", lambda deadline=None: _async_iter([]) + ) + + def _annotation_stream(annotation_type, deadline=None): + if annotation_type == "vanished": + return _async_iter([]) + return _async_iter( + [_encode_annotation_row("ENCFF002BBB", "ENCSR001AAA", "y.bed.gz")] + ) + + mocker.patch.object( + encode_module, + "fetch_encode_annotation_metadata", + side_effect=_annotation_stream, + ) + + # Act + with caplog.at_level(logging.INFO): + await _sync_encode(SyncTask(id="t1", dcc_names=["encode"])) + + # Assert + distribution = next( + m for m in caplog.messages if "annotation_type distribution" in m + ) + assert "'vanished': 0" in distribution + + @pytest.mark.asyncio + async def test__sync_encode_should_propagate_a_cancellation( + self, mock_db, mocker, monkeypatch + ): + """Test cancellation cancels the sync rather than failing one phase. + + The per-phase handler is deliberately broad, and a CancelledError + caught by it would be logged as a phase failure and followed by + every remaining phase -- the opposite of cancelling. + + Given: + An experiment stream raising CancelledError and a configured + annotation type. + When: + _sync_encode runs. + Then: + The CancelledError should propagate and the annotation phase + should never run. + """ + # Arrange + monkeypatch.setenv( + "ENCODE_ANNOTATION_TYPES", "candidate Cis-Regulatory Elements" + ) + mocker.patch.object( + encode_module, + "fetch_encode_metadata", + lambda deadline=None: _async_raise(asyncio.CancelledError()), + ) + fetch_annotations = mocker.patch.object( + encode_module, + "fetch_encode_annotation_metadata", + side_effect=lambda annotation_type, deadline=None: _async_iter([]), + ) + + # Act & assert + with pytest.raises(asyncio.CancelledError): + await _sync_encode(SyncTask(id="t1", dcc_names=["encode"])) + + fetch_annotations.assert_not_called() + + @pytest.mark.asyncio + async def test__sync_encode_should_not_commit_buffered_rows_when_cancelled( + self, mock_db, mocker, monkeypatch + ): + """Test a cancelled sync stops writing rather than flushing on its way out. + + The trailing-batch flush runs from a ``finally``, which a cancellation + unwinds through as readily as a failure. Writing there would commit + rows after the decision to stop, and a cancellation re-delivered by + that await would replace the original and skip the phase's tally. + + Given: + A batch size large enough to leave every row buffered, and a + stream that yields three rows and then raises CancelledError. + When: + _sync_encode runs. + Then: + The cancellation should propagate with the buffered rows never + committed. + """ + # Arrange + monkeypatch.setattr(sync_module, "BATCH_SIZE", 100) + rows = [ + _encode_metadata_row(f"ENCFF00{i}AAA", f"f{i}.bed.gz") for i in range(3) + ] + mocker.patch.object( + encode_module, + "fetch_encode_metadata", + lambda deadline=None: _async_iter_then_raise(rows, asyncio.CancelledError()), + ) + + # Act & assert + with pytest.raises(asyncio.CancelledError): + await _sync_encode(SyncTask(id="t1", dcc_names=["encode"])) + + assert mock_db.files.docs == [] + + @pytest.mark.asyncio + async def test__sync_encode_should_still_index_when_every_phase_fails( + self, mock_db, mocker, monkeypatch + ): + """Test the total-failure path leaves a coherent, queryable state. + + No phase delivered a row, so no phase replaced its slice: the corpus + is last sync's, in full, rather than empty. Emptying it would have + been the one outcome with nothing to recommend it -- the load failed, + so there is nothing to put in its place. + + Given: + An experiment stream and both configured annotation streams + all raising, over a pre-seeded stale ENCODE document. + When: + _sync_encode runs. + Then: + The stale document should survive, the indexes should still be + ensured, and the error should name all three phases. + """ + # Arrange + monkeypatch.setenv("ENCODE_ANNOTATION_TYPES", "alpha,beta") + mock_db.files.docs = [{"submission": "encode", "local_id": "STALE"}] + mocker.patch.object( + encode_module, + "fetch_encode_metadata", + lambda deadline=None: _async_raise(RuntimeError("experiment died")), + ) + mocker.patch.object( + encode_module, + "fetch_encode_annotation_metadata", + side_effect=lambda annotation_type, deadline=None: _async_raise( + RuntimeError(f"{annotation_type} died") + ), + ) + ensure = mocker.patch.object( + sync_module, "ensure_indexes", mocker.AsyncMock(return_value=0) + ) + task = SyncTask(id="t1", dcc_names=["encode"]) + + # Act & assert + with pytest.raises(RuntimeError) as excinfo: + await _sync_encode(task) + + assert [d["local_id"] for d in mock_db.files.docs] == ["STALE"] + assert ensure.await_count == 1 + for label in ("experiment", "annotation[alpha]", "annotation[beta]"): + assert label in str(excinfo.value) + assert "0 of 3 phases" in task.progress + + @pytest.mark.asyncio + async def test__sync_encode_should_chain_the_first_failure_as_the_cause( + self, mock_db, mocker, monkeypatch + ): + """Test the raised error keeps the first failure's traceback. + + Given: + Two failing phases raising distinguishable exceptions. + When: + _sync_encode runs. + Then: + The RuntimeError's __cause__ should be the first one, so the + traceback points at the failure that started the cascade. + """ + # Arrange + monkeypatch.setenv("ENCODE_ANNOTATION_TYPES", "alpha") + first = RuntimeError("experiment died first") + second = RuntimeError("annotation died second") + mocker.patch.object( + encode_module, "fetch_encode_metadata", lambda deadline=None: _async_raise(first) + ) + mocker.patch.object( + encode_module, + "fetch_encode_annotation_metadata", + side_effect=lambda annotation_type, deadline=None: _async_raise(second), + ) + + # Act & assert + with pytest.raises(RuntimeError) as excinfo: + await _sync_encode(SyncTask(id="t1", dcc_names=["encode"])) + + assert excinfo.value.__cause__ is first + + @pytest.mark.asyncio + async def test__sync_encode_should_not_report_a_partial_load_as_complete( + self, mock_db, mocker, monkeypatch + ): + """Test the task's own progress text admits a phase was lost. + + Given: + An experiment stream that raises and a healthy annotation + stream. + When: + _sync_encode runs. + Then: + The task's final progress should say the sync was incomplete, + rather than reporting a clean sync over a partial corpus. + """ + # Arrange + monkeypatch.setenv( + "ENCODE_ANNOTATION_TYPES", "candidate Cis-Regulatory Elements" + ) + mocker.patch.object( + encode_module, + "fetch_encode_metadata", + lambda deadline=None: _async_raise(RuntimeError("experiment stream died")), + ) + mocker.patch.object( + encode_module, + "fetch_encode_annotation_metadata", + lambda annotation_type, deadline=None: _async_iter( + [_encode_annotation_row("ENCFF002BBB", "ENCSR001AAA", "y.bed.gz")] + ), + ) + task = SyncTask(id="t1", dcc_names=["encode"]) + + # Act & assert + with pytest.raises(RuntimeError): + await _sync_encode(task) + + assert "incomplete" in task.progress + + @pytest.mark.asyncio + @pytest.mark.parametrize( + "row_count, expected_inserts", + [(3, 1), (4, 2), (0, 0)], + ids=["exactly-one-batch", "one-batch-plus-remainder", "empty-stream"], + ) + async def test__sync_encode_should_insert_one_batch_per_batch_size_rows( + self, mock_db, mocker, monkeypatch, row_count, expected_inserts + ): + """Test the batching boundary commits every row exactly once. + + The three cases are the boundary itself, the remainder path, and + the empty stream that must not issue a trailing empty insert. + + Given: + A batch size of 3 and a stream of 3, 4 or 0 rows. + When: + _sync_encode runs. + Then: + insert_many should be called the expected number of times and + every row should land exactly once. + """ + # Arrange + monkeypatch.setattr(sync_module, "BATCH_SIZE", 3) + rows = [ + _encode_metadata_row(f"ENCFF00{i}AAA", f"f{i}.bed.gz") + for i in range(row_count) + ] + mocker.patch.object( + encode_module, "fetch_encode_metadata", lambda deadline=None: _async_iter(rows) + ) + # Spied, not asserted on by argument: _ingest_encode_rows clears the + # same list it hands to insert_many, so a recorded call reads back + # empty. + spy = mocker.spy(mock_db.files, "insert_many") + + # Act + await _sync_encode(SyncTask(id="t1", dcc_names=["encode"])) + + # Assert + assert spy.call_count == expected_inserts + assert len(mock_db.files.docs) == row_count + + @pytest.mark.asyncio + async def test__sync_encode_should_not_count_a_row_it_skipped( + self, mock_db, mocker, caplog + ): + """Test an untransformable row is absent from the corpus and tallies. + + Given: + Three experiment rows where the middle one has no File + accession and so transforms to None. + When: + _sync_encode runs. + Then: + Two documents should be inserted and the compression + distribution should account for two, not three. + """ + # Arrange + rows = [ + _encode_metadata_row("ENCFF001AAA", "a.bed.gz"), + {"File accession": " ", "File format": "bed"}, + _encode_metadata_row("ENCFF003CCC", "c.bed.gz"), + ] + mocker.patch.object( + encode_module, "fetch_encode_metadata", lambda deadline=None: _async_iter(rows) + ) + task = SyncTask(id="t1", dcc_names=["encode"]) + + # Act + with caplog.at_level(logging.INFO): + await _sync_encode(task) + + # Assert + assert len(mock_db.files.docs) == 2 + assert task.progress == "ENCODE sync complete: 2 files" + distribution = next( + m for m in caplog.messages if "compression_format distribution" in m + ) + assert "'format:3989': 2" in distribution + + @pytest.mark.asyncio + async def test__sync_encode_should_not_let_a_phase_clear_another_phases_rows( + self, mock_db, mocker, monkeypatch + ): + """Test each phase's clear reaches only the slice that phase reloads. + + Every phase clears before loading, so a filter that selected more + than its own slice would delete the preceding phases' documents as + each new one started, leaving only the last phase's rows. + + Given: + A stale ENCODE document and three healthy phases. + When: + _sync_encode runs. + Then: + The stale document should be gone and all three phases' + documents should survive together. + """ + # Arrange + monkeypatch.setenv("ENCODE_ANNOTATION_TYPES", "alpha,beta") + mock_db.files.docs = [{"submission": "encode", "local_id": "STALE"}] + mocker.patch.object( + encode_module, + "fetch_encode_metadata", + lambda deadline=None: _async_iter([_encode_metadata_row("ENCFF001AAA", "a.bed.gz")]), + ) + mocker.patch.object( + encode_module, + "fetch_encode_annotation_metadata", + side_effect=lambda annotation_type, deadline=None: _async_iter( + [ + _encode_annotation_row( + f"ENCFF_{annotation_type}", "ENCSR001AAA", "y.bed.gz" + ) + ] + ), + ) + + # Act + await _sync_encode(SyncTask(id="t1", dcc_names=["encode"])) + + # Assert + assert {d["local_id"] for d in mock_db.files.docs} == { + "ENCFF001AAA", + "ENCFF_alpha", + "ENCFF_beta", + } + + @pytest.mark.asyncio + async def test__sync_encode_should_keep_a_failed_phases_rows_until_it_reloads_them( + self, mock_db, mocker, monkeypatch + ): + """Test a failed phase serves stale data rather than none. + + One corpus-wide clear before the fan-out destroyed every phase's data + before any failure was knowable, so a failed experiment phase left + the API serving the annotation documents as the whole corpus. The + experiment phase is the largest and slowest stream, so it is the + likeliest to be the one that fails. + + Given: + A stale experiment document, an experiment phase that fails + before yielding, and a healthy annotation phase. + When: + _sync_encode runs. + Then: + The stale experiment document should survive alongside the + freshly loaded annotation document. + """ + # Arrange + monkeypatch.setenv("ENCODE_ANNOTATION_TYPES", "alpha") + mock_db.files.docs = [{"submission": "encode", "local_id": "STALE_EXPERIMENT"}] + mocker.patch.object( + encode_module, + "fetch_encode_metadata", + lambda deadline=None: _async_raise(RuntimeError("experiment died")), + ) + mocker.patch.object( + encode_module, + "fetch_encode_annotation_metadata", + side_effect=lambda annotation_type, deadline=None: _async_iter( + [_encode_annotation_row("ENCFF_alpha", "ENCSR001AAA", "y.bed.gz")] + ), + ) + + # Act & assert + with pytest.raises(RuntimeError): + await _sync_encode(SyncTask(id="t1", dcc_names=["encode"])) + + assert {d["local_id"] for d in mock_db.files.docs} == { + "STALE_EXPERIMENT", + "ENCFF_alpha", + } + + @pytest.mark.asyncio + async def test__sync_encode_should_empty_a_slice_whose_stream_returned_nothing( + self, mock_db, mocker, monkeypatch + ): + """Test a type that stopped being published stops being served. + + The clear is deferred until a phase has rows to put back, so that a + phase failing before it delivers any leaves its previous rows alone. + A stream that drains cleanly with nothing in it is the other case + entirely: the empty result is the answer, and keeping last sync's + rows would serve documents ENCODE no longer publishes. + + Given: + A stale document for a configured type whose stream then yields + no rows at all. + When: + _sync_encode runs. + Then: + The stale document should be gone. + """ + # Arrange + monkeypatch.setenv("ENCODE_ANNOTATION_TYPES", "alpha") + mock_db.files.docs = [ + { + "submission": "encode", + "local_id": "STALE_ALPHA", + "extra": {"encode": {"annotation_type": "alpha"}}, + } + ] + mocker.patch.object( + encode_module, "fetch_encode_metadata", lambda deadline=None: _async_iter([]) + ) + mocker.patch.object( + encode_module, + "fetch_encode_annotation_metadata", + side_effect=lambda annotation_type, deadline=None: _async_iter([]), + ) + + # Act + await _sync_encode(SyncTask(id="t1", dcc_names=["encode"])) + + # Assert + assert mock_db.files.docs == [] + + @pytest.mark.asyncio + async def test__sync_encode_should_record_a_failed_phase_as_zero_in_the_per_phase_log( + self, mock_db, mocker, monkeypatch, caplog + ): + """Test the per-phase log distinguishes what loaded from what did not. + + Given: + One healthy annotation phase and one that fails before + yielding. + When: + _sync_encode runs. + Then: + The per-phase counts should name the healthy phase and record + the failed one as having loaded nothing. + """ + # Arrange + monkeypatch.setenv("ENCODE_ANNOTATION_TYPES", "healthy,broken") + mocker.patch.object( + encode_module, "fetch_encode_metadata", lambda deadline=None: _async_iter([]) + ) + + def _annotation_stream(annotation_type, deadline=None): + if annotation_type == "broken": + return _async_raise(RuntimeError("died")) + return _async_iter( + [_encode_annotation_row("ENCFF002BBB", "ENCSR001AAA", "y.bed.gz")] + ) + + mocker.patch.object( + encode_module, + "fetch_encode_annotation_metadata", + side_effect=_annotation_stream, + ) + + # Act + with caplog.at_level(logging.INFO): + with pytest.raises(RuntimeError): + await _sync_encode(SyncTask(id="t1", dcc_names=["encode"])) + + # Assert + per_phase = next(m for m in caplog.messages if "inserted per phase" in m) + assert "'annotation[healthy]': 1" in per_phase + assert "'annotation[broken]': 0" in per_phase + + @pytest.mark.asyncio + @settings( + max_examples=40, + deadline=None, + suppress_health_check=[HealthCheck.function_scoped_fixture], + ) + @given( + # One (row count, fails) pair per annotation phase, plus one for the + # experiment phase, so the fan-out and the failure pattern both vary. + phase_specs=st.lists( + st.tuples(st.integers(min_value=0, max_value=6), st.booleans()), + min_size=1, + max_size=4, + ), + batch_size=st.integers(min_value=1, max_value=4), + ) + async def test__sync_encode_should_report_exactly_what_it_loaded( + self, mocker, monkeypatch, phase_specs, batch_size + ): + """Test the reported total always equals the documents committed. + + The invariant the mid-stream accounting defect broke, over + arbitrary fan-out and failure patterns rather than the handful of + shapes the example tests fix. + + Given: + Any number of phases with any row counts, any subset of which + die after yielding all their rows, at any batch size. + When: + _sync_encode runs. + Then: + The count in the final progress should equal the ENCODE + documents in the collection. + """ + # Arrange + # A fresh database per generated example rather than the function + # scoped ``mock_db`` fixture, which Hypothesis does not reset between + # examples -- documents would accumulate across them. + db = FakeDB() + monkeypatch.setattr(api, "db", db) + monkeypatch.setattr(sync_module, "BATCH_SIZE", batch_size) + experiment_spec, *annotation_specs = phase_specs + monkeypatch.setenv( + "ENCODE_ANNOTATION_TYPES", + ",".join(f"type{i}" for i in range(len(annotation_specs))), + ) + + def _stream(prefix, spec): + count, fails = spec + rows = [ + _encode_metadata_row(f"ENCFF{prefix}{i:03d}", f"f{i}.bed.gz") + for i in range(count) + ] + if fails: + return _async_iter_then_raise(rows, RuntimeError("died")) + return _async_iter(rows) + + mocker.patch.object( + encode_module, + "fetch_encode_metadata", + lambda deadline=None: _stream("E", experiment_spec), + ) + mocker.patch.object( + encode_module, + "fetch_encode_annotation_metadata", + side_effect=lambda annotation_type, deadline=None: _stream( + annotation_type[-1], annotation_specs[int(annotation_type[-1])] + ), + ) + task = SyncTask(id="t1", dcc_names=["encode"]) + any_failure = any(fails for _, fails in phase_specs) + + # Act + if any_failure: + with pytest.raises(RuntimeError): + await _sync_encode(task) + else: + await _sync_encode(task) + + # Assert + committed = len([d for d in db.files.docs if d["submission"] == "encode"]) + # Anchored rather than a substring. The generated counts reach into + # the twenties, so "6 files" would be satisfied by a reported + # "16 files" -- and this assertion is the one standing guard over + # the reported half of the accounting invariant. + state = "incomplete" if any_failure else "complete" + assert task.progress.startswith(f"ENCODE sync {state}: {committed} files") + assert committed == sum(count for count, _ in phase_specs) + + @pytest.mark.asyncio + async def test__sync_encode_should_run_the_default_allowlist_when_unset( + self, mock_db, mocker, monkeypatch + ): + """Test the default allowlist reaches the sync, not just the parser. + + Every other test in this class pins the variable, so without this + one nothing joins ``annotation_types_from_env``'s default to the + phases actually run. + + Given: + No ENCODE_ANNOTATION_TYPES in the environment. + When: + _sync_encode runs. + Then: + It should fetch exactly the two default annotation types. + """ + # Arrange + monkeypatch.delenv("ENCODE_ANNOTATION_TYPES", raising=False) + mocker.patch.object( + encode_module, "fetch_encode_metadata", lambda deadline=None: _async_iter([]) + ) + fetch_annotations = mocker.patch.object( + encode_module, + "fetch_encode_annotation_metadata", + side_effect=lambda annotation_type, deadline=None: _async_iter([]), + ) + + # Act + await _sync_encode(SyncTask(id="t1", dcc_names=["encode"])) + + # Assert + assert [call.args[0] for call in fetch_annotations.call_args_list] == [ + "candidate Cis-Regulatory Elements", + "element gene regulatory interaction predictions", + ] + + @pytest.mark.asyncio + async def test__sync_encode_should_skip_annotations_when_the_allowlist_is_empty( + self, mock_db, mocker, monkeypatch + ): + """Test that an explicitly empty allowlist disables the fetch entirely. + + Given: + ENCODE_ANNOTATION_TYPES set to an empty value. + When: + _sync_encode runs. + Then: + No annotation request should be made, and the experiment ingest + should complete normally. + """ + # Arrange + monkeypatch.setenv("ENCODE_ANNOTATION_TYPES", "") + mocker.patch.object( + encode_module, + "fetch_encode_metadata", + lambda deadline=None: _async_iter([_encode_metadata_row("ENCFF001AAA", "x.bed.gz")]), + ) + fetch_annotations = mocker.patch.object( + encode_module, + "fetch_encode_annotation_metadata", + side_effect=lambda annotation_type, deadline=None: _async_iter([]), + ) + + # Act + await _sync_encode(SyncTask(id="t1", dcc_names=["encode"])) + + # Assert + fetch_annotations.assert_not_called() + assert [d["local_id"] for d in mock_db.files.docs] == ["ENCFF001AAA"] + + class TestStamp4dnFileAccessions: @pytest.mark.asyncio async def test__stamp_4dn_file_accessions_should_write_the_raw_file_collection( @@ -1049,7 +2253,7 @@ async def test__log_accession_coverage_should_warn_when_nothing_is_covered( # Assert assert "will return no matches" in caplog.text -class TestSyncEncodeIndexes: +class TestSyncEncodeIndexes(_EncodeSyncTestBase): @pytest.mark.asyncio async def test__sync_encode_should_ensure_the_accession_indexes( self, mocker, mock_db @@ -1072,7 +2276,7 @@ async def test__sync_encode_should_ensure_the_accession_indexes( # Arrange row = _encode_metadata_row("encff001aaa", "encff001aaa.bed.gz") mocker.patch.object( - encode_module, "fetch_encode_metadata", lambda: _async_iter([row]) + encode_module, "fetch_encode_metadata", lambda deadline=None: _async_iter([row]) ) # Act From c53c5742e645c53dac80d6174e2c6a3f16749eb4 Mon Sep 17 00:00:00 2001 From: Conrad Date: Thu, 13 Aug 2026 14:07:33 -0400 Subject: [PATCH 11/15] test: Cover the annotation filter paths and facets A filter is only useful if the path it flattens to is the path the ingest writes. A mismatch there would leave the whole annotation corpus unfilterable while every ingest test still passed, so each of the eight new fields is asserted from the GraphQL input through to its dotted predicate -- including the five-segment biosample path, which crosses two array levels and is the one most easily got wrong. A property test pins that none of them is folded the way accessions are. ENCODE's vocabulary is case- and space-significant, so folding any of them would make the documents permanently unmatchable with nothing raising. The end-to-end tests then ask the questions a client would: give me the cCRE files, narrow them to mouse, narrow them to mm10, and read the fields back out. The distinct-values tests pin that the annotation types and the assemblies can both be enumerated, since filtering by either is only useful if the vocabulary is discoverable. The index spec tests split by concern -- the accession paths, the annotation facets, the core assembly field -- so each claim stays readable now that the list covers more than accessions. A separate assertion pins the full set by equality, because containment alone lets an unintended spec merge unnoticed into a list that runs createIndex against a public collection on every sync. --- tests/test_indexes.py | 86 ++++++++++++- tests/test_inputs.py | 168 ++++++++++++++++++++++++- tests/test_schema.py | 286 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 538 insertions(+), 2 deletions(-) diff --git a/tests/test_indexes.py b/tests/test_indexes.py index a4cf19a..97bfcc9 100644 --- a/tests/test_indexes.py +++ b/tests/test_indexes.py @@ -656,6 +656,37 @@ def test_materialized_files_index_specs_should_not_overlap_the_data_specs(): assert {s.collection for s in materialized_files_index_specs()} == {"files"} +def test_materialized_files_index_specs_should_declare_exactly_these_keys(): + """Test the spec list stays a deliberate enumeration. + + The per-concern tests below assert subsets, which reads well but lets an + unintended spec merge unnoticed -- and this list is applied with + ``createIndex`` against a public collection on every ENCODE sync, so an + accidental index is a build cost and a write cost that nothing else + would flag. ``test_all_index_specs_should_not_repeat_a_collection_and_name`` + catches only duplicate names, not extra distinct ones. + + Given: + The materialized files index specs. + When: + Their key tuples are collected. + Then: + They should be exactly the declared set, with no additions. + """ + # Act + keys = {spec.keys for spec in materialized_files_index_specs()} + + # Assert + assert keys == { + (("accession_id", 1),), + (("collections.accession_id", 1),), + (("extra.encode.annotation_type", 1),), + (("extra.encode.organism", 1),), + (("extra.encode.assembly", 1),), + (("genome_assembly", 1),), + } + + def test_materialized_files_index_specs_should_cover_both_accession_paths(): """Test that both queryable accession paths are indexed. @@ -674,7 +705,60 @@ def test_materialized_files_index_specs_should_cover_both_accession_paths(): keys = {spec.keys for spec in materialized_files_index_specs()} # Assert - assert keys == {(("accession_id", 1),), (("collections.accession_id", 1),)} + assert { + (("accession_id", 1),), + (("collections.accession_id", 1),), + } <= keys + + +def test_materialized_files_index_specs_should_cover_the_annotation_facets(): + """Test that the ENCODE annotation filters are indexed. + + ``annotation_type`` is the field the whole annotation corpus is meant + to be reached through, and ``files`` is written directly by the ENCODE + sync, so nothing else would create these. Unindexed, each is a full + scan of the collection on an unauthenticated endpoint. + + Given: + The materialized files index specs. + When: + Their key tuples are collected. + Then: + They should cover the annotation type, organism and assembly paths + that ``to_query`` emits for an ``extra.encode`` filter. + """ + # Act + keys = {spec.keys for spec in materialized_files_index_specs()} + + # Assert + assert { + (("extra.encode.annotation_type", 1),), + (("extra.encode.organism", 1),), + (("extra.encode.assembly", 1),), + } <= keys + + +def test_materialized_files_index_specs_should_cover_the_core_assembly_field(): + """Test the documented assembly filter is indexed, not just its mirror. + + ``genome_assembly`` is the field the schema publishes and the one a + client narrowing to GRCh38 filters on; ``extra.encode.assembly`` is the + DCC mirror. The materializer indexes the core field, but an ENCODE-only + database never runs the materializer, so without this the documented + filter is the one left scanning. + + Given: + The materialized files index specs. + When: + Their key tuples are collected. + Then: + They should cover the core ``genome_assembly`` path. + """ + # Act + keys = {spec.keys for spec in materialized_files_index_specs()} + + # Assert + assert (("genome_assembly", 1),) in keys def test_all_index_specs_should_not_repeat_a_collection_and_name(): diff --git a/tests/test_inputs.py b/tests/test_inputs.py index ffccfca..f301d89 100644 --- a/tests/test_inputs.py +++ b/tests/test_inputs.py @@ -1,8 +1,21 @@ +import pytest from hypothesis import given, settings from hypothesis import strategies as st from cfdb.accessions import normalize_accession -from cfdb.api.gql.inputs import CollectionInput, FileMetadataInput, to_dict, to_query +from cfdb.api.gql.inputs import ( + BiosampleInput, + CollectionInput, + EnrichedBiosampleInput, + EnrichedCollectionInput, + EnrichedEncodeBiosampleInput, + EnrichedEncodeCollectionInput, + EnrichedEncodeFileInput, + EnrichedFileInput, + FileMetadataInput, + to_dict, + to_query, +) #: Alphabet the DCCs actually issue accessions from. _ACCESSION_CHARS = "ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789" @@ -383,6 +396,159 @@ def test_to_query_should_accept_accession_id_from_the_graphql_inputs(): } +#: Every path the ENCODE annotation ingest writes, paired with the filter +#: that must reach it. A mismatch between the two would leave the whole +#: annotation corpus unfilterable while every ingest test still passed, so +#: these are asserted against the paths ``transform_annotation_to_c2m2`` +#: actually emits rather than against the schema alone. +_ANNOTATION_FILTER_PATHS = [ + ("annotation_type", "extra.encode.annotation_type"), + ("organism", "extra.encode.organism"), + ("assembly", "extra.encode.assembly"), +] + + +@pytest.mark.parametrize("field, path", _ANNOTATION_FILTER_PATHS) +def test_to_query_should_reach_the_file_level_encode_fields(field, path): + """Test a file-level ENCODE filter flattens to the stored path. + + Given: + A FileMetadataInput naming one of the ENCODE file-level fields. + When: + The input is converted with to_dict and then to_query. + Then: + It should produce a predicate on the dotted path the ingest + writes. + """ + # Arrange + payload = FileMetadataInput( + extra=[EnrichedFileInput(encode=[EnrichedEncodeFileInput(**{field: ["v"]})])] + ) + + # Act + query = to_query(to_dict(payload)) + + # Assert + assert query == {path: "v"} + + +@pytest.mark.parametrize( + "field, path", + [ + ("annotation_type", "collections.extra.encode.annotation_type"), + ("software_used", "collections.extra.encode.software_used"), + ("encyclopedia_version", "collections.extra.encode.encyclopedia_version"), + ], +) +def test_to_query_should_reach_the_dataset_level_encode_fields(field, path): + """Test a dataset-level ENCODE filter flattens to the stored path. + + Given: + A FileMetadataInput naming one of the ENCODE dataset-level fields + inside a nested CollectionInput. + When: + The input is converted with to_dict and then to_query. + Then: + It should produce a predicate on the dotted path the ingest + writes. + """ + # Arrange + payload = FileMetadataInput( + collections=[ + CollectionInput( + extra=[ + EnrichedCollectionInput( + encode=[EnrichedEncodeCollectionInput(**{field: ["v"]})] + ) + ] + ) + ] + ) + + # Act + query = to_query(to_dict(payload)) + + # Assert + assert query == {path: "v"} + + +@pytest.mark.parametrize("field", ["life_stage", "age", "age_units"]) +def test_to_query_should_reach_the_biosample_level_encode_fields(field): + """Test the deepest ENCODE filter flattens to the stored path. + + Five segments through two array levels -- the shape most likely to be + got wrong, and the one a client would notice last. + + Given: + A FileMetadataInput naming a donor-trait field nested under + collections.biosamples. + When: + The input is converted with to_dict and then to_query. + Then: + It should produce a predicate on the dotted path the ingest + writes. + """ + # Arrange + payload = FileMetadataInput( + collections=[ + CollectionInput( + biosamples=[ + BiosampleInput( + extra=[ + EnrichedBiosampleInput( + encode=[ + EnrichedEncodeBiosampleInput(**{field: ["v"]}) + ] + ) + ] + ) + ] + ) + ] + ) + + # Act + query = to_query(to_dict(payload)) + + # Assert + assert query == {f"collections.biosamples.extra.encode.{field}": "v"} + + +@given( + value=st.text(min_size=1, max_size=40), + field_and_path=st.sampled_from(_ANNOTATION_FILTER_PATHS), +) +@settings(max_examples=100) +def test_to_query_should_not_fold_an_encode_annotation_filter(value, field_and_path): + """Test the annotation filters match byte-exactly. + + ENCODE's vocabulary is case- and space-significant -- "candidate + Cis-Regulatory Elements" is stored exactly as published -- so folding + any of these the way accessions are folded would make them + permanently unmatchable. + + Given: + Any text value on any ENCODE file-level annotation field. + When: + to_query builds the predicate. + Then: + It should carry the value unaltered under its full dotted path. + """ + # Arrange + field, path = field_and_path + payload = FileMetadataInput( + extra=[ + EnrichedFileInput(encode=[EnrichedEncodeFileInput(**{field: [value]})]) + ] + ) + + # Act + query = to_query(to_dict(payload)) + + # Assert + assert query == {path: value} + + def test_to_query_should_collapse_a_single_clause(): """Test that one set field yields a bare predicate, not a wrapper. diff --git a/tests/test_schema.py b/tests/test_schema.py index 78c5a4b..e0cd244 100644 --- a/tests/test_schema.py +++ b/tests/test_schema.py @@ -96,6 +96,34 @@ def _make_file_doc( } +def _make_annotation_doc( + local_id: str, + annotation_type: str = "candidate Cis-Regulatory Elements", + organism: str = "Homo sapiens", + genome_assembly: str = "GRCh38", +) -> dict: + """Return an ENCODE annotation file document as the sync writes one.""" + return { + "id_namespace": "ns", + "local_id": local_id, + "project_id_namespace": "ns", + "project_local_id": "proj", + "filename": f"{local_id}.bed.gz", + "submission": "encode", + "data_access_level": "public", + "genome_assembly": genome_assembly, + "dcc": {"dcc_name": "ENCODE", "dcc_abbreviation": "encode"}, + "collections": [], + "extra": { + "encode": { + "annotation_type": annotation_type, + "organism": organism, + "assembly": genome_assembly, + } + }, + } + + def _named_type(type_ref: dict) -> str | None: """Unwrap an introspection type reference down to its named type.""" while type_ref is not None and type_ref.get("name") is None: @@ -912,6 +940,174 @@ async def test_files_should_return_the_uncompressed_sentinel_as_an_empty_string( } assert returned == {"f1": "", "f2": "format:3989"} + @pytest.mark.asyncio + async def test_files_should_filter_documents_by_annotation_type(self, mock_db): + """Test a client can ask for cCRE files without matching filenames. + + The headline promise of the annotation ingest, asserted through + the same GraphQL surface a client uses rather than at the + transform. + + Given: + Two cCRE files, one interaction-prediction file, and one + experiment file carrying no annotation type. + When: + The files query filters on the cCRE annotation type. + Then: + It should return only the two cCRE files. + """ + # Arrange + mock_db.files.docs = [ + _make_annotation_doc("f1", "candidate Cis-Regulatory Elements"), + _make_annotation_doc("f2", "candidate Cis-Regulatory Elements"), + _make_annotation_doc( + "f3", "element gene regulatory interaction predictions" + ), + _make_file_doc("f4", submission="encode"), + ] + + # Act + result = await schema.execute( + """ + query { + files(input: [{ extra: [{ encode: [{ + annotationType: ["candidate Cis-Regulatory Elements"] + }] }] }]) { + totalCount + items { localId } + } + } + """ + ) + + # Assert + assert result.errors is None + assert result.data["files"]["totalCount"] == 2 + assert result.data["files"]["items"] == [{"localId": "f1"}, {"localId": "f2"}] + + @pytest.mark.asyncio + async def test_files_should_filter_documents_by_organism(self, mock_db): + """Test a multi-organism annotation result set can be narrowed. + + The released cCREs span human and mouse, and an annotation row + names no donor -- so without the file-level organism there is + nothing to narrow on but the filename. + + Given: + A human cCRE file and a mouse one. + When: + The files query filters on the mouse organism. + Then: + It should return only the mouse file. + """ + # Arrange + mock_db.files.docs = [ + _make_annotation_doc("f1", organism="Homo sapiens"), + _make_annotation_doc("f2", organism="Mus musculus"), + ] + + # Act + result = await schema.execute( + """ + query { + files(input: [{ extra: [{ encode: [{ + organism: ["Mus musculus"] + }] }] }]) { + totalCount + items { localId } + } + } + """ + ) + + # Assert + assert result.errors is None + assert result.data["files"]["items"] == [{"localId": "f2"}] + + @pytest.mark.asyncio + async def test_files_should_narrow_an_annotation_type_by_assembly(self, mock_db): + """Test the multi-assembly result set the issue describes. + + A client asking for cCREs unqualified gets GRCh38, mm10 and hg19 + together; both fields have to compose for that to be narrowable. + + Given: + cCRE files under GRCh38, mm10 and hg19, plus an mm10 file of a + different annotation type. + When: + The files query filters on the cCRE type and the mm10 + assembly together. + Then: + It should return only the mm10 cCRE file. + """ + # Arrange + mock_db.files.docs = [ + _make_annotation_doc("f1", genome_assembly="GRCh38"), + _make_annotation_doc("f2", genome_assembly="mm10"), + _make_annotation_doc("f3", genome_assembly="hg19"), + _make_annotation_doc( + "f4", + annotation_type="element gene regulatory interaction predictions", + genome_assembly="mm10", + ), + ] + + # Act + result = await schema.execute( + """ + query { + files(input: [{ + genomeAssembly: ["mm10"], + extra: [{ encode: [{ + annotationType: ["candidate Cis-Regulatory Elements"] + }] }] + }]) { + totalCount + items { localId } + } + } + """ + ) + + # Assert + assert result.errors is None + assert result.data["files"]["items"] == [{"localId": "f2"}] + + @pytest.mark.asyncio + async def test_files_should_serialize_the_annotation_fields(self, mock_db): + """Test the annotation fields are readable, not only filterable. + + Given: + One annotation file document. + When: + The files query selects its ENCODE extra fields. + Then: + It should return the annotation type, organism and assembly. + """ + # Arrange + mock_db.files.docs = [_make_annotation_doc("f1")] + + # Act + result = await schema.execute( + """ + query { + files { + items { + extra { encode { annotationType organism assembly } } + } + } + } + """ + ) + + # Assert + assert result.errors is None + assert result.data["files"]["items"][0]["extra"]["encode"] == { + "annotationType": "candidate Cis-Regulatory Elements", + "organism": "Homo sapiens", + "assembly": "GRCh38", + } + @pytest.mark.asyncio async def test_files_should_filter_documents_by_compression_format(self, mock_db): """Test that the derived term is queryable through the input filter. @@ -1298,6 +1494,96 @@ def _patch_cutover(self, mocker): """No-op ``locks.wait_for_cutover`` for every test in this class.""" mocker.patch.object(locks, "wait_for_cutover", return_value=None) + @pytest.mark.asyncio + async def test_distinct_values_should_enumerate_the_annotation_types( + self, mock_db + ): + """Test a client can discover which annotation types exist. + + Filtering by annotation type is only useful if the vocabulary is + discoverable -- otherwise a client has to already know the exact + ENCODE spelling, which is precisely the string-matching the + annotation ingest set out to replace. Low cardinality, so it + belongs in the allowlist where accessions do not. + + Given: + Files spanning two annotation types and one with none. + When: + distinctValues is asked for extra.encode.annotation_type. + Then: + It should return the two types. + """ + # Arrange + mock_db.files.docs = [ + _make_annotation_doc("f1", "candidate Cis-Regulatory Elements"), + _make_annotation_doc("f2", "candidate Cis-Regulatory Elements"), + _make_annotation_doc( + "f3", "element gene regulatory interaction predictions" + ), + _make_distinct_doc("f4", "ENCODE", "encode"), + ] + + # Act + result = await schema.execute( + """ + query { + distinctValues(fields: ["extra.encode.annotation_type"]) { + field + values + } + } + """ + ) + + # Assert + assert result.errors is None + entry = result.data["distinctValues"][0] + assert sorted(entry["values"]) == [ + "candidate Cis-Regulatory Elements", + "element gene regulatory interaction predictions", + ] + + @pytest.mark.asyncio + async def test_distinct_values_should_enumerate_the_assemblies(self, mock_db): + """Test a client can discover which assemblies exist. + + Assembly is named alongside organism in the same acceptance + criterion, and cCREs are published against several of them, so + narrowing to one is the first move a client makes -- which it can + only do knowing the vocabulary. Same small closed vocabulary as its + two sibling facets. + + Given: + ENCODE files spanning two assemblies. + When: + distinctValues is asked for extra.encode.assembly. + Then: + It should return both assemblies. + """ + # Arrange + mock_db.files.docs = [ + _make_annotation_doc("f1", genome_assembly="GRCh38"), + _make_annotation_doc("f2", genome_assembly="GRCh38"), + _make_annotation_doc("f3", genome_assembly="mm10"), + ] + + # Act + result = await schema.execute( + """ + query { + distinctValues(fields: ["extra.encode.assembly"]) { + field + values + } + } + """ + ) + + # Assert + assert result.errors is None + entry = result.data["distinctValues"][0] + assert sorted(entry["values"]) == ["GRCh38", "mm10"] + @pytest.mark.asyncio @pytest.mark.parametrize( "field", ["accession_id", "collections.accession_id"] From cbec04808135a4c0613492443399630a427fa6bb Mon Sep 17 00:00:00 2001 From: Conrad Date: Thu, 13 Aug 2026 14:07:52 -0400 Subject: [PATCH 12/15] test: Cover paired-interval formats through data and index The point of giving bedpe and bigInteract their own format terms is what the routes do with them, and that was asserted only at the mapping table. Both routes are now pinned against a registry wired the way the API wires it: a data request reaches the direct streaming path without dispatching a workflow, and an index request returns 404 without consulting the executor at all. Serving the raw file is the correct outcome here rather than a degradation. The alternative these tests rule out is the previous behavior, where the pipeline claimed the file and committed an index built from the first locus of each record. Both fixtures inject the minted term rather than producing it, and each carries a comment saying so. The format table has one consumer, the ENCODE transform, so the 4DN ingest cannot mint it and a real 4DN .bedpe still arrives declaring BED. The assertions hold as registry-routing pins, since lookup keys on the format name irrespective of DCC, but read as evidence that 4DN paired-interval files are safe they would be wrong. --- tests/test_data.py | 88 +++++++++++++++++++++++++++++++++++++++++++++ tests/test_index.py | 61 +++++++++++++++++++++++++++++++ 2 files changed, 149 insertions(+) diff --git a/tests/test_data.py b/tests/test_data.py index dd1277b..d27bb08 100644 --- a/tests/test_data.py +++ b/tests/test_data.py @@ -12,8 +12,10 @@ from cfdb import api from cfdb.api.routers.data import stream_file, stream_file_status from cfdb.services import drs, locks +from cfdb.services.ontology_mappings import get_file_format from cfdb.workflows.executor import WoolExecutor from cfdb.workflows.processors.bam import BamIndexProcessor +from cfdb.workflows.processors.tabix import TabixIntervalProcessor from tests.test_workflows import FIXTURE_MD5 from cfdb.workflows.processors.registry import ProcessorRegistry, default_registry @@ -592,6 +594,92 @@ async def fake_drs(*args, **kwargs): ) + @pytest.mark.asyncio + @pytest.mark.parametrize("format_name", ["bedpe", "bigInteract"]) + async def test_stream_file_should_serve_a_paired_interval_format_unprocessed( + self, mock_db, mocker, format_name + ): + """Test a paired-interval file reaches the direct streaming path. + + While these formats were aliased to BED and bigBed the tabix + pipeline claimed them and committed an index built from the first + locus of each record. Now that each carries its own format name no + processor claims it, so the request must fall through to the + upstream file rather than dispatch a workflow that would produce + the wrong artifact. + + Given: + A file in a paired-interval format and a wired workflow + subsystem carrying the processors the API registers. + When: + stream_file is called. + Then: + It should reach the direct DRS streaming path rather than + returning a 202 workflow dispatch. + """ + # Arrange + mocker.patch.object(locks, "wait_for_cutover", return_value=None) + registry = default_registry() + registry.register(BamIndexProcessor()) + registry.register(TabixIntervalProcessor()) + mocker.patch.object(api, "processor_registry", registry) + + class _Cache: + async def head(self, _k): + raise AssertionError("an unclaimed format must not consult cache") + + def get(self, *_a, **_kw): + raise AssertionError + + async def put(self, *_a, **_kw): + raise AssertionError + + async def delete(self, _k): + return False + + mocker.patch.object(api, "cache", _Cache()) + mocker.patch.object(api, "executor", object()) + + mock_db.dcc.docs = [ + { + "dcc_abbreviation": "4DN_DCIC", + "project_id_namespace": "tag:4dn.org,2015:", + } + ] + # The minted term is injected rather than produced: get_file_format + # has one consumer, the ENCODE transform, so the 4DN ingest cannot + # mint it and a real 4DN .bedpe still arrives declaring BED. This + # pins registry routing, which keys on the format name irrespective + # of DCC -- not that 4DN paired-interval files are safe today. See + # the paired-interval section of ENCODE-SUPPLEMENT.md. + mock_db.file.docs = [ + { + "submission": "4dn", + "id_namespace": "tag:4dn.org,2015:", + "local_id": "4DNFIPAIR01", + "filename": "x.bedpe.gz", + "md5": FIXTURE_MD5, + "access_url": "drs://4dn/abc", + "file_format": get_file_format(format_name), + } + ] + + drs_calls: list = [] + + async def fake_drs(*args, **kwargs): + drs_calls.append((args, kwargs)) + raise Exception("DRS unavailable") + + mocker.patch.object(drs, "fetch_drs_object", side_effect=fake_drs) + + # Act & assert + with pytest.raises(HTTPException) as exc_info: + await stream_file("4dn", "4DNFIPAIR01", _make_request(), range=None) + + assert exc_info.value.status_code != 202 + assert len(drs_calls) == 1 + + class TestStreamFileStatus: @pytest.mark.asyncio async def test_stream_file_status_should_raise_400_when_dcc_invalid( diff --git a/tests/test_index.py b/tests/test_index.py index 63adf37..12a10c5 100644 --- a/tests/test_index.py +++ b/tests/test_index.py @@ -10,10 +10,12 @@ from cfdb import api from cfdb.api.routers.index import stream_index_file, stream_index_file_status from cfdb.services import drs, locks +from cfdb.services.ontology_mappings import get_file_format from cfdb.workflows.executor import WoolExecutor from cfdb.workflows.models import ArtifactKind from cfdb.workflows.processors.bam import BamIndexProcessor from cfdb.workflows.processors.registry import ProcessorRegistry, default_registry +from cfdb.workflows.processors.tabix import TabixIntervalProcessor from tests.test_workflows import FIXTURE_MD5 @@ -841,6 +843,65 @@ async def test_stream_index_file_should_return_404_when_no_sidecar_and_no_proces await stream_index_file("4dn", "4DNFIBED01", _make_request(), range=None) assert exc_info.value.status_code == 404 + @pytest.mark.asyncio + @pytest.mark.parametrize("format_name", ["bedpe", "bigInteract"]) + async def test_stream_index_file_should_not_index_a_paired_interval_format( + self, mock_db, mocker, tmp_path, format_name + ): + """Test a paired-interval file is refused rather than mis-indexed. + + Both formats pair two loci per record. While they were aliased to + BED and bigBed the tabix pipeline claimed them and indexed the + first locus alone, committing an artifact that looked successful + and was wrong. Now that each carries its own format name, the + wired registry should claim neither -- so the request fails + cleanly instead. + + Given: + A file in a paired-interval format, no sidecar, and the + workflow subsystem wired with the processors the API + registers. + When: + stream_index_file is called. + Then: + It should raise HTTPException(404) rather than dispatching a + workflow. + """ + # Arrange + from cfdb.workflows.cache import LocalFsCache + + mocker.patch.object(locks, "wait_for_cutover", return_value=None) + # The minted term is injected rather than produced: get_file_format + # has one consumer, the ENCODE transform, so the 4DN ingest cannot + # mint it and a real 4DN .bedpe still arrives declaring BED. This + # pins registry routing, which keys on the format name irrespective + # of DCC -- not that 4DN paired-interval files are safe today. See + # the paired-interval section of ENCODE-SUPPLEMENT.md. + doc = _make_file_doc(file_format=get_file_format(format_name)) + doc.pop("extra", None) + mock_db.files.docs = [] + mock_db.file.docs = [doc] + registry = default_registry() + registry.register(BamIndexProcessor()) + registry.register(TabixIntervalProcessor()) + mocker.patch.object(api, "cache", LocalFsCache(tmp_path / "cache")) + mocker.patch.object(api, "processor_registry", registry) + executor = WoolExecutor( + mock_db, + api.cache, + api.processor_registry, + workdir_root=tmp_path / "jobs", + ) + mocker.patch.object(api, "executor", executor) + ensure = mocker.spy(executor, "ensure_workflow") + + # Act & assert + with pytest.raises(HTTPException) as exc_info: + await stream_index_file("4dn", "4DNFIBED01", _make_request(), range=None) + + assert exc_info.value.status_code == 404 + assert ensure.call_count == 0 + @pytest.mark.asyncio async def test_stream_index_file_should_return_503_when_subsystem_disabled( self, mock_db, mocker From 63d9a8c3c23a4f21d6ae79664fc92616181f85de Mon Sep 17 00:00:00 2001 From: Conrad Date: Sun, 16 Aug 2026 14:25:22 -0400 Subject: [PATCH 13/15] feat: Isolate per-DCC failures in the sync run A DCC that raised aborted every DCC after it. get_all_dcc_names sorts, so one failed ENCODE phase cost the whole HuBMAP sync, and the data-collection indexes built after the loop were skipped along with it. The annotation fan-out did not introduce the cascade but made it likelier: one network stream became three by default, against a portal that returns 504 on requests it cannot assemble in time. Each DCC is now isolated the way _sync_encode already isolates its phases. A failure is logged and recorded, the remaining DCCs still run, the indexes are ensured regardless, and the run fails once at the end naming every DCC that broke, with the first failure chained as the cause. The per-DCC success log and the accession coverage report move inside the success path, so a DCC that failed no longer reports itself as synced. The except stays narrower than BaseException, so a CancelledError cancels the run rather than being recorded as one DCC's failure and followed by every remaining DCC. --- src/cfdb/services/sync.py | 48 ++++++++-- tests/test_sync.py | 193 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 235 insertions(+), 6 deletions(-) diff --git a/src/cfdb/services/sync.py b/src/cfdb/services/sync.py index 732688a..f9a76b8 100644 --- a/src/cfdb/services/sync.py +++ b/src/cfdb/services/sync.py @@ -125,7 +125,16 @@ async def _run_sync(task: SyncTask) -> None: async def _sync_dccs(task: SyncTask) -> None: - """Core sync implementation for API.""" + """Core sync implementation for API. + + Each DCC is isolated the same way ``_sync_encode`` isolates its phases: + one that raises is logged and recorded, and the remaining DCCs still + run. Letting the first failure escape meant every DCC ordered after it + was skipped -- ``get_all_dcc_names`` sorts, so one failed ENCODE phase + cost the whole HuBMAP sync -- along with the data-collection indexes + built at the end. The run still fails once every DCC has been attempted, + naming the ones that broke. + """ if api.db is None: raise RuntimeError("Database not initialized") @@ -134,15 +143,28 @@ async def _sync_dccs(task: SyncTask) -> None: downloads_path = data_path / "downloads" downloads_path.mkdir(exist_ok=True) + failures: list[tuple[str, Exception]] = [] + for dcc in task.dcc_names: task.current_dcc = dcc dcc_type = get_dcc_type(dcc) - # Branch on DCC type - if dcc_type == "rest_api" and dcc == "encode": - await _sync_encode(task) - else: - await _sync_c2m2_zip(task, data_path, downloads_path) + try: + # Branch on DCC type + if dcc_type == "rest_api" and dcc == "encode": + await _sync_encode(task) + else: + await _sync_c2m2_zip(task, data_path, downloads_path) + except Exception as exc: + # Narrower than BaseException on purpose, so a CancelledError + # still cancels the run rather than being recorded as one DCC's + # failure and followed by every remaining DCC. + logger.exception( + "%s sync failed; continuing with the remaining DCCs", + dcc.upper(), + ) + failures.append((dcc, exc)) + continue logger.info(f"{dcc.upper()} synced successfully") await _log_accession_coverage(dcc) @@ -152,12 +174,26 @@ async def _sync_dccs(task: SyncTask) -> None: # (operational indexes only) so we never build these against an # empty database on a cold start; likewise skip when no DCC was # actually synced so an empty request doesn't build them either. + # Runs even when a DCC failed: whatever did load still has to be + # queryable without a full collection scan. if task.dcc_names: task.current_step = "indexing" task.progress = "Ensuring data indexes..." logger.info(task.progress) await ensure_indexes(api.db, data_index_specs()) + if failures: + failed_names = ", ".join(dcc for dcc, _ in failures) + task.progress = ( + f"Sync incomplete: {len(task.dcc_names) - len(failures)} of " + f"{len(task.dcc_names)} DCCs synced" + ) + logger.info(task.progress) + raise RuntimeError( + f"Sync completed with {len(failures)} failed DCC(s) " + f"({failed_names})" + ) from failures[0][1] + task.progress = "All DCCs synced successfully" logger.info(task.progress) diff --git a/tests/test_sync.py b/tests/test_sync.py index 6311488..e712609 100644 --- a/tests/test_sync.py +++ b/tests/test_sync.py @@ -185,6 +185,199 @@ async def test__sync_dccs_should_skip_indexing_when_no_dccs( ensure_mock.assert_not_awaited() +class TestSyncDccsFailureIsolation: + @pytest.mark.asyncio + async def test__sync_dccs_should_sync_the_remaining_dccs_when_one_fails( + self, mocker, mock_db, tmp_path + ): + """Test one DCC's failure does not cost the others their sync. + + get_all_dcc_names sorts, so ENCODE is attempted before HuBMAP on a + whole-corpus sync. Letting the failure escape the loop meant one + failed ENCODE phase -- three network streams by default, against a + portal that 504s on slow requests -- skipped HuBMAP entirely. + + Given: + A three-DCC sync whose first DCC raises. + When: + _sync_dccs runs. + Then: + It should still attempt both remaining DCCs before failing. + """ + # Arrange + mocker.patch.object(sync_module, "DATA_DIR", str(tmp_path)) + mocker.patch.object(sync_module, "get_dcc_type", return_value="rest_api") + mocker.patch.object( + sync_module, + "_sync_encode", + mocker.AsyncMock(side_effect=RuntimeError("encode died")), + ) + zip_sync = mocker.patch.object( + sync_module, "_sync_c2m2_zip", mocker.AsyncMock() + ) + mocker.patch.object(sync_module, "ensure_indexes", mocker.AsyncMock()) + mocker.patch.object( + sync_module, "_log_accession_coverage", mocker.AsyncMock() + ) + task = SyncTask(id="t1", dcc_names=["encode", "4dn", "hubmap"]) + + # Act & assert + with pytest.raises(RuntimeError, match="encode"): + await _sync_dccs(task) + + assert zip_sync.await_count == 2 + + @pytest.mark.asyncio + async def test__sync_dccs_should_ensure_data_indexes_when_a_dcc_failed( + self, mocker, mock_db, tmp_path + ): + """Test a partial run still leaves what loaded queryable. + + The index build sat after the loop, so the first failure skipped it + and left the DCCs that did load answering every query by collection + scan. + + Given: + A two-DCC sync whose first DCC raises. + When: + _sync_dccs runs. + Then: + It should still ensure the data indexes once. + """ + # Arrange + mocker.patch.object(sync_module, "DATA_DIR", str(tmp_path)) + mocker.patch.object(sync_module, "get_dcc_type", return_value="rest_api") + mocker.patch.object( + sync_module, + "_sync_encode", + mocker.AsyncMock(side_effect=RuntimeError("encode died")), + ) + mocker.patch.object(sync_module, "_sync_c2m2_zip", mocker.AsyncMock()) + ensure_mock = mocker.patch.object( + sync_module, "ensure_indexes", mocker.AsyncMock() + ) + mocker.patch.object( + sync_module, "_log_accession_coverage", mocker.AsyncMock() + ) + task = SyncTask(id="t1", dcc_names=["encode", "hubmap"]) + + # Act & assert + with pytest.raises(RuntimeError): + await _sync_dccs(task) + + ensure_mock.assert_awaited_once() + + @pytest.mark.asyncio + async def test__sync_dccs_should_not_report_a_failed_dcc_as_synced( + self, mocker, mock_db, tmp_path + ): + """Test the per-DCC success reporting skips the DCC that failed. + + Given: + A two-DCC sync whose first DCC raises. + When: + _sync_dccs runs. + Then: + Only the DCC that succeeded should be logged as synced and have + its accession coverage reported. + """ + # Arrange + mocker.patch.object(sync_module, "DATA_DIR", str(tmp_path)) + mocker.patch.object(sync_module, "get_dcc_type", return_value="rest_api") + mocker.patch.object( + sync_module, + "_sync_encode", + mocker.AsyncMock(side_effect=RuntimeError("encode died")), + ) + mocker.patch.object(sync_module, "_sync_c2m2_zip", mocker.AsyncMock()) + mocker.patch.object(sync_module, "ensure_indexes", mocker.AsyncMock()) + coverage = mocker.patch.object( + sync_module, "_log_accession_coverage", mocker.AsyncMock() + ) + task = SyncTask(id="t1", dcc_names=["encode", "hubmap"]) + + # Act & assert + with pytest.raises(RuntimeError): + await _sync_dccs(task) + + assert [call.args[0] for call in coverage.await_args_list] == ["hubmap"] + assert task.progress == "Sync incomplete: 1 of 2 DCCs synced" + + @pytest.mark.asyncio + async def test__sync_dccs_should_chain_the_first_dcc_failure_as_the_cause( + self, mocker, mock_db, tmp_path + ): + """Test the raised error keeps the first failure's traceback. + + Given: + A two-DCC sync where both DCCs raise distinguishable errors. + When: + _sync_dccs runs. + Then: + The error's cause should be the first failure's exception. + """ + # Arrange + first = RuntimeError("encode died") + second = RuntimeError("hubmap died") + mocker.patch.object(sync_module, "DATA_DIR", str(tmp_path)) + mocker.patch.object(sync_module, "get_dcc_type", return_value="rest_api") + mocker.patch.object( + sync_module, "_sync_encode", mocker.AsyncMock(side_effect=first) + ) + mocker.patch.object( + sync_module, "_sync_c2m2_zip", mocker.AsyncMock(side_effect=second) + ) + mocker.patch.object(sync_module, "ensure_indexes", mocker.AsyncMock()) + mocker.patch.object( + sync_module, "_log_accession_coverage", mocker.AsyncMock() + ) + task = SyncTask(id="t1", dcc_names=["encode", "hubmap"]) + + # Act & assert + with pytest.raises(RuntimeError) as excinfo: + await _sync_dccs(task) + + assert excinfo.value.__cause__ is first + + @pytest.mark.asyncio + async def test__sync_dccs_should_propagate_a_cancellation( + self, mocker, mock_db, tmp_path + ): + """Test cancellation cancels the run rather than failing one DCC. + + The per-DCC handler is deliberately broad, and a CancelledError + caught by it would be recorded as a DCC failure and followed by + every remaining DCC -- the opposite of cancelling. + + Given: + A two-DCC sync whose first DCC raises CancelledError. + When: + _sync_dccs runs. + Then: + The CancelledError should propagate and the second DCC should + never run. + """ + # Arrange + mocker.patch.object(sync_module, "DATA_DIR", str(tmp_path)) + mocker.patch.object(sync_module, "get_dcc_type", return_value="rest_api") + mocker.patch.object( + sync_module, + "_sync_encode", + mocker.AsyncMock(side_effect=asyncio.CancelledError()), + ) + zip_sync = mocker.patch.object( + sync_module, "_sync_c2m2_zip", mocker.AsyncMock() + ) + mocker.patch.object(sync_module, "ensure_indexes", mocker.AsyncMock()) + task = SyncTask(id="t1", dcc_names=["encode", "hubmap"]) + + # Act & assert + with pytest.raises(asyncio.CancelledError): + await _sync_dccs(task) + + zip_sync.assert_not_awaited() + + # --------------------------------------------------------------------------- # _prune_non_public_hubmap_raw_records # --------------------------------------------------------------------------- From 45972760b8e6f3bec3c871a022a02415d030e4e7 Mon Sep 17 00:00:00 2001 From: Conrad Date: Sun, 16 Aug 2026 14:25:32 -0400 Subject: [PATCH 14/15] docs: Warn that the cache key does not identify the processor cache_key derives dcc/local_id/artifact_kind/md5-vN. The processor's identity is absent -- only its version number is present -- so two processors claiming the same file and artifact kind at equal processor_version derive the same key and read back each other's artifacts as cache hits. That is a wrong answer rather than a miss. Nothing collides today because each pair is claimed by at most one processor, which is a property of the current registry rather than of this function. Re-typing bedpe and bigInteract makes the case concrete: those files still carry index artifacts built by TabixIntervalProcessor before the re-typing, so a paired-interval processor is precisely what would collide with them. A processor identity has to be folded into the key before that processor lands. --- src/cfdb/workflows/keys.py | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/src/cfdb/workflows/keys.py b/src/cfdb/workflows/keys.py index eb8f407..455d7aa 100644 --- a/src/cfdb/workflows/keys.py +++ b/src/cfdb/workflows/keys.py @@ -185,6 +185,21 @@ def cache_key( row for the new key) but does NOT invalidate cached artifacts. To force fresh cache entries for a single processor's outputs, bump that processor's ``processor_version`` instead. + + Warning: + The key identifies the processor only by ``processor_version``, not + by which processor it is. Two processors that claim the same + ``(file, artifact_kind)`` pair at equal ``processor_version`` derive + the *same* key and would read back each other's artifacts as cache + hits -- a wrong answer rather than a miss. This holds today only + because each pair is claimed by at most one processor, which is a + property of the current registry and not of this function. Fold a + processor identity (class name, or a registry-assigned id) into the + key before landing a second processor for any pair. The paired + interval formats make this concrete: ``.bedpe`` and ``bigInteract`` + files carry ``index`` artifacts built by ``TabixIntervalProcessor`` + before they were re-typed, so a future paired-interval processor is + exactly the case that would collide. """ if processor_version < 0: raise ValueError("processor_version must be non-negative") From 28349e6e7089ac5281f95d76a9ffc8cba2dbbc82 Mon Sep 17 00:00:00 2001 From: Conrad Date: Sun, 16 Aug 2026 15:09:07 -0400 Subject: [PATCH 15/15] test: Pin dotted-path existence in the fake collection matcher The contract suite already claimed to cover this. Its name said ``$exists`` resolves dotted paths and its docstring said the doc should be returned, but the body only checked that the call did not raise, with a comment conceding the matcher "may or may not return the doc" -- so it passed against a matcher that resolved nothing. Both assertions now hold the implementation to what the name claims, and both fail against the previous one. The negated case is the one with consequences. Matching by top-level key reports every dotted path as absent, so a filter on ``extra.encode.annotation_type`` not existing matched the whole collection -- and that predicate is how the ENCODE ingest selects the experiment slice it clears before reloading. A delete built on it would have taken the corpus rather than the slice, with the per-phase tests passing throughout. --- tests/test_fake_collection.py | 44 +++++++++++++++++++++++++++-------- 1 file changed, 34 insertions(+), 10 deletions(-) diff --git a/tests/test_fake_collection.py b/tests/test_fake_collection.py index ba82eae..db8f2b9 100644 --- a/tests/test_fake_collection.py +++ b/tests/test_fake_collection.py @@ -194,16 +194,40 @@ async def test_find_one_should_resolve_dotted_existence_path(self): ) # Assert - # ``$exists`` in the FakeCollection matcher checks ``key in doc``; - # the dotted path ``extra.fourdn.extra_files`` is not a top-level - # key, so the matcher's strict behavior may or may not return the - # doc. Just assert that the call does not raise — the test pins - # the present contract. - # If the matcher does return the doc, confirm the dotted resolver - # delivered the right one. If not, the matcher considers the - # dotted key absent — also valid for the current implementation. - if found is not None: - assert found["local_id"] == "x" + assert found is not None + assert found["local_id"] == "x" + + @pytest.mark.asyncio + async def test_find_one_should_not_match_an_absent_dotted_path(self): + """Test that a negated ``$exists`` excludes docs that have the path. + + Matching by top-level key reports every dotted path as absent, so + this predicate matched the whole collection -- which is how the + ENCODE per-slice clearing selects the documents carrying no + annotation type. A delete built on it would have taken the corpus. + + Given: + One doc carrying a nested path and one without it. + When: + ``find_one`` negates ``$exists`` on that dotted path. + Then: + It should return only the doc that lacks the path. + """ + # Arrange + coll = FakeCollection() + coll.docs.append( + {"local_id": "has", "extra": {"encode": {"annotation_type": "cCRE"}}} + ) + coll.docs.append({"local_id": "lacks", "extra": {"encode": {}}}) + + # Act + found = await coll.find_one( + {"extra.encode.annotation_type": {"$exists": False}} + ) + + # Assert + assert found is not None + assert found["local_id"] == "lacks" @pytest.mark.asyncio async def test_find_one_and_update_should_match_lt_operator_on_datetime(self):