diff --git a/ena-submission/src/ena_deposition/check_external_visibility.py b/ena-submission/src/ena_deposition/check_external_visibility.py index f058448fc5..0f2794d6fc 100644 --- a/ena-submission/src/ena_deposition/check_external_visibility.py +++ b/ena-submission/src/ena_deposition/check_external_visibility.py @@ -68,7 +68,13 @@ def check_visibility(self, config: Config, accession: str) -> datetime | None: class ENAVisibilityChecker(VisibilityChecker): - """Checker for ENA visibility""" + """Checker for ENA visibility + + This class is adapted and re-used in + preprocessing/nextclade/src/loculus_preprocessing/external_services.py + If you change anything here, check if the update needs to be applied there + as well. + """ def check_visibility(self, config: Config, accession: str) -> datetime | None: file_type = ( diff --git a/kubernetes/loculus/values.yaml b/kubernetes/loculus/values.yaml index ba2f9148b3..5a306d2e8e 100644 --- a/kubernetes/loculus/values.yaml +++ b/kubernetes/loculus/values.yaml @@ -721,6 +721,12 @@ defaultOrganismConfig: &defaultOrganismConfig url: "https://www.ebi.ac.uk/ena/browser/view/__value__" - name: View in DDBJ url: "https://ddbj.nig.ac.jp/search/entry/bioproject/__value__" + preprocessing: + function: check_ena_accession + inputs: + accession: bioprojectAccession + args: + accession_type: bioproject header: "INSDC" orderOnDetailsPage: 1060 ingest: bioprojects @@ -748,6 +754,12 @@ defaultOrganismConfig: &defaultOrganismConfig url: "https://www.ebi.ac.uk/ena/browser/view/__value__" - name: View in DDBJ url: "https://ddbj.nig.ac.jp/search/entry/biosample/__value__" + preprocessing: + function: check_ena_accession + inputs: + accession: biosampleAccession + args: + accession_type: biosample header: "INSDC" orderOnDetailsPage: 1080 guidance: "Only add this field if you have already submitted the raw reads to the INSDC, read more at https://pathoplexus.org/docs/how-to/upload-sequences. If you add this field we will not create a new biosample for you and assume you have submitted all required metadata in the linked biosample." @@ -1599,6 +1611,12 @@ defaultOrganismConfig: &defaultOrganismConfig customDisplay: type: link url: "https://www.ncbi.nlm.nih.gov/sra/?term=__value__" + preprocessing: + function: check_ena_accession + inputs: + accession: insdcRawReadsAccession + args: + accession_type: raw_reads header: "INSDC" orderOnDetailsPage: 1120 ingest: ncbiSraAccessions diff --git a/preprocessing/nextclade/README.md b/preprocessing/nextclade/README.md index 3e42e18be3..2297c43a1f 100644 --- a/preprocessing/nextclade/README.md +++ b/preprocessing/nextclade/README.md @@ -139,6 +139,7 @@ taxonomyService: 9. `resolve_host_taxon_id`: Validates that a host taxon ID or scientific name exists in the NCBI taxonomy, returning the taxon ID if validation is successful. Requires an input field called `host` (entries ingested from the INSDC do not error). 10. `scientific_name_from_id`: Returns a scientific name for a taxon given a taxon ID `hostTaxonId` as input (entries ingested from the INSDC do not error). 11. `common_name_from_id`: Returns a common name for a taxon given a taxon ID `hostTaxonId` as input (entries ingested from the INSDC do not error). +12. `check_ena_accession`: checks whether a submitter-provided `biosampleAccession`, `bioprojectAccession`, or `insdcRawReadsAccession` is publicly visible on ENA. Using these functions in your `values.yaml` will look like: diff --git a/preprocessing/nextclade/src/loculus_preprocessing/config.py b/preprocessing/nextclade/src/loculus_preprocessing/config.py index b2bf3901f7..9c6a1d43d8 100644 --- a/preprocessing/nextclade/src/loculus_preprocessing/config.py +++ b/preprocessing/nextclade/src/loculus_preprocessing/config.py @@ -20,6 +20,7 @@ Topology, ) from loculus_preprocessing.external_services import ( + ENAVisibilityChecker, ExternalServices, FileProcessingService, TaxonomyService, @@ -143,7 +144,9 @@ class Config(BaseModel): # External services taxonomy_service_url: str | None = None _external_services: ExternalServices = PrivateAttr( - default=ExternalServices(taxonomy_service=TaxonomyService(None)) + default=ExternalServices( + taxonomy_service=TaxonomyService(None), ena_visibility_checker=ENAVisibilityChecker() + ) ) raw_reads_processing_service_url: str | None = None raw_reads_processing_service_timeout_seconds: int = 600 @@ -171,7 +174,8 @@ def finalize(self): validate_required_when(self) self.processing_order = get_processing_order(self) self._external_services = ExternalServices( - taxonomy_service=TaxonomyService(self.taxonomy_service_url) + taxonomy_service=TaxonomyService(self.taxonomy_service_url), + ena_visibility_checker=ENAVisibilityChecker(), ) return self diff --git a/preprocessing/nextclade/src/loculus_preprocessing/external_services.py b/preprocessing/nextclade/src/loculus_preprocessing/external_services.py index b6b63187c2..920ea1f388 100644 --- a/preprocessing/nextclade/src/loculus_preprocessing/external_services.py +++ b/preprocessing/nextclade/src/loculus_preprocessing/external_services.py @@ -1,7 +1,12 @@ +import io import logging +import re import urllib.parse +import xml.etree.ElementTree as ET from collections import OrderedDict from dataclasses import dataclass +from enum import StrEnum +from http import HTTPStatus import requests from pydantic import BaseModel, Field, ValidationError @@ -17,6 +22,7 @@ ProcessingAnnotation, RawProcessingResult, _internal_error_message, + processing_error, raw_internal_error, ) @@ -210,17 +216,6 @@ def get_common_name(self, tax_id: str) -> RawProcessingResult: return RawProcessingResult(datum=common_name) -@dataclass(frozen=True) -class ExternalServices: - """External services available to processing functions. - - Kept separate from `ProcessingContext` since these don't vary per accession, unlike - `ProcessingContext`'s fields. - """ - - taxonomy_service: TaxonomyService - - FileName = str @@ -316,3 +311,111 @@ def _annotation( if not internal_error: return ProcessingAnnotation([source], [source], message) return ProcessingAnnotation([source], [source], _internal_error_message(message)) + + +# Successful ENA responses are small XML documents (a few KB). Cache hits refresh an entry's +# position; a bioproject shared across a batch stays cached while unique biosamples and runs +# cycle through. +ena_cache = RequestCache(max_size=16, retries=1) + + +class EnaAccessionType(StrEnum): + BIOPROJECT = "bioproject" + BIOSAMPLE = "biosample" + RAW_READS = "raw_reads" + + +# Also guards against comma-separated lists, which the ENA browser API +# would otherwise resolve to multiple records. +# (see https://ena-docs.readthedocs.io/en/latest/submit/general-guide/accessions.html) +ACCESSION_PATTERNS: dict[EnaAccessionType, re.Pattern[str]] = { + EnaAccessionType.BIOPROJECT: re.compile(r"PRJ[EDN][A-Z][0-9]+"), + EnaAccessionType.BIOSAMPLE: re.compile(r"SAM[EDN][A-Z]?[0-9]+"), + EnaAccessionType.RAW_READS: re.compile(r"[EDS]RR[0-9]{6,}"), +} + + +class ENAVisibilityChecker: + """Used to check ENA visibility + + Adapted from the ENAVisibilityChecker in + ena-submission/src/ena_deposition/check_external_visibility.py. + If anything changes in that class, check whether the update needs to + be applied here as well. + """ + + def __init__( + self, + timeout_seconds: int = 10, + ): + self.timeout_seconds = timeout_seconds + + def check_visibility(self, accession: str, accession_type_arg: str) -> RawProcessingResult: + try: + accession_type = EnaAccessionType(accession_type_arg) + except ValueError: + return raw_internal_error( + f"Invalid accession_type '{accession_type_arg}', expected one of " + f"{[t.value for t in EnaAccessionType]}." + ) + accession = accession.strip().upper() + pattern = ACCESSION_PATTERNS[accession_type] + if not pattern.fullmatch(accession): + return processing_error( + f"'{accession}' is not a valid {accession_type.replace('_', ' ')} accession, " + f"expected a value matching '{pattern.pattern}'." + ) + try: + response = ena_cache.get_or_fetch( + f"https://www.ebi.ac.uk/ena/browser/api/xml/{accession}", + timeout=self.timeout_seconds, + ) + except requests.RequestException as e: + logger.warning(f"Failed to check ENA visibility of '{accession}': {e}") + response = None + if response is None or response.status_code >= HTTPStatus.INTERNAL_SERVER_ERROR: + return RawProcessingResult( + datum=accession, + warnings=[ + f"Unable to check visibility of '{accession}': could not reach ENA. Please " + f"verify yourself that '{accession}' is public before proceeding or we will " + "not be able to propagate your submission to the INSDC." + ], + ) + if response.status_code != HTTPStatus.OK: + return RawProcessingResult( + datum=accession, + warnings=[ + f"Accession '{accession}' is not visible on ENA. Please make '{accession}' " + "public before proceeding or we will not be able to propagate your " + "submission to the INSDC." + ], + ) + if accession_type == EnaAccessionType.BIOPROJECT and self._is_umbrella_project( + response.content + ): + return processing_error( + f"bioproject '{accession}' is an umbrella project. " + "Please provide the accession of a submission project instead." + ) + return RawProcessingResult(datum=accession) + + def _is_umbrella_project(self, xml_bytes) -> bool: + for _, elem in ET.iterparse(io.BytesIO(xml_bytes), events=("start",)): # noqa: S314 + if elem.tag == "UMBRELLA_PROJECT": + return True + if elem.tag == "SUBMISSION_PROJECT": + return False + return False + + +@dataclass(frozen=True) +class ExternalServices: + """External services available to processing functions. + + Kept separate from `ProcessingContext` since these don't vary per accession, unlike + `ProcessingContext`'s fields. + """ + + taxonomy_service: TaxonomyService + ena_visibility_checker: ENAVisibilityChecker diff --git a/preprocessing/nextclade/src/loculus_preprocessing/processing_functions.py b/preprocessing/nextclade/src/loculus_preprocessing/processing_functions.py index 79f83c5914..8a414d9d69 100644 --- a/preprocessing/nextclade/src/loculus_preprocessing/processing_functions.py +++ b/preprocessing/nextclade/src/loculus_preprocessing/processing_functions.py @@ -1297,6 +1297,27 @@ def common_name_from_id( # noqa: PLR0913, PLR0917 return external_services.taxonomy_service.get_common_name(tax_id) + @staticmethod + def check_ena_accession( # noqa: PLR0913, PLR0917 + input_data: InputMetadata, + output_field: str, + input_fields: list[str], + args: FunctionArgs, + context: ProcessingContext, + external_services: ExternalServices, + ) -> RawProcessingResult: + accession_type = args.get("accession_type") + if not isinstance(accession_type, str): + return raw_internal_error( + f"check_ena_accession requires an accession_type arg, got args {args}." + ) + accession: str | None = input_data.get("accession") + if not accession: + return RawProcessingResult() + if context.is_insdc_ingest_group: + return RawProcessingResult(datum=accession) + return external_services.ena_visibility_checker.check_visibility(accession, accession_type) + def single_metadata_annotation( source_name: str, diff --git a/preprocessing/nextclade/tests/factory_methods.py b/preprocessing/nextclade/tests/factory_methods.py index e05b9b5acf..af9845320a 100644 --- a/preprocessing/nextclade/tests/factory_methods.py +++ b/preprocessing/nextclade/tests/factory_methods.py @@ -22,7 +22,11 @@ SegmentName, UnprocessedEntry, ) -from loculus_preprocessing.external_services import ExternalServices, TaxonomyService +from loculus_preprocessing.external_services import ( + ENAVisibilityChecker, + ExternalServices, + TaxonomyService, +) from loculus_preprocessing.nextclade_annotation import GffAttributes, NextcladeAnnotation # Default ProcessingContext for tests that don't care about its contents (no INSDC @@ -38,7 +42,9 @@ # Default ExternalServices for tests that don't care about its contents (no taxonomy # service URL configured). -DEFAULT_EXTERNAL_SERVICES = ExternalServices(taxonomy_service=TaxonomyService(None)) +DEFAULT_EXTERNAL_SERVICES = ExternalServices( + taxonomy_service=TaxonomyService(None), ena_visibility_checker=ENAVisibilityChecker() +) def ts_from_ymd(year: int, month: int, day: int) -> str: diff --git a/preprocessing/nextclade/tests/test_metadata_processing_functions.py b/preprocessing/nextclade/tests/test_metadata_processing_functions.py index 0732959cb4..157f53c291 100644 --- a/preprocessing/nextclade/tests/test_metadata_processing_functions.py +++ b/preprocessing/nextclade/tests/test_metadata_processing_functions.py @@ -3,6 +3,7 @@ from unittest import mock import pytest +import requests from factory_methods import ( DEFAULT_EXTERNAL_SERVICES, DEFAULT_TEST_CONTEXT, @@ -16,6 +17,7 @@ verify_processed_entry, ) +from loculus_preprocessing import external_services from loculus_preprocessing.config import Config, ProcessingSpec, get_config, get_processing_order from loculus_preprocessing.datatypes import ( AnnotationSource, @@ -1805,5 +1807,205 @@ def test_call_function_converts_raw_errors_to_annotations() -> None: ) +SUBMISSION_PROJECT_XML = b""" + + A submission project + + +""" + +UMBRELLA_PROJECT_XML = b""" + + An umbrella project + + + + + +""" + +BIOSAMPLE_XML = b"""""" + + +@dataclass +class EnaAccessionCase: + name: str + accession: str + accession_type: str | None = "bioproject" + # How ENA responds: an HTTP status code, or an exception raised by the session. + ena_response: int | Exception | None = None + ena_xml: bytes = b"" + is_insdc_ingest_group: bool = False + expected_datum: str | None = None + expected_error: str | None = None + expected_warning: str | None = None + # None means ENA must not be contacted at all. + expected_url: str | None = None + + +ena_accession_cases = [ + EnaAccessionCase( + name="public_bioproject_is_accepted", + accession="PRJEB12345", + ena_response=200, + ena_xml=SUBMISSION_PROJECT_XML, + expected_datum="PRJEB12345", + expected_url="https://www.ebi.ac.uk/ena/browser/api/xml/PRJEB12345", + ), + EnaAccessionCase( + name="umbrella_bioproject_is_rejected", + accession="PRJEB106529", + ena_response=200, + ena_xml=UMBRELLA_PROJECT_XML, + expected_error="is an umbrella project", + expected_url="https://www.ebi.ac.uk/ena/browser/api/xml/PRJEB106529", + ), + EnaAccessionCase( + name="public_biosample_is_accepted", + accession="SAMEA123456", + accession_type="biosample", + ena_response=200, + ena_xml=BIOSAMPLE_XML, + expected_datum="SAMEA123456", + expected_url="https://www.ebi.ac.uk/ena/browser/api/xml/SAMEA123456", + ), + EnaAccessionCase( + name="public_sra_run_is_accepted", + accession="SRR27477368", + accession_type="raw_reads", + ena_response=200, + expected_datum="SRR27477368", + expected_url="https://www.ebi.ac.uk/ena/browser/api/xml/SRR27477368", + ), + EnaAccessionCase( + name="sra_experiment_in_raw_reads_field_is_rejected", + accession="SRX23184375", + accession_type="raw_reads", + expected_error="not a valid raw reads accession", + ), + EnaAccessionCase( + name="biosample_in_bioproject_field_is_rejected", + accession="SAMN12345678", + expected_error="not a valid bioproject accession", + ), + EnaAccessionCase( + name="bioproject_in_biosample_field_is_rejected", + accession="PRJEB12345", + accession_type="biosample", + expected_error="not a valid biosample accession", + ), + EnaAccessionCase( + name="comma_separated_bioprojects_are_rejected", + accession="PRJNA591860,PRJEB106529", + expected_error="not a valid bioproject accession", + ), + EnaAccessionCase( + name="lowercase_accession_is_uppercased", + accession="prjeb12345", + ena_response=200, + ena_xml=SUBMISSION_PROJECT_XML, + expected_datum="PRJEB12345", + expected_url="https://www.ebi.ac.uk/ena/browser/api/xml/PRJEB12345", + ), + EnaAccessionCase( + name="surrounding_whitespace_is_stripped", + accession=" PRJEB12345 ", + ena_response=200, + ena_xml=SUBMISSION_PROJECT_XML, + expected_datum="PRJEB12345", + expected_url="https://www.ebi.ac.uk/ena/browser/api/xml/PRJEB12345", + ), + EnaAccessionCase( + name="invalid_accession_type_is_internal_error", + accession="PRJEB12345", + accession_type="nucleotide", + expected_error="Invalid accession_type 'nucleotide'", + ), + EnaAccessionCase( + name="missing_accession_type_is_internal_error", + accession="PRJEB12345", + accession_type=None, + expected_error="requires an accession_type arg", + ), + EnaAccessionCase( + name="not_visible_accession_is_warned_about", + accession="PRJEB99999", + ena_response=404, + expected_datum="PRJEB99999", + expected_warning="is not visible on ENA", + expected_url="https://www.ebi.ac.uk/ena/browser/api/xml/PRJEB99999", + ), + EnaAccessionCase( + name="server_error_is_warned_about", + accession="PRJEB12345", + ena_response=501, + expected_datum="PRJEB12345", + expected_warning="could not reach ENA", + expected_url="https://www.ebi.ac.uk/ena/browser/api/xml/PRJEB12345", + ), + EnaAccessionCase( + name="exhausted_retries_are_warned_about", + accession="PRJEB12345", + ena_response=requests.exceptions.RetryError(), + expected_datum="PRJEB12345", + expected_warning="could not reach ENA", + expected_url="https://www.ebi.ac.uk/ena/browser/api/xml/PRJEB12345", + ), + EnaAccessionCase( + name="missing_accession_is_not_checked", + accession="", + ), + EnaAccessionCase( + name="insdc_ingested_accession_is_not_checked", + accession="PRJNA123,PRJNA456", + is_insdc_ingest_group=True, + expected_datum="PRJNA123,PRJNA456", + ), +] + + +@pytest.mark.parametrize("case", ena_accession_cases, ids=lambda c: c.name) +def test_check_ena_accession(case: EnaAccessionCase) -> None: + external_services.ena_cache.clear() + with mock.patch.object(external_services.ena_cache, "session") as mock_session: + if isinstance(case.ena_response, Exception): + mock_session.get.side_effect = case.ena_response + else: + mock_session.get.return_value = mock.MagicMock( + status_code=case.ena_response, content=case.ena_xml + ) + + ingest_group_id = DEFAULT_TEST_CONTEXT.insdc_ingest_group_id + result = ProcessingFunctions.check_ena_accession( + input_data={"accession": case.accession}, + output_field="externalAccession", + input_fields=["externalAccession"], + args={"accession_type": case.accession_type}, + context=replace( + DEFAULT_TEST_CONTEXT, + group_id=ingest_group_id if case.is_insdc_ingest_group else ingest_group_id + 1, + ), + external_services=DEFAULT_EXTERNAL_SERVICES, + ) + + assert result.datum == case.expected_datum + if case.expected_error is None: + assert result.errors == [] + else: + assert len(result.errors) == 1 + assert case.expected_error in result.errors[0] + if case.expected_warning is None: + assert result.warnings == [] + else: + assert len(result.warnings) == 1 + assert case.expected_warning in result.warnings[0] + + if case.expected_url is None: + assert mock_session.get.call_count == 0 + else: + mock_session.get.assert_called_once() + assert mock_session.get.call_args.args[0] == case.expected_url + + if __name__ == "__main__": pytest.main()