-
Notifications
You must be signed in to change notification settings - Fork 5
Simplify machine type loading with TSV fetch and fallback #21
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,17 +1,45 @@ | ||||||||||||||||||||||||||||||||||||||||||||
| import csv | ||||||||||||||||||||||||||||||||||||||||||||
| import re | ||||||||||||||||||||||||||||||||||||||||||||
| import warnings | ||||||||||||||||||||||||||||||||||||||||||||
| from io import TextIOWrapper | ||||||||||||||||||||||||||||||||||||||||||||
| from pathlib import Path | ||||||||||||||||||||||||||||||||||||||||||||
| from urllib.error import URLError | ||||||||||||||||||||||||||||||||||||||||||||
| from urllib.request import urlopen | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| MACHINE_TYPES = { | ||||||||||||||||||||||||||||||||||||||||||||
| MACHINE_TYPES_FALLBACK = { | ||||||||||||||||||||||||||||||||||||||||||||
| "VH": "Illumina-NextSeq", | ||||||||||||||||||||||||||||||||||||||||||||
| "D": "Illumina-HiSeq", | ||||||||||||||||||||||||||||||||||||||||||||
| "M": "Illumina-MiSeq", | ||||||||||||||||||||||||||||||||||||||||||||
| "A": "Illumina-NovaSeq", | ||||||||||||||||||||||||||||||||||||||||||||
| "NB": "Illumina-MiniSeq", | ||||||||||||||||||||||||||||||||||||||||||||
| "LH": "Illumina-NovaSeqX", | ||||||||||||||||||||||||||||||||||||||||||||
| "SH": "Illumina-MiSeq", | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
| } # Fallback mapping if machine_types.tsv is unavailable. | ||||||||||||||||||||||||||||||||||||||||||||
| MACHINE_TYPES_URL = ( | ||||||||||||||||||||||||||||||||||||||||||||
| "https://raw.githubusercontent.com/PennChopMicrobiomeProgram/" | ||||||||||||||||||||||||||||||||||||||||||||
| "SampleRegistry/master/sample_registry/data/machine_types.tsv" | ||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||||||||||||||||
| with urlopen(MACHINE_TYPES_URL, timeout=10) as response: | ||||||||||||||||||||||||||||||||||||||||||||
| rows = list( | ||||||||||||||||||||||||||||||||||||||||||||
| csv.reader(response.read().decode("utf-8").splitlines(), delimiter="\t") | ||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||
| if rows and rows[0] and rows[0][0].lower() in {"instrument_code", "code"}: | ||||||||||||||||||||||||||||||||||||||||||||
| rows = rows[1:] | ||||||||||||||||||||||||||||||||||||||||||||
| MACHINE_TYPES = { | ||||||||||||||||||||||||||||||||||||||||||||
| row[0].strip(): row[1].strip() | ||||||||||||||||||||||||||||||||||||||||||||
| for row in rows | ||||||||||||||||||||||||||||||||||||||||||||
| if len(row) >= 2 and row[0].strip() and row[1].strip() | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
| if not MACHINE_TYPES: | ||||||||||||||||||||||||||||||||||||||||||||
| raise ValueError("machine_types.tsv contained no usable rows") | ||||||||||||||||||||||||||||||||||||||||||||
| except (URLError, TimeoutError, ValueError) as exc: | ||||||||||||||||||||||||||||||||||||||||||||
| warnings.warn( | ||||||||||||||||||||||||||||||||||||||||||||
| f"Falling back to bundled machine types; unable to load {MACHINE_TYPES_URL}: {exc}", | ||||||||||||||||||||||||||||||||||||||||||||
| RuntimeWarning, | ||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||
| MACHINE_TYPES = MACHINE_TYPES_FALLBACK | ||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+37
to
+42
|
||||||||||||||||||||||||||||||||||||||||||||
| except (URLError, TimeoutError, ValueError) as exc: | |
| warnings.warn( | |
| f"Falling back to bundled machine types; unable to load {MACHINE_TYPES_URL}: {exc}", | |
| RuntimeWarning, | |
| ) | |
| MACHINE_TYPES = MACHINE_TYPES_FALLBACK | |
| except (URLError, TimeoutError) as exc: | |
| warnings.warn( | |
| f"Falling back to bundled machine types; unable to load {MACHINE_TYPES_URL}: {exc}", | |
| RuntimeWarning, | |
| ) | |
| MACHINE_TYPES = MACHINE_TYPES_FALLBACK | |
| except ValueError as exc: | |
| if str(exc) == "machine_types.tsv contained no usable rows": | |
| warnings.warn( | |
| f"Falling back to bundled machine types; unable to load {MACHINE_TYPES_URL}: {exc}", | |
| RuntimeWarning, | |
| ) | |
| MACHINE_TYPES = MACHINE_TYPES_FALLBACK | |
| else: | |
| raise |
Copilot
AI
Jan 30, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The import-time network fetch is the stated design goal, but be aware of operational implications: (1) Python caches modules after first import, so subsequent imports in the same process won't make new requests; (2) However, every new Python process will make a request, which could cause issues in high-frequency scenarios like AWS Lambda cold starts or parallel test execution; (3) The 10-second timeout means slow networks could delay application startup. These tradeoffs are acceptable for many use cases but worth documenting.
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1,8 +1,10 @@ | ||||||
| import gzip | ||||||
| import importlib | ||||||
| from urllib.error import URLError | ||||||
|
|
||||||
| import pytest | ||||||
| from pathlib import Path | ||||||
|
|
||||||
| from seqBackupLib.backup import DEFAULT_MIN_FILE_SIZE | ||||||
| from seqBackupLib.illumina import IlluminaDir, IlluminaFastq, MACHINE_TYPES | ||||||
|
|
||||||
|
|
||||||
| machine_fixtures = { | ||||||
|
|
@@ -16,8 +18,44 @@ | |||||
| } | ||||||
|
|
||||||
|
|
||||||
| @pytest.mark.parametrize("machine_type", MACHINE_TYPES.keys()) | ||||||
| def test_illumina_fastq(machine_type, request): | ||||||
| @pytest.fixture | ||||||
| def illumina_module(monkeypatch): | ||||||
| tsv_rows = ["instrument_code\tmachine_type"] | ||||||
| fallback = { | ||||||
| "VH": "Illumina-NextSeq", | ||||||
| "D": "Illumina-HiSeq", | ||||||
| "M": "Illumina-MiSeq", | ||||||
| "A": "Illumina-NovaSeq", | ||||||
| "NB": "Illumina-MiniSeq", | ||||||
| "LH": "Illumina-NovaSeqX", | ||||||
| "SH": "Illumina-MiSeq", | ||||||
| } | ||||||
| tsv_rows.extend(f"{code}\t{machine}" for code, machine in fallback.items()) | ||||||
| tsv = "\n".join(tsv_rows) + "\n" | ||||||
|
|
||||||
| class FakeResponse: | ||||||
| def __init__(self, data: str): | ||||||
| self._data = data | ||||||
|
|
||||||
| def read(self): | ||||||
| return self._data.encode("utf-8") | ||||||
|
|
||||||
| def __enter__(self): | ||||||
| return self | ||||||
|
|
||||||
| def __exit__(self, exc_type, exc, tb): | ||||||
| return False | ||||||
|
|
||||||
| monkeypatch.setattr( | ||||||
| "urllib.request.urlopen", lambda *args, **kwargs: FakeResponse(tsv) | ||||||
| ) | ||||||
| import seqBackupLib.illumina as illumina | ||||||
|
|
||||||
| return importlib.reload(illumina) | ||||||
|
Comment on lines
+22
to
+54
|
||||||
|
|
||||||
|
|
||||||
| @pytest.mark.parametrize("machine_type", machine_fixtures.keys()) | ||||||
|
||||||
| def test_illumina_fastq(machine_type, request, illumina_module): | ||||||
| fixture_name = machine_fixtures.get(machine_type) | ||||||
| if not fixture_name: | ||||||
| raise ValueError( | ||||||
|
|
@@ -27,18 +65,18 @@ def test_illumina_fastq(machine_type, request): | |||||
| fp = request.getfixturevalue(fixture_name) | ||||||
|
|
||||||
| with gzip.open(fp / "Undetermined_S0_L001_R1_001.fastq.gz", "rt") as f: | ||||||
| r1 = IlluminaFastq(f) | ||||||
| r1 = illumina_module.IlluminaFastq(f) | ||||||
|
|
||||||
| print("FASTQ info: ", r1.fastq_info, "\nFolder info: ", r1.folder_info) | ||||||
| assert r1.machine_type == MACHINE_TYPES[machine_type] | ||||||
| assert r1.machine_type == illumina_module.MACHINE_TYPES[machine_type] | ||||||
| assert r1.check_fp_vs_content()[0], r1.check_fp_vs_content() | ||||||
| assert not r1.check_file_size(DEFAULT_MIN_FILE_SIZE) | ||||||
| assert r1.check_file_size(100) | ||||||
| assert r1.check_index_read_exists() | ||||||
|
|
||||||
|
|
||||||
| @pytest.mark.parametrize("machine_type", MACHINE_TYPES.keys()) | ||||||
| def test_illumina_dir(machine_type, request): | ||||||
| @pytest.mark.parametrize("machine_type", machine_fixtures.keys()) | ||||||
| def test_illumina_dir(machine_type, request, illumina_module): | ||||||
| fixture_name = machine_fixtures.get(machine_type) | ||||||
| if not fixture_name: | ||||||
| raise ValueError( | ||||||
|
|
@@ -47,14 +85,51 @@ def test_illumina_dir(machine_type, request): | |||||
|
|
||||||
| fp = request.getfixturevalue(fixture_name) | ||||||
|
|
||||||
| d = IlluminaDir(fp.name) | ||||||
| d = illumina_module.IlluminaDir(fp.name) | ||||||
|
||||||
| d = illumina_module.IlluminaDir(fp.name) | |
| illumina_module.IlluminaDir(fp.name) |
Copilot
AI
Jan 30, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The FakeResponse class is duplicated in both illumina_module fixture and test_load_machine_types_from_tsv. Consider extracting this into a helper function or a separate fixture to reduce duplication and improve maintainability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The exception handler should catch
OSErrorinstead of justURLErrorandTimeoutError. WhileURLErroris a subclass ofOSErrorand will catch many network errors, otherOSErrorsubclasses likeConnectionRefusedError,ConnectionResetError, and SSL-related errors can also be raised byurlopen. CatchingOSErrorwould provide more comprehensive error handling and ensure the fallback is used for all network-related failures. Alternatively, catchExceptionif you want to ensure any failure triggers the fallback.