ENA validation: platform-scoped re-run on stored output, dormant assay check (#330)

[NoopDog]

Closes #330. Related: #329 (NA-scoring policy across validators), #331.

What changed

• validate_ena_accessions.py validates the stored classification output instead of live-reclassifying: accessions come from the stored archive_accession field or are parsed from the file name ([ESD]RR + digits, boundary-safe for ERR3988887_1.fastq.gz and SAMPLE_ERR123456 naming alike); per-dimension values are read via the canonical models.field_value/field_status readers.
• Per-dimension verdicts are match / mismatch / unknown: a sentinel on our side (not_classified / not_applicable / conflict / absent) scores unknown — excluded from both match and mismatch — instead of the old behavior that counted an empty modality as a mismatch. Modality is also only compared when ENA actually declares evidence (library_source/library_strategy); no more forced "genomic" default.
• A dormant library_strategy → assay_type comparison is wired via the new ENA_LIBRARY_STRATEGY_MAP (validation_maps, beside its HPRC sibling; values verified against assay_type_enum spellings). It scores all-unknown today and activates the moment import work fills assay values. The circularity policy (ENA must not validate values imported from ENA) is documented as policy, explicitly not yet enforced in code.
• Operational hardening: pooled requests.Session sized to --workers (~7K TLS handshakes eliminated); default input auto-detects the latest run with a clean error on fresh clones; results metadata records the input path and the stored-field vs name-parsed accession split, noting the population definition changed vs the 2026-01 artifact; summary math guarded and corrected.

Why

The stored ENA validation artifact was dated 2026-01 and its headline modality figure validated a since-removed guessing classifier — misleading against the current corpus, where FASTQ modality is honestly not_classified (the content ceiling). Re-run scoped to what is meaningful today: platform, where our byte-derived value and ENA's submitter declaration are genuinely independent routes to the same fact.

Re-run results (2026-08-18, run 20260802_170826)

• 6,962 accession-bearing FASTQs; 6,830 validated (132 ENA lookup errors)
• platform: 6,830/6,830 agree — 100.00%, 0 unknown
• modality / assay: nothing scored — all unknown-by-design (our side uncommitted)

Assumptions I made

• Validating the stored output (not re-running current rules live) is the intended semantics — the old sample_reads live-reclassify path is dead for current inputs anyway, since the slimmed output format no longer carries sample_reads. (An earlier commit message misattributed this to a header-classifier rework; the correction is recorded in the follow-up commit.)
• The name-parsed accession fallback widens the validated population vs January's stored-field-only requirement; the results metadata records the split so runs are comparable.
• Cross-validator policy consolidation (shared verdict helper + sentinel set — the ENA and HPRC copies have already drifted) is deliberately deferred to HPRC validation: our not_applicable vs a declared catalog value should score as mismatch, not unknown #329, which owns that scope.

How to verify

• uv run python scripts/validate_ena_accessions.py --limit 25 -o /tmp/ena_smoke.json — expect: ~25 files found, platform ~100% agree, MODALITY: nothing scored — N unknown, ASSAY: nothing scored — N unknown.
• Inspect /tmp/ena_smoke.json: metadata.input names the run file; metadata.accession_source splits stored-field vs name-parsed; results carries *_match/_mismatch/_unknown for all three dimensions.
• Fresh-clone behavior: run with no output/ dir — expect a one-line Error: Output directory not found… instead of a traceback.
• Gate: make lint && make format-check && make test (782 unit tests; test_evals.py network failures are pre-existing/environmental).

🤖 Generated with Claude Code

[Copilot]

Pull request overview

Updates the ENA FASTQ validator to validate stored classification outputs (rather than re-running classifiers), and expands the comparison model to explicitly support match/mismatch/unknown verdicts per dimension, including a newly wired (currently dormant) ENA library_strategy → assay_type check.

Changes:

• Add ENA_LIBRARY_STRATEGY_MAP to translate ENA/SRA library_strategy values into assay_type_enum.
• Refactor validate_ena_accessions.py to (a) extract accessions from stored archive_accession or parse from file_name, (b) read per-dimension values/status via field_value/field_status, and (c) score platform/modality/assay as match/mismatch/unknown.
• Add operational hardening: shared requests.Session with worker-sized connection pool, latest-run autodetection, and richer output metadata.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
src/meta_disco/validation_maps.py	Adds ENA library_strategy → assay_type mapping constant used by the validator.
scripts/validate_ena_accessions.py	Validates stored FASTQ classifications against ENA, adds accession parsing fallback, per-dimension verdict scoring, pooled HTTP session, and improved output metadata.

Suppressed comments (2)

scripts/validate_ena_accessions.py:136

• This comment references issue #329 as supporting the current "unknown" scoring, but #329 specifically argues that not_applicable vs a declared external value should count as a mismatch (not unknown). The comment is misleading as written; either drop the reference or clarify the distinction.

    # Results tracking. "unknown" = our side committed nothing (sentinel or
    # absent status) OR, assay only, ENA's strategy has no mapping into our
    # vocabulary. Unknown is excluded from both match and mismatch (#330;
    # same policy direction as #329).

scripts/validate_ena_accessions.py:295

• The printed summary label "API errors (no data)" is broader than what is actually counted (e.g. it includes reason=no_platform, which is an ENA record/content issue rather than an API failure). Renaming the label would reduce confusion without changing the output JSON keys.

    print(f"API errors (no data):           {results['api_errors']:,}")

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (2)

scripts/validate_ena_accessions.py:77

• our_field() claims schema-invalid drift (non-string value) is treated as “uncommitted” so the verdict becomes unknown, but field_status() can raise ValueError on an incoherent {value,status} pair (e.g. status='classified' with a non-string/None value). That would currently crash the validator instead of producing an unknown verdict. Consider catching ValueError (and treating it as uncommitted) so older/corrupt outputs don’t take down the whole run.

    value = field_value(rec, field)
    status = field_status(rec, field)
    if not isinstance(value, str):
        return "", str(status or "")
    return value, str(status or "")

scripts/validate_ena_accessions.py:304

• The output label API errors (no data) is misleading because api_errors also includes cases where ENA returns a record but it lacks instrument_platform (reason=no_platform). Renaming the label makes the summary consistent with the docstring and the actual error bucket contents.

    print(f"Total files with ENA accession: {len(with_acc):,}")
    print(f"Successfully validated:         {n:,}")
    print(f"API errors (no data):           {results['api_errors']:,}")
    print(f"Time elapsed:                   {elapsed:.1f}s ({rate:.1f} files/sec)")

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (4)

scripts/validate_ena_accessions.py:95

• requests.Session() is shared across all ThreadPoolExecutor workers via the module-level _session, but requests.Session is not guaranteed to be thread-safe. Under concurrent use this can cause intermittent request/connection issues that are hard to reproduce. Prefer a per-thread session (e.g., threading.local()), or avoid sharing Session across threads while still keeping keep-alive benefits per worker.

# One pooled session for the whole run: ~7K calls to the same host would
# otherwise each pay a fresh TLS handshake. Pool size is raised to match
# --workers in validate_against_ena().
_session = requests.Session()


def fetch_ena_metadata(acc: str) -> dict | None:
    """Fetch metadata for a single accession from ENA API."""
    try:
        resp = _session.get(
            ENA_API,

scripts/validate_ena_accessions.py:309

• The printed label API errors (no data) is misleading because api_errors includes non-API conditions too (e.g. reason=no_platform). Consider naming this consistently with the bucket semantics (ENA lookup/record errors, not validated).

    print(f"Total files with ENA accession: {len(with_acc):,}")
    print(f"Successfully validated:         {n:,}")
    print(f"API errors (no data):           {results['api_errors']:,}")
    print(f"Time elapsed:                   {elapsed:.1f}s ({rate:.1f} files/sec)")

scripts/validate_ena_accessions.py:64

• The accession extraction logic is now more permissive (stored archive_accession OR regex parsed from file_name) and the regex boundary behavior is subtle. There are no unit tests covering ACCESSION_RE / extract_accession() for the documented edge cases (e.g. ERR3988887_1.fastq.gz, SAMPLE_ERR123456, embedded XERR123456). Adding focused tests would help prevent future regressions in the join key used by this validator.

# Boundaries chosen for real naming: ERR3988887_1.fastq.gz has a word char
# (underscore) right after the digits, so no trailing \b; HG002_ERR123456
# style prefixes put an underscore *before* the accession, so the leading
# guard rejects only letters/digits (an embedded ...XERR123456 is not an
# accession), not underscores. Greedy \d{6,} consumes the whole digit run.
ACCESSION_RE = re.compile(r"(?<![A-Za-z0-9])([ESD]RR\d{6,})")
# Statuses that mean "our side committed nothing" ("" = status absent).
_SENTINEL_STATUSES = {"not_classified", "not_applicable", "conflict", ""}


def extract_accession(rec: dict) -> str | None:
    """The record's run accession: the stored field, else parsed from file_name."""
    acc = rec.get("archive_accession")
    if acc:
        return str(acc)
    m = ACCESSION_RE.search(str(rec.get("file_name") or ""))
    return m.group(1) if m else None

src/meta_disco/validation_maps.py:40

• ENA_LIBRARY_STRATEGY_MAP is new validator-critical vocabulary mapping, but there’s no test coverage ensuring (a) mapped values are valid assay_type_enum spellings and (b) expected keys remain supported. Consider adding a small unit test (similar to tests/test_hprc_validation.py) to pin this mapping and catch typos/drift early.

# ENA/SRA library_strategy → our assay_type_enum. Only strategies with a
# clean equivalent are mapped; anything absent scores "unknown" in the ENA
# validator rather than being force-fitted (#330).
ENA_LIBRARY_STRATEGY_MAP = {
    "WGS": "WGS",
    "WXS": "WES",
    "WES": "WES",
    "RNA-Seq": "RNA-seq",
    "ATAC-seq": "ATAC-seq",
    "ChIP-Seq": "ChIP-seq",
    "Bisulfite-Seq": "Bisulfite-seq",
}

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (2)

scripts/validate_ena_accessions.py:319

• The "nothing scored" summary message says unknown means "our side uncommitted", but unknown can also happen when ENA provides no comparable evidence (e.g. missing modality source/strategy) or when assay strategy is unmapped. The message should avoid attributing unknowns solely to our side.

            print(f"{label}: nothing scored — {unknown:,} unknown (our side uncommitted; see module docstring)")

scripts/validate_ena_accessions.py:146

• The comment describing what counts as an "unknown" verdict is incomplete: verdicts also become unknown when the ENA side has no comparable value for that dimension (e.g. modality when ENA declares neither library_source nor library_strategy). This comment should reflect all cases so the output/metrics are interpretable.

This issue also appears on line 319 of the same file.

    # Results tracking. "unknown" = our side committed nothing (sentinel or
    # absent status) OR, assay only, ENA's strategy has no mapping into our
    # vocabulary. Unknown is excluded from both match and mismatch (#330).

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (2)

scripts/validate_ena_accessions.py:98

• requests.Session() is shared across all ThreadPoolExecutor workers via the module-level _session. requests.Session is not guaranteed to be thread-safe, so concurrent .get() calls can lead to subtle connection/state races under load. Consider using a per-thread session (e.g., threading.local()), or create a separate Session per worker (still mounting an adapter/pool) to keep connection reuse without cross-thread sharing.

# One pooled session for the whole run: ~7K calls to the same host would
# otherwise each pay a fresh TLS handshake. Pool size is raised to match
# --workers in validate_against_ena().
_session = requests.Session()


def fetch_ena_metadata(acc: str) -> dict | None:
    """Fetch metadata for a single accession from ENA API."""
    try:
        resp = _session.get(
            ENA_API,
            params={"accession": acc, "result": "read_run", "fields": FIELDS},
            timeout=10,
        )

scripts/validate_ena_accessions.py:407

• --workers accepts any int, but ThreadPoolExecutor(max_workers=workers) will raise a ValueError for 0 or negative values. Adding an explicit validation makes the CLI fail fast with a clear error message (and avoids mounting an adapter with an invalid pool size).

        "--workers",
        "-w",
        type=int,
        default=10,
        help="Number of parallel workers (default: 10)",
    )
    args = parser.parse_args()

    if args.input:
        input_path = args.input
    else:
        try:
            input_path = find_latest_run(Path("output/anvil")) / "fastq_classifications.json"
        except FileNotFoundError as exc:
            print(f"Error: {exc}", file=sys.stderr)
            sys.exit(1)

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (4)

scripts/validate_ena_accessions.py:64

• extract_accession() only checks rec["archive_accession"], but pipeline OutputRecord layout stores FASTQ join keys under rec["classifications"]["archive_accession"] (see tests/fixtures/golden/expected_output.json). As written, stored accessions will be ignored and the function will fall back to filename parsing, which can silently shrink/shift the validated population and make accession_source reporting incorrect.

def extract_accession(rec: dict) -> str | None:
    """The record's run accession: the stored field, else parsed from file_name."""
    acc = rec.get("archive_accession")
    if acc:
        return str(acc)

scripts/validate_ena_accessions.py:128

• data = json.load(f) can be a list (some producers/tests may emit a bare list of records), but the code unconditionally does data.get(...), which will raise AttributeError before the new shape guard runs. Consider normalizing data to classifications based on whether it is a dict vs list so unexpected shapes still fail fast with the intended actionable error.

    classifications = data.get("classifications", data)
    if not isinstance(classifications, list):
        # Fail fast: iterating an unexpected dict shape would yield keys and
        # silently report "Found 0 files" instead of an actionable error.
        print(f"Error: {input_path} does not hold a classification list", file=sys.stderr)

scripts/validate_ena_accessions.py:350

• metadata.accession_source counts r.get("archive_accession"), but in the current pipeline output shape the stored accession is under r["classifications"]["archive_accession"]. This will report stored_field=0 / inflate name_parsed even when stored accessions exist.

                    "accession_source": {
                        "stored_field": sum(1 for r in with_acc if r.get("archive_accession")),
                        "name_parsed": sum(1 for r in with_acc if not r.get("archive_accession")),
                    },

tests/test_ena_validation.py:55

• extract_accession() is documented as preferring a stored archive_accession, but the tests only cover the legacy top-level {"archive_accession": ...} shape. Add a test for the pipeline OutputRecord layout where archive_accession lives under rec["classifications"] so regressions are caught.

    def test_stored_field_preferred_over_name(self):
        rec = {"archive_accession": "ERR999999", "file_name": "ERR111111_1.fastq.gz"}
        assert ena.extract_accession(rec) == "ERR999999"

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (2)

scripts/validate_ena_accessions.py:144

• validate_against_ena() calls sys.exit(1) when the input JSON isn't a list. Because this module is imported by tests (and could be imported by other code), exiting from a non-main() function makes it hard to reuse/test and can terminate a larger process unexpectedly. Prefer raising an exception (e.g. ValueError) here and handling it in main() (print the same message + exit code) so CLI behavior stays the same without forcing process exit from a helper.

    if not isinstance(classifications, list):
        # Fail fast: iterating an unexpected dict shape would yield keys and
        # silently report "Found 0 files" instead of an actionable error.
        print(f"Error: {input_path} does not hold a classification list", file=sys.stderr)
        sys.exit(1)

scripts/validate_ena_accessions.py:104

• requests.Session() is shared globally (_session) and then used concurrently from multiple ThreadPoolExecutor worker threads. requests.Session maintains mutable state (connection pools, cookies, headers) and is not guaranteed to be safe for concurrent use, which can lead to intermittent failures under load. Consider using a per-thread session (thread-local) or creating the session inside validate_against_ena() and ensuring requests state isn't shared across threads; you can still keep the performance win by reusing connections per thread.

# One pooled session for the whole run: ~7K calls to the same host would
# otherwise each pay a fresh TLS handshake. Pool size is raised to match
# --workers in validate_against_ena().
_session = requests.Session()

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (2)

scripts/validate_ena_accessions.py:70

• stored_accession() only falls back to rec["classifications"]["archive_accession"] when the top-level field is falsy. If the top-level archive_accession exists but is an unexpected type (e.g. dict/list drift), the nested valid string is ignored and we unnecessarily fall back to filename parsing.

    acc = rec.get("archive_accession")
    if not acc:
        nested = rec.get("classifications")
        if isinstance(nested, dict):
            acc = nested.get("archive_accession")
    return str(acc) if acc and not isinstance(acc, (dict, list)) else None

scripts/validate_ena_accessions.py:158

• HTTPAdapter(pool_connections=workers, ...) misuses pool_connections (it controls the number of host pools, not concurrent connections to a single host). With a single ENA host, this should stay small (e.g. 1) and pool_block=True better matches the stated goal of reusing keep-alive connections rather than creating/discarding extras beyond the pool.

    # Size the shared session's pool to the worker count so threads reuse
    # keep-alive connections instead of serializing on the default pool.
    adapter = HTTPAdapter(pool_connections=workers, pool_maxsize=workers)
    _session.mount("https://", adapter)