From 1414449580bc6b40756d0dc042600aa9d07ec998 Mon Sep 17 00:00:00 2001 From: Yasset Perez-Riverol Date: Wed, 1 Jul 2026 17:16:39 +0100 Subject: [PATCH 1/9] feat(cli): expose -w/--parallel-files across-file concurrency; wire download-px-raw-files (protocol/threads/parallel) --- pridepy/download/client.py | 11 ++- pridepy/download/proteomexchange.py | 12 +++- pridepy/pridepy.py | 76 +++++++++++++++++++++ pridepy/tests/test_cli_flatten.py | 100 ++++++++++++++++++++++++++-- 4 files changed, 191 insertions(+), 8 deletions(-) diff --git a/pridepy/download/client.py b/pridepy/download/client.py index 5dd10f7..16a5db9 100644 --- a/pridepy/download/client.py +++ b/pridepy/download/client.py @@ -318,8 +318,17 @@ def download_px_raw_files( output_folder: str, skip_if_downloaded_already: bool = True, flatten: bool = True, + parallel_files: int = 1, + download_threads: int = 1, + protocol: str = "ftp", ) -> None: """Delegate to :meth:`ProteomeXchangeProvider.download_from_accession_or_url`.""" return ProteomeXchangeProvider().download_from_accession_or_url( - px_id_or_url, output_folder, skip_if_downloaded_already, flatten=flatten + px_id_or_url, + output_folder, + skip_if_downloaded_already, + flatten=flatten, + parallel_files=parallel_files, + download_threads=download_threads, + protocol=protocol, ) diff --git a/pridepy/download/proteomexchange.py b/pridepy/download/proteomexchange.py index 32ea72e..958075e 100644 --- a/pridepy/download/proteomexchange.py +++ b/pridepy/download/proteomexchange.py @@ -170,13 +170,19 @@ def download_from_accession_or_url( output_folder: str, skip_if_downloaded_already: bool = True, flatten: bool = True, + parallel_files: int = 1, + download_threads: int = 1, + protocol: str = "ftp", ) -> None: """End-to-end: resolve XML, list files, partition by scheme, download. Convenience for the ``download-px-raw-files`` CLI command — combines :meth:`list_files` and :meth:`download_files` with the original ``download_px_raw_files`` defaults (skip-if-downloaded-already - defaults to ``True``, no parallel workers). + defaults to ``True``). ``parallel_files`` controls across-file + concurrency and ``download_threads`` controls per-file HTTP Range + segments; ``protocol`` flows into :meth:`download_files` (ftp/http(s) + are handled directly today). """ records = self.list_files(px_id_or_url) if not records: @@ -187,6 +193,8 @@ def download_from_accession_or_url( records=records, output_folder=output_folder, skip_if_downloaded_already=skip_if_downloaded_already, - protocol="ftp", + protocol=protocol, flatten=flatten, + parallel_files=parallel_files, + download_threads=download_threads, ) diff --git a/pridepy/pridepy.py b/pridepy/pridepy.py index 17723cf..c4855df 100644 --- a/pridepy/pridepy.py +++ b/pridepy/pridepy.py @@ -60,6 +60,15 @@ def main(): type=click.IntRange(1, 32), help="Number of threads for each file download. Default is 1.", ) +@click.option( + "-w", + "--parallel-files", + "parallel_files", + default=1, + type=click.IntRange(1, 32), + help="Number of files to download in parallel (across-file concurrency). " + "Combine with -t/--threads (per-file segments). Default is 1.", +) @click.option( "--preserve-structure", is_flag=True, @@ -75,6 +84,7 @@ def download_all_public_raw_files( aspera_maximum_bandwidth: str = "50M", checksum_check: bool = False, download_threads: int = 1, + parallel_files: int = 1, preserve_structure: bool = False, ): """ @@ -88,6 +98,7 @@ def download_all_public_raw_files( aspera_maximum_bandwidth (str): Maximum bandwidth for Aspera protocol. Default is 100M. checksum_check (bool): Flag to download checksum file for the project. Default is False. download_threads (int): Number of threads for each file download. Default is 1. + parallel_files (int): Number of files to download in parallel. Default is 1. """ raw_files = Files() @@ -105,6 +116,7 @@ def download_all_public_raw_files( aspera_maximum_bandwidth=aspera_maximum_bandwidth, checksum_check=checksum_check, download_threads=download_threads, + parallel_files=parallel_files, flatten=not preserve_structure, ) @@ -161,6 +173,15 @@ def download_all_public_raw_files( type=click.IntRange(1, 32), help="Number of threads for each file download. Default is 1.", ) +@click.option( + "-w", + "--parallel-files", + "parallel_files", + default=1, + type=click.IntRange(1, 32), + help="Number of files to download in parallel (across-file concurrency). " + "Combine with -t/--threads (per-file segments). Default is 1.", +) @click.option( "--preserve-structure", is_flag=True, @@ -177,6 +198,7 @@ def download_all_public_category_files( checksum_check: bool = False, category: str = "RAW", download_threads: int = 1, + parallel_files: int = 1, preserve_structure: bool = False, ): """ @@ -191,6 +213,7 @@ def download_all_public_category_files( checksum_check (bool): If True, downloads the checksum file for the project. category (str): Comma-separated categories of files to download (e.g. RAW or RAW,SEARCH). download_threads (int): Number of threads for each file download. Default is 1. + parallel_files (int): Number of files to download in parallel. Default is 1. """ valid_categories = {"RAW", "PEAK", "SEARCH", "RESULT", "SPECTRUM_LIBRARY", "OTHER", "FASTA"} @@ -218,6 +241,7 @@ def download_all_public_category_files( checksum_check=checksum_check, categories=categories, download_threads=download_threads, + parallel_files=parallel_files, flatten=not preserve_structure, ) @@ -332,6 +356,30 @@ def download_file_by_name( default=False, help="Skip the download if the file has already been downloaded.", ) +@click.option( + "-p", + "--protocol", + default="ftp", + type=PROTOCOL_CHOICES, + help="Protocol to use for download: ftp, aspera, globus, s3. Default is ftp with fallback enabled.", +) +@click.option( + "-t", + "--threads", + "download_threads", + default=1, + type=click.IntRange(1, 32), + help="Number of threads for each file download. Default is 1.", +) +@click.option( + "-w", + "--parallel-files", + "parallel_files", + default=1, + type=click.IntRange(1, 32), + help="Number of files to download in parallel (across-file concurrency). " + "Combine with -t/--threads (per-file segments). Default is 1.", +) @click.option( "--preserve-structure", is_flag=True, @@ -343,6 +391,9 @@ def download_px_raw_files( accession: str, output_folder: str, skip_if_downloaded_already: bool, + protocol: str = "ftp", + download_threads: int = 1, + parallel_files: int = 1, preserve_structure: bool = False, ): """CLI wrapper to download raw files via ProteomeXchange XML.""" @@ -353,6 +404,9 @@ def download_px_raw_files( output_folder, skip_if_downloaded_already, flatten=not preserve_structure, + protocol=protocol, + download_threads=download_threads, + parallel_files=parallel_files, ) @@ -602,6 +656,15 @@ def _read_url_arguments(url_list_path, urls_csv=None): type=click.IntRange(1, 32), help="Number of threads for each file download. Default is 1.", ) +@click.option( + "-w", + "--parallel-files", + "parallel_files", + default=1, + type=click.IntRange(1, 32), + help="Number of files to download in parallel (across-file concurrency). " + "Combine with -t/--threads (per-file segments). Default is 1.", +) @click.option( "--preserve-structure", is_flag=True, @@ -619,6 +682,7 @@ def download_files_by_list( aspera_maximum_bandwidth, checksum_check, download_threads, + parallel_files: int = 1, preserve_structure: bool = False, ): """Download a named subset of files from a PRIDE project.""" @@ -635,6 +699,7 @@ def download_files_by_list( aspera_maximum_bandwidth=aspera_maximum_bandwidth, checksum_check=checksum_check, download_threads=download_threads, + parallel_files=parallel_files, flatten=not preserve_structure, ) @@ -693,6 +758,15 @@ def download_files_by_list( type=click.IntRange(1, 32), help="Number of threads for each file download. Default is 1.", ) +@click.option( + "-w", + "--parallel-files", + "parallel_files", + default=1, + type=click.IntRange(1, 32), + help="Number of files to download in parallel (across-file concurrency). " + "Combine with -t/--threads (per-file segments). Default is 1.", +) def download_files_by_url( url_list_path, urls_csv, @@ -701,6 +775,7 @@ def download_files_by_url( protocol, checksum_check, download_threads, + parallel_files: int = 1, ): """Download files from raw URLs (http/https/ftp), dispatched by scheme.""" urls = _read_url_arguments(url_list_path, urls_csv) @@ -711,6 +786,7 @@ def download_files_by_url( skip_if_downloaded_already=skip_if_downloaded_already, protocol=protocol, download_threads=download_threads, + parallel_files=parallel_files, checksum_check=checksum_check, ) diff --git a/pridepy/tests/test_cli_flatten.py b/pridepy/tests/test_cli_flatten.py index 92c00df..01f887c 100644 --- a/pridepy/tests/test_cli_flatten.py +++ b/pridepy/tests/test_cli_flatten.py @@ -23,7 +23,23 @@ def test_download_all_public_raw_files_flattens_by_default(self): kwargs = files_cls.return_value.download_all_raw_files.call_args.kwargs assert kwargs["flatten"] is True assert kwargs["download_threads"] == 1 - assert "parallel_files" not in kwargs + assert kwargs["parallel_files"] == 1 + + def test_download_all_public_raw_files_parallel_files(self): + with patch("pridepy.pridepy.Files") as files_cls: + self._invoke( + [ + "download-all-public-raw-files", + "-a", + "MSV000012345", + "-o", + "/tmp/x", + "-w", + "8", + ] + ) + kwargs = files_cls.return_value.download_all_raw_files.call_args.kwargs + assert kwargs["parallel_files"] == 8 def test_download_all_public_raw_files_threads(self): with patch("pridepy.pridepy.Files") as files_cls: @@ -40,7 +56,7 @@ def test_download_all_public_raw_files_threads(self): ) kwargs = files_cls.return_value.download_all_raw_files.call_args.kwargs assert kwargs["download_threads"] == 4 - assert "parallel_files" not in kwargs + assert kwargs["parallel_files"] == 1 def test_download_all_public_raw_files_preserve_structure(self): with patch("pridepy.pridepy.Files") as files_cls: @@ -74,7 +90,25 @@ def test_download_all_public_category_files_preserve_structure(self): kwargs = files_cls.return_value.download_all_category_files.call_args.kwargs assert kwargs["flatten"] is False assert kwargs["download_threads"] == 1 - assert "parallel_files" not in kwargs + assert kwargs["parallel_files"] == 1 + + def test_download_all_public_category_files_parallel_files(self): + with patch("pridepy.pridepy.Files") as files_cls: + self._invoke( + [ + "download-all-public-category-files", + "-a", + "MSV000012345", + "-o", + "/tmp/x", + "-c", + "RAW", + "-w", + "8", + ] + ) + kwargs = files_cls.return_value.download_all_category_files.call_args.kwargs + assert kwargs["parallel_files"] == 8 def test_download_files_by_list_preserve_structure(self): with patch("pridepy.pridepy.Files") as files_cls: @@ -93,7 +127,25 @@ def test_download_files_by_list_preserve_structure(self): kwargs = files_cls.return_value.download_files_by_list.call_args.kwargs assert kwargs["flatten"] is False assert kwargs["download_threads"] == 1 - assert "parallel_files" not in kwargs + assert kwargs["parallel_files"] == 1 + + def test_download_files_by_list_parallel_files(self): + with patch("pridepy.pridepy.Files") as files_cls: + self._invoke( + [ + "download-files-by-list", + "-a", + "MSV000012345", + "-o", + "/tmp/x", + "-f", + "a.raw", + "-w", + "8", + ] + ) + kwargs = files_cls.return_value.download_files_by_list.call_args.kwargs + assert kwargs["parallel_files"] == 8 def test_download_files_by_url_threads(self): with patch("pridepy.pridepy.Files") as files_cls: @@ -110,7 +162,23 @@ def test_download_files_by_url_threads(self): ) kwargs = files_cls.download_files_by_url.call_args.kwargs assert kwargs["download_threads"] == 4 - assert "parallel_files" not in kwargs + assert kwargs["parallel_files"] == 1 + + def test_download_files_by_url_parallel_files(self): + with patch("pridepy.pridepy.Files") as files_cls: + self._invoke( + [ + "download-files-by-url", + "-u", + "https://example.org/a.raw", + "-o", + "/tmp/x", + "-w", + "8", + ] + ) + kwargs = files_cls.download_files_by_url.call_args.kwargs + assert kwargs["parallel_files"] == 8 def test_download_px_raw_files_preserve_structure(self): with patch("pridepy.pridepy.Files") as files_cls: @@ -126,3 +194,25 @@ def test_download_px_raw_files_preserve_structure(self): ) kwargs = files_cls.return_value.download_px_raw_files.call_args.kwargs assert kwargs["flatten"] is False + + def test_download_px_raw_files_protocol_threads_parallel(self): + with patch("pridepy.pridepy.Files") as files_cls: + self._invoke( + [ + "download-px-raw-files", + "-a", + "PXD000001", + "-o", + "/tmp/x", + "-w", + "8", + "-t", + "4", + "-p", + "ftp", + ] + ) + kwargs = files_cls.return_value.download_px_raw_files.call_args.kwargs + assert kwargs["parallel_files"] == 8 + assert kwargs["download_threads"] == 4 + assert kwargs["protocol"] == "ftp" From 8a56efc19f9b9c8032988c9704a559da60122621 Mon Sep 17 00:00:00 2001 From: Yasset Perez-Riverol Date: Wed, 1 Jul 2026 17:21:52 +0100 Subject: [PATCH 2/9] feat(download): add opt-in iProX Aspera transport for download-px-raw-files Adds IproxProvider.aspera_download (ascp on port 33001, credentials via ASPERA_SCP_PASS env only) and routes ProteomeXchangeProvider through it when --protocol aspera is selected, gated on iProX-hosted URLs. Plumbs --iprox-user/--iprox-password (env: IPROX_USER/IPROX_ASPERA_PASSWORD) through Client.download_px_raw_files and the CLI. HTTP-parallel remains the default transport. --- pridepy/download/client.py | 4 + pridepy/download/iprox.py | 64 ++++++++++++ pridepy/download/proteomexchange.py | 40 ++++++- pridepy/pridepy.py | 24 +++++ pridepy/tests/test_iprox_aspera.py | 156 ++++++++++++++++++++++++++++ 5 files changed, 287 insertions(+), 1 deletion(-) create mode 100644 pridepy/tests/test_iprox_aspera.py diff --git a/pridepy/download/client.py b/pridepy/download/client.py index 16a5db9..6707810 100644 --- a/pridepy/download/client.py +++ b/pridepy/download/client.py @@ -321,6 +321,8 @@ def download_px_raw_files( parallel_files: int = 1, download_threads: int = 1, protocol: str = "ftp", + iprox_user: Optional[str] = None, + iprox_password: Optional[str] = None, ) -> None: """Delegate to :meth:`ProteomeXchangeProvider.download_from_accession_or_url`.""" return ProteomeXchangeProvider().download_from_accession_or_url( @@ -331,4 +333,6 @@ def download_px_raw_files( parallel_files=parallel_files, download_threads=download_threads, protocol=protocol, + iprox_user=iprox_user, + iprox_password=iprox_password, ) diff --git a/pridepy/download/iprox.py b/pridepy/download/iprox.py index db5e7fc..d59de97 100644 --- a/pridepy/download/iprox.py +++ b/pridepy/download/iprox.py @@ -14,6 +14,7 @@ import logging import os import re +import subprocess import defusedxml.ElementTree as ET from typing import ClassVar, Dict, List, Optional from urllib.parse import urlparse @@ -34,6 +35,9 @@ class IproxProvider(Provider): PX_XML_URL_TEMPLATE: ClassVar[str] = ( "http://download.iprox.org/{accession}/PX_{accession}.xml" ) + ASPERA_HOST: ClassVar[str] = "download.iprox.org" + ASPERA_PORT: ClassVar[str] = "33001" + ASPERA_ROOT: ClassVar[str] = "/data/iprox" # iProX PX XML uses the same PSI-MS cvParam "name" values as JPOST PROXI, # so we reuse JpostProvider's category map. PX_CATEGORY_MAP: ClassVar[Dict[str, str]] = JpostProvider.PROXI_CATEGORY_MAP @@ -45,6 +49,66 @@ def matches(accession: str) -> bool: return False return bool(re.fullmatch(r"IPX\d{7,10}", accession.upper())) + @staticmethod + def _ascp_binary() -> str: + # Reuse PRIDE's bundled ascp binary resolution. + from pridepy.download.pride import PrideProvider + return PrideProvider.get_ascp_binary() + + @classmethod + def aspera_download( + cls, + urls: List[str], + output_folder: str, + relative_paths: List[Optional[str]], + user: Optional[str], + password: Optional[str], + maximum_bandwidth: str = "100M", + skip_if_downloaded_already: bool = False, + ) -> None: + """Download iProX-hosted URLs via ascp on port 33001. + + Requires iProX account credentials; the password is passed to the + subprocess through ASPERA_SCP_PASS (never argv). + """ + if not user or not password: + raise ValueError( + "iProX Aspera requires credentials: pass --iprox-user and " + "--iprox-password (or IPROX_USER / IPROX_ASPERA_PASSWORD), or " + "use the default parallel HTTP transport instead." + ) + ascp = cls._ascp_binary() + env = dict(os.environ) + env["ASPERA_SCP_PASS"] = password + os.makedirs(output_folder, exist_ok=True) + failed: List[str] = [] + for idx, url in enumerate(urls): + path = urlparse(url).path.lstrip("/") # e.g. IPX.../.../a.raw + source = f"{user}@{cls.ASPERA_HOST}:{cls.ASPERA_ROOT}/{path}" + relpath = relative_paths[idx] if idx < len(relative_paths) else None + dest = os.path.join(output_folder, relpath) if relpath else output_folder + dest_parent = os.path.dirname(dest) or output_folder + os.makedirs(dest_parent, exist_ok=True) + if skip_if_downloaded_already and os.path.exists(dest): + logging.info(f"Skipping download as file already exists: {dest}") + continue + argv = [ + ascp, "-QT", "-P", cls.ASPERA_PORT, "-l", maximum_bandwidth, + "-k", "2", source, dest, + ] + logging.info( + "Aspera: %s -> %s", source.replace(password, "***"), dest + ) + try: + subprocess.run(argv, check=True, env=env) + except subprocess.CalledProcessError as e: + logging.error(f"iProX Aspera failed for {url}: {e}") + failed.append(url) + if failed: + raise RuntimeError( + f"iProX Aspera download failed for {len(failed)} file(s): {failed}" + ) + @staticmethod def _get_public_root(accession: str) -> str: return f"/{accession.upper()}" diff --git a/pridepy/download/proteomexchange.py b/pridepy/download/proteomexchange.py index 958075e..10e4374 100644 --- a/pridepy/download/proteomexchange.py +++ b/pridepy/download/proteomexchange.py @@ -26,10 +26,11 @@ import posixpath import re import defusedxml.ElementTree as ET -from typing import ClassVar, Dict, List +from typing import ClassVar, Dict, List, Optional from urllib.parse import urlparse from pridepy.download.base import Provider +from pridepy.download.util import flatten_relative_paths from pridepy.util.api_handling import Util @@ -173,6 +174,8 @@ def download_from_accession_or_url( parallel_files: int = 1, download_threads: int = 1, protocol: str = "ftp", + iprox_user: Optional[str] = None, + iprox_password: Optional[str] = None, ) -> None: """End-to-end: resolve XML, list files, partition by scheme, download. @@ -183,11 +186,46 @@ def download_from_accession_or_url( concurrency and ``download_threads`` controls per-file HTTP Range segments; ``protocol`` flows into :meth:`download_files` (ftp/http(s) are handled directly today). + + When ``protocol == "aspera"``, iProX-hosted files are routed through + :meth:`IproxProvider.aspera_download` instead of the HTTP/FTP path + (opt-in, requires ``iprox_user``/``iprox_password``). """ records = self.list_files(px_id_or_url) if not records: logging.info("No Associated raw file URIs found in PX XML") return + + if protocol.lower() == "aspera": + from pridepy.download.iprox import IproxProvider + iprox_urls, rels = [], [] + for r in records: + loc = self.get_download_url(r, protocol) + if "download.iprox.org" in loc: + iprox_urls.append(loc) + rels.append(r.get("relativePath")) + if not iprox_urls: + raise ValueError( + "Aspera requested but no iProX-hosted files found in this dataset." + ) + if flatten: + sources = [ + rel if rel else urlparse(url).path + for url, rel in zip(iprox_urls, rels) + ] + dest_rels: List[Optional[str]] = flatten_relative_paths(sources) + else: + dest_rels = rels + IproxProvider.aspera_download( + urls=iprox_urls, + output_folder=output_folder, + relative_paths=dest_rels, + user=iprox_user, + password=iprox_password, + skip_if_downloaded_already=skip_if_downloaded_already, + ) + return + self.download_files( accession=px_id_or_url, records=records, diff --git a/pridepy/pridepy.py b/pridepy/pridepy.py index c4855df..a260946 100644 --- a/pridepy/pridepy.py +++ b/pridepy/pridepy.py @@ -1,6 +1,8 @@ #!/usr/bin/env python3 import asyncio import logging +from typing import Optional + import click from pridepy.download.client import Client as Files from pridepy.pdc import download_pdc_files as run_pdc_download @@ -387,6 +389,24 @@ def download_file_by_name( help="Recreate the dataset's subdirectory layout under the output folder. " "By default files are downloaded flat into the output folder.", ) +@click.option( + "--iprox-user", + "iprox_user", + envvar="IPROX_USER", + default=None, + type=str, + help="iProX account username. Only used with --protocol aspera.", +) +@click.option( + "--iprox-password", + "iprox_password", + envvar="IPROX_ASPERA_PASSWORD", + default=None, + type=str, + help="iProX account password. Only used with --protocol aspera. " + "Best supplied via the IPROX_ASPERA_PASSWORD environment variable " + "rather than on the command line.", +) def download_px_raw_files( accession: str, output_folder: str, @@ -395,6 +415,8 @@ def download_px_raw_files( download_threads: int = 1, parallel_files: int = 1, preserve_structure: bool = False, + iprox_user: Optional[str] = None, + iprox_password: Optional[str] = None, ): """CLI wrapper to download raw files via ProteomeXchange XML.""" files = Files() @@ -407,6 +429,8 @@ def download_px_raw_files( protocol=protocol, download_threads=download_threads, parallel_files=parallel_files, + iprox_user=iprox_user, + iprox_password=iprox_password, ) diff --git a/pridepy/tests/test_iprox_aspera.py b/pridepy/tests/test_iprox_aspera.py new file mode 100644 index 0000000..b4600b9 --- /dev/null +++ b/pridepy/tests/test_iprox_aspera.py @@ -0,0 +1,156 @@ +"""iProX Aspera command construction + credential handling.""" +import os +import re +import subprocess +import tempfile +from unittest import TestCase +from unittest.mock import patch, MagicMock + +import pytest + +from pridepy.download.iprox import IproxProvider + + +class TestIproxAspera(TestCase): + def test_builds_ascp_source_and_env(self): + url = "http://download.iprox.org/IPX0003578000/IPX0003578001/a.raw" + with tempfile.TemporaryDirectory() as tmp, \ + patch("pridepy.download.iprox.subprocess.run") as run, \ + patch.object(IproxProvider, "_ascp_binary", return_value="/bin/ascp"): + run.return_value = MagicMock(returncode=0) + IproxProvider.aspera_download( + urls=[url], + output_folder=tmp, + relative_paths=["IPX0003578001/a.raw"], + user="bob", + password="secret", + maximum_bandwidth="100M", + ) + args, kwargs = run.call_args + argv = args[0] + assert argv[0] == "/bin/ascp" + assert "33001" in argv + assert "bob@download.iprox.org:/data/iprox/IPX0003578000/IPX0003578001/a.raw" in argv + # password only via env, never argv + assert "secret" not in argv + assert kwargs["env"]["ASPERA_SCP_PASS"] == "secret" + + def test_missing_credentials_raises(self): + with pytest.raises(ValueError, match="credentials"): + IproxProvider.aspera_download( + urls=["http://download.iprox.org/IPX1/a.raw"], + output_folder="/tmp/x", + relative_paths=["a.raw"], + user=None, + password=None, + ) + + def test_failed_transfer_raises_runtime_error(self): + url = "http://download.iprox.org/IPX0003578000/IPX0003578001/a.raw" + with tempfile.TemporaryDirectory() as tmp, \ + patch("pridepy.download.iprox.subprocess.run") as run, \ + patch.object(IproxProvider, "_ascp_binary", return_value="/bin/ascp"): + run.side_effect = subprocess.CalledProcessError(1, ["ascp"]) + with pytest.raises(RuntimeError, match=re.escape(url)): + IproxProvider.aspera_download( + urls=[url], + output_folder=tmp, + relative_paths=["IPX0003578001/a.raw"], + user="bob", + password="secret", + maximum_bandwidth="100M", + ) + + def test_skip_if_downloaded_already_skips_existing_file(self): + url = "http://download.iprox.org/IPX0003578000/IPX0003578001/a.raw" + with tempfile.TemporaryDirectory() as tmp, \ + patch("pridepy.download.iprox.subprocess.run") as run, \ + patch.object(IproxProvider, "_ascp_binary", return_value="/bin/ascp"): + dest_dir = os.path.join(tmp, "IPX0003578001") + os.makedirs(dest_dir, exist_ok=True) + dest_file = os.path.join(dest_dir, "a.raw") + with open(dest_file, "w") as f: + f.write("already here") + + IproxProvider.aspera_download( + urls=[url], + output_folder=tmp, + relative_paths=["IPX0003578001/a.raw"], + user="bob", + password="secret", + maximum_bandwidth="100M", + skip_if_downloaded_already=True, + ) + run.assert_not_called() + + +class TestPxAsperaRouting(TestCase): + def test_px_aspera_routes_to_iprox(self): + from pridepy.download.proteomexchange import ProteomeXchangeProvider + prov = ProteomeXchangeProvider() + rec = { + "publicFileLocations": [ + {"name": "FTP Protocol", + "value": "http://download.iprox.org/IPX1/IPX2/a.raw"} + ], + "relativePath": "IPX2/a.raw", + } + with patch.object(prov, "list_files", return_value=[rec]), \ + patch("pridepy.download.iprox.IproxProvider.aspera_download") as asp: + prov.download_from_accession_or_url( + "PXD000001", "/tmp/x", protocol="aspera", + iprox_user="bob", iprox_password="secret", + ) + asp.assert_called_once() + assert asp.call_args.kwargs["user"] == "bob" + + def test_px_aspera_flattens_relative_paths_by_default(self): + """Aspera branch should honor flatten=True like the HTTP/FTP path: + dataset subtree paths collapse to deduplicated basenames.""" + from pridepy.download.proteomexchange import ProteomeXchangeProvider + prov = ProteomeXchangeProvider() + records = [ + { + "publicFileLocations": [ + {"name": "FTP Protocol", + "value": "http://download.iprox.org/IPX1/run1/a.raw"} + ], + "relativePath": "run1/a.raw", + }, + { + "publicFileLocations": [ + {"name": "FTP Protocol", + "value": "http://download.iprox.org/IPX1/run2/a.raw"} + ], + "relativePath": "run2/a.raw", + }, + ] + with patch.object(prov, "list_files", return_value=records), \ + patch("pridepy.download.iprox.IproxProvider.aspera_download") as asp: + prov.download_from_accession_or_url( + "PXD000001", "/tmp/x", protocol="aspera", flatten=True, + iprox_user="bob", iprox_password="secret", + ) + asp.assert_called_once() + rels = asp.call_args.kwargs["relative_paths"] + # Flattened + de-duped basenames, no subdirectories preserved. + assert set(rels) == {"a.raw", "a_1.raw"} + + def test_px_aspera_preserves_structure_when_not_flattened(self): + from pridepy.download.proteomexchange import ProteomeXchangeProvider + prov = ProteomeXchangeProvider() + rec = { + "publicFileLocations": [ + {"name": "FTP Protocol", + "value": "http://download.iprox.org/IPX1/IPX2/a.raw"} + ], + "relativePath": "IPX2/a.raw", + } + with patch.object(prov, "list_files", return_value=[rec]), \ + patch("pridepy.download.iprox.IproxProvider.aspera_download") as asp: + prov.download_from_accession_or_url( + "PXD000001", "/tmp/x", protocol="aspera", flatten=False, + iprox_user="bob", iprox_password="secret", + ) + asp.assert_called_once() + assert asp.call_args.kwargs["relative_paths"] == ["IPX2/a.raw"] From b91613ea9dd0fcc744aab926743bd046037c16f4 Mon Sep 17 00:00:00 2001 From: Yasset Perez-Riverol Date: Wed, 1 Jul 2026 17:23:06 +0100 Subject: [PATCH 3/9] docs: document -w/--parallel-files and iProX Aspera for PX downloads --- docs/usage.md | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/docs/usage.md b/docs/usage.md index e3885f1..0468d6d 100644 --- a/docs/usage.md +++ b/docs/usage.md @@ -239,7 +239,58 @@ pridepy download-px-raw-files \ | --- | --- | --- | | `-a, --accession` | ProteomeXchange accession (e.g. `PXD039236`). `--px` is a deprecated alias | required | | `-o, --output-folder` | Destination directory | required | +| `-p, --protocol` | Transfer protocol: `ftp`, `aspera`, `globus`, `s3` (FTP-first with fallback) | `ftp` | +| `-w, --parallel-files` | Download 1–32 files concurrently (across-file concurrency) | `1` | +| `-t, --threads` | Parallel HTTP Range threads per file (1–32) for fast per-file downloads | `1` | | `--skip-if-downloaded-already` | Skip files already present locally | off | +| `--preserve-structure` | Recreate the dataset's subdirectory layout under the output folder | off | +| `--iprox-user` | iProX account username (with `--protocol aspera`; env fallback: `IPROX_USER`) | — | +| `--iprox-password` | iProX account password (with `--protocol aspera`; env fallback: `IPROX_ASPERA_PASSWORD`) | — | + +### Fast downloads: parallel files and per-file segments + +Combine `-w` (files in parallel) and `-t` (Range segments per file) for fast bulk downloads. +The total concurrent connections is approximately `parallel_files × threads`. + +**Parallel across files (recommended for most users, no account required):** + +```bash +# Download up to 8 files concurrently from ProteomeXchange +pridepy download-px-raw-files \ + -a PXD077178 \ + -o ./PXD077178 \ + -w 8 +``` + +**Combine parallel files with per-file segments:** + +```bash +# Download 8 files in parallel, each split into 4 Range segments +pridepy download-px-raw-files \ + -a PXD077178 \ + -o ./out \ + -w 8 \ + -t 4 +``` + +### Fast downloads with iProX Aspera (account required) + +iProX offers Aspera (`faspe://`) for very large bulk transfers. Aspera is faster +than HTTP on high-bandwidth connections but requires an iProX account. Combine +`--protocol aspera` with `--iprox-user` and `--iprox-password`: + +```bash +# Download via iProX Aspera with 8-file parallelism +IPROX_ASPERA_PASSWORD=your_password pridepy download-px-raw-files \ + -a PXD077178 \ + -o ./out \ + --protocol aspera \ + --iprox-user your_username \ + -w 8 +``` + +The password is best supplied via the `IPROX_ASPERA_PASSWORD` environment variable +to avoid exposing it on the command line. ### Go directly to the hosting repository (native MassIVE / JPOST / iProX accessions) From 14b00f95ada197bc4f0f6c8b54ceb5bcb96c799f Mon Sep 17 00:00:00 2001 From: Yasset Perez-Riverol Date: Wed, 1 Jul 2026 18:06:17 +0100 Subject: [PATCH 4/9] fix(download): harden iProX Aspera (path traversal, skip logic, exact host, honor -w) + concurrency cap + hidden password prompt --- docs/usage.md | 17 +-- pridepy/download/iprox.py | 122 ++++++++++++++---- pridepy/download/proteomexchange.py | 13 +- pridepy/download/transport.py | 18 +++ pridepy/pridepy.py | 24 ++-- pridepy/tests/test_download_resilience.py | 33 +++++ pridepy/tests/test_iprox_aspera.py | 144 ++++++++++++++++++++++ 7 files changed, 323 insertions(+), 48 deletions(-) diff --git a/docs/usage.md b/docs/usage.md index 0468d6d..c1d7eb8 100644 --- a/docs/usage.md +++ b/docs/usage.md @@ -245,7 +245,10 @@ pridepy download-px-raw-files \ | `--skip-if-downloaded-already` | Skip files already present locally | off | | `--preserve-structure` | Recreate the dataset's subdirectory layout under the output folder | off | | `--iprox-user` | iProX account username (with `--protocol aspera`; env fallback: `IPROX_USER`) | — | -| `--iprox-password` | iProX account password (with `--protocol aspera`; env fallback: `IPROX_ASPERA_PASSWORD`) | — | + +The iProX Aspera password is never accepted as a command-line flag. Set the +`IPROX_ASPERA_PASSWORD` environment variable, or omit it and `pridepy` will +prompt for it securely (hidden input) when `--protocol aspera` is used. ### Fast downloads: parallel files and per-file segments @@ -275,9 +278,10 @@ pridepy download-px-raw-files \ ### Fast downloads with iProX Aspera (account required) -iProX offers Aspera (`faspe://`) for very large bulk transfers. Aspera is faster -than HTTP on high-bandwidth connections but requires an iProX account. Combine -`--protocol aspera` with `--iprox-user` and `--iprox-password`: +iProX offers Aspera for very large bulk transfers. Aspera is faster than HTTP +on high-bandwidth connections but requires an iProX account. Combine +`--protocol aspera` with `--iprox-user`; the password is read from +`IPROX_ASPERA_PASSWORD` or prompted for securely (never passed as a flag): ```bash # Download via iProX Aspera with 8-file parallelism @@ -289,9 +293,6 @@ IPROX_ASPERA_PASSWORD=your_password pridepy download-px-raw-files \ -w 8 ``` -The password is best supplied via the `IPROX_ASPERA_PASSWORD` environment variable -to avoid exposing it on the command line. - ### Go directly to the hosting repository (native MassIVE / JPOST / iProX accessions) Datasets that do not have a ProteomeXchange accession — or where you already @@ -319,7 +320,7 @@ How each repository is enumerated: - **MassIVE** walks the FTPS tree at `massive-ftp.ucsd.edu` (the server requires TLS). MassIVE distributes datasets across versioned root directories (`/v01`–`/vNN`); `pridepy` discovers the correct root automatically. If FTP/FTPS is blocked by the network, `pridepy` falls back to HTTPS: it lists the dataset from the GNPS2 file index (`datasetcache.gnps2.org`) and downloads each file from the ProteoSAFe endpoint at `massive.ucsd.edu` (byte-identical to the FTPS copy). - **JPOST** lists files through the JSON PROXI endpoint at `https://repository.jpostdb.org/proxi/datasets/` and downloads from `ftp.jpostdb.org` over plain FTP. The PROXI listing avoids the source-IP connection limit JPOST enforces on FTP. -- **iProX** fetches the dataset's ProteomeXchange XML from `http://download.iprox.org//PX_.xml`, then downloads each referenced file from the same host over anonymous HTTP (with `Range` support for resume). iProX also exposes Aspera (`faspe://`) with username/password for very large bulk transfers; `pridepy` uses the public HTTP endpoint so no iProX credentials are required. +- **iProX** fetches the dataset's ProteomeXchange XML from `http://download.iprox.org//PX_.xml`, then downloads each referenced file from the same host over anonymous HTTP (with `Range` support for resume). iProX also exposes Aspera with username/password for very large bulk transfers; `pridepy` uses the public HTTP endpoint so no iProX credentials are required. `download-all-public-raw-files` retrieves the files stored under the dataset's `raw/` collection. These direct downloads support resume (REST for FTP, diff --git a/pridepy/download/iprox.py b/pridepy/download/iprox.py index d59de97..608fd22 100644 --- a/pridepy/download/iprox.py +++ b/pridepy/download/iprox.py @@ -15,6 +15,7 @@ import os import re import subprocess +from concurrent.futures import ThreadPoolExecutor, as_completed import defusedxml.ElementTree as ET from typing import ClassVar, Dict, List, Optional from urllib.parse import urlparse @@ -24,6 +25,7 @@ from pridepy.download import registry from pridepy.download.base import Provider from pridepy.download.jpost import JpostProvider +from pridepy.download.transport import _safe_join @registry.register @@ -55,6 +57,49 @@ def _ascp_binary() -> str: from pridepy.download.pride import PrideProvider return PrideProvider.get_ascp_binary() + @classmethod + def _aspera_download_one( + cls, + ascp: str, + url: str, + relpath: Optional[str], + output_folder: str, + user: str, + password: str, + maximum_bandwidth: str, + skip_if_downloaded_already: bool, + env: Dict[str, str], + ) -> Optional[str]: + """Download a single URL via ascp. Returns ``url`` on failure, else None.""" + path = urlparse(url).path.lstrip("/") # e.g. IPX.../.../a.raw + source = f"{user}@{cls.ASPERA_HOST}:{cls.ASPERA_ROOT}/{path}" + if relpath: + dest = _safe_join(output_folder, relpath) + else: + dest = os.path.join(output_folder, os.path.basename(urlparse(url).path)) + dest_parent = os.path.dirname(dest) or output_folder + os.makedirs(dest_parent, exist_ok=True) + if ( + skip_if_downloaded_already + and os.path.isfile(dest) + and os.path.getsize(dest) > 0 + ): + logging.info(f"Skipping download as file already exists: {dest}") + return None + argv = [ + ascp, "-QT", "-P", cls.ASPERA_PORT, "-l", maximum_bandwidth, + "-k", "2", source, dest, + ] + logging.info( + "Aspera: %s -> %s", source.replace(password, "***"), dest + ) + try: + subprocess.run(argv, check=True, env=env) + return None + except subprocess.CalledProcessError as e: + logging.error(f"iProX Aspera failed for {url}: {e}") + return url + @classmethod def aspera_download( cls, @@ -65,45 +110,70 @@ def aspera_download( password: Optional[str], maximum_bandwidth: str = "100M", skip_if_downloaded_already: bool = False, + parallel_files: int = 1, ) -> None: """Download iProX-hosted URLs via ascp on port 33001. Requires iProX account credentials; the password is passed to the - subprocess through ASPERA_SCP_PASS (never argv). + subprocess through ASPERA_SCP_PASS (never argv). When + ``parallel_files`` > 1, transfers run concurrently: each ``ascp`` + invocation is its own subprocess writing its own destination file, so + this is safe. """ if not user or not password: raise ValueError( - "iProX Aspera requires credentials: pass --iprox-user and " - "--iprox-password (or IPROX_USER / IPROX_ASPERA_PASSWORD), or " - "use the default parallel HTTP transport instead." + "iProX Aspera requires credentials: pass --iprox-user and set " + "IPROX_ASPERA_PASSWORD (or answer the password prompt), or use " + "the default parallel HTTP transport instead." ) ascp = cls._ascp_binary() env = dict(os.environ) env["ASPERA_SCP_PASS"] = password os.makedirs(output_folder, exist_ok=True) failed: List[str] = [] - for idx, url in enumerate(urls): - path = urlparse(url).path.lstrip("/") # e.g. IPX.../.../a.raw - source = f"{user}@{cls.ASPERA_HOST}:{cls.ASPERA_ROOT}/{path}" - relpath = relative_paths[idx] if idx < len(relative_paths) else None - dest = os.path.join(output_folder, relpath) if relpath else output_folder - dest_parent = os.path.dirname(dest) or output_folder - os.makedirs(dest_parent, exist_ok=True) - if skip_if_downloaded_already and os.path.exists(dest): - logging.info(f"Skipping download as file already exists: {dest}") - continue - argv = [ - ascp, "-QT", "-P", cls.ASPERA_PORT, "-l", maximum_bandwidth, - "-k", "2", source, dest, - ] - logging.info( - "Aspera: %s -> %s", source.replace(password, "***"), dest - ) - try: - subprocess.run(argv, check=True, env=env) - except subprocess.CalledProcessError as e: - logging.error(f"iProX Aspera failed for {url}: {e}") - failed.append(url) + workers = max(1, min(parallel_files, len(urls))) + if workers > 1: + with ThreadPoolExecutor(max_workers=workers) as executor: + future_to_url = { + executor.submit( + cls._aspera_download_one, + ascp, + url, + relative_paths[idx] if idx < len(relative_paths) else None, + output_folder, + user, + password, + maximum_bandwidth, + skip_if_downloaded_already, + env, + ): url + for idx, url in enumerate(urls) + } + for future in as_completed(future_to_url): + url = future_to_url[future] + try: + result = future.result() + if result is not None: + failed.append(result) + except Exception as e: + logging.error(f"iProX Aspera failed for {url}: {e}") + failed.append(url) + else: + for idx, url in enumerate(urls): + relpath = relative_paths[idx] if idx < len(relative_paths) else None + result = cls._aspera_download_one( + ascp, + url, + relpath, + output_folder, + user, + password, + maximum_bandwidth, + skip_if_downloaded_already, + env, + ) + if result is not None: + failed.append(result) if failed: raise RuntimeError( f"iProX Aspera download failed for {len(failed)} file(s): {failed}" diff --git a/pridepy/download/proteomexchange.py b/pridepy/download/proteomexchange.py index 10e4374..81bf508 100644 --- a/pridepy/download/proteomexchange.py +++ b/pridepy/download/proteomexchange.py @@ -201,13 +201,23 @@ def download_from_accession_or_url( iprox_urls, rels = [], [] for r in records: loc = self.get_download_url(r, protocol) - if "download.iprox.org" in loc: + host = (urlparse(loc).hostname or "").lower() + if host == "download.iprox.org": iprox_urls.append(loc) rels.append(r.get("relativePath")) if not iprox_urls: raise ValueError( "Aspera requested but no iProX-hosted files found in this dataset." ) + if len(iprox_urls) < len(records): + logging.warning( + "%d of %d file(s) are NOT hosted on iProX and were NOT " + "downloaded: --protocol aspera only handles iProX-hosted " + "files. Use the default HTTP transport (omit --protocol, " + "or pass --protocol ftp) to download the full set.", + len(records) - len(iprox_urls), + len(records), + ) if flatten: sources = [ rel if rel else urlparse(url).path @@ -223,6 +233,7 @@ def download_from_accession_or_url( user=iprox_user, password=iprox_password, skip_if_downloaded_already=skip_if_downloaded_already, + parallel_files=parallel_files, ) return diff --git a/pridepy/download/transport.py b/pridepy/download/transport.py index feae6ab..5f6882e 100644 --- a/pridepy/download/transport.py +++ b/pridepy/download/transport.py @@ -19,6 +19,11 @@ from pridepy.util.api_handling import Util +# Combined cap on parallel_files (-w) x download_threads (-t): each factor is +# independently clamped to 32, but nested they can reach 1024 concurrent HTTP +# connections. Clamp the product to keep peak connections reasonable. +MAX_TOTAL_HTTP_CONNECTIONS = 64 + def _safe_join(output_folder: str, relative_path: str) -> str: """Join ``output_folder`` with a dataset-relative path. @@ -845,6 +850,19 @@ def _rel(idx: int) -> Optional[str]: failed: List[str] = [] workers = max(1, min(parallel_files, len(http_urls))) + if workers * download_threads > MAX_TOTAL_HTTP_CONNECTIONS: + clamped_threads = max(1, MAX_TOTAL_HTTP_CONNECTIONS // workers) + logging.warning( + "parallel_files (%d) x download_threads (%d) = %d exceeds the " + "combined connection cap of %d; reducing download_threads to %d " + "to keep peak HTTP connections bounded.", + workers, + download_threads, + workers * download_threads, + MAX_TOTAL_HTTP_CONNECTIONS, + clamped_threads, + ) + download_threads = clamped_threads if workers > 1: logging.info( f"Downloading {len(http_urls)} HTTP(S) file(s) with {workers} parallel workers" diff --git a/pridepy/pridepy.py b/pridepy/pridepy.py index a260946..590e089 100644 --- a/pridepy/pridepy.py +++ b/pridepy/pridepy.py @@ -1,6 +1,7 @@ #!/usr/bin/env python3 import asyncio import logging +import os from typing import Optional import click @@ -395,17 +396,10 @@ def download_file_by_name( envvar="IPROX_USER", default=None, type=str, - help="iProX account username. Only used with --protocol aspera.", -) -@click.option( - "--iprox-password", - "iprox_password", - envvar="IPROX_ASPERA_PASSWORD", - default=None, - type=str, - help="iProX account password. Only used with --protocol aspera. " - "Best supplied via the IPROX_ASPERA_PASSWORD environment variable " - "rather than on the command line.", + help="iProX account username. Only used with --protocol aspera. The " + "password is never accepted as a command-line flag: it is read from " + "the IPROX_ASPERA_PASSWORD environment variable, or prompted for " + "securely (hidden input) if not set.", ) def download_px_raw_files( accession: str, @@ -416,11 +410,15 @@ def download_px_raw_files( parallel_files: int = 1, preserve_structure: bool = False, iprox_user: Optional[str] = None, - iprox_password: Optional[str] = None, ): """CLI wrapper to download raw files via ProteomeXchange XML.""" files = Files() logging.info(f"PX accession/URL: {accession}") + + password = os.environ.get("IPROX_ASPERA_PASSWORD") + if protocol.lower() == "aspera" and not password: + password = click.prompt("iProX Aspera password", hide_input=True) + files.download_px_raw_files( accession, output_folder, @@ -430,7 +428,7 @@ def download_px_raw_files( download_threads=download_threads, parallel_files=parallel_files, iprox_user=iprox_user, - iprox_password=iprox_password, + iprox_password=password, ) diff --git a/pridepy/tests/test_download_resilience.py b/pridepy/tests/test_download_resilience.py index 532f624..c2b8cc7 100644 --- a/pridepy/tests/test_download_resilience.py +++ b/pridepy/tests/test_download_resilience.py @@ -247,6 +247,39 @@ def test_download_http_urls_raises_when_a_file_fails(self): max_retries=1, ) + def test_download_http_urls_clamps_combined_connection_cap(self): + """parallel_files x download_threads must be clamped to + transport.MAX_TOTAL_HTTP_CONNECTIONS, with a warning explaining why, + so e.g. -w 32 -t 32 doesn't open 1024 connections.""" + seen_threads = [] + + def fake_http_download_one(url, output_folder, skip_if_downloaded_already, + max_retries=3, position=0, relative_path=None, + download_threads=1): + seen_threads.append(download_threads) + + with tempfile.TemporaryDirectory() as tmp_dir: + with patch.object( + transport, "_http_download_one", side_effect=fake_http_download_one + ): + with self.assertLogs(level="WARNING") as log_ctx: + transport.download_http_urls( + http_urls=[ + "https://example.org/a.raw", + "https://example.org/b.raw", + "https://example.org/c.raw", + ], + output_folder=tmp_dir, + skip_if_downloaded_already=False, + parallel_files=32, + download_threads=32, + ) + assert any("connection cap" in m.lower() for m in log_ctx.output) + workers = min(32, 3) + expected_threads = max(1, transport.MAX_TOTAL_HTTP_CONNECTIONS // workers) + assert workers * expected_threads <= transport.MAX_TOTAL_HTTP_CONNECTIONS + assert all(t == expected_threads for t in seen_threads) + def test_download_ftp_urls_raises_when_a_file_fails(self): """A failed FTP transfer must surface as an exception.""" with tempfile.TemporaryDirectory() as tmp_dir: diff --git a/pridepy/tests/test_iprox_aspera.py b/pridepy/tests/test_iprox_aspera.py index b4600b9..6fa6bab 100644 --- a/pridepy/tests/test_iprox_aspera.py +++ b/pridepy/tests/test_iprox_aspera.py @@ -83,6 +83,98 @@ def test_skip_if_downloaded_already_skips_existing_file(self): ) run.assert_not_called() + def test_skip_if_downloaded_already_does_not_skip_when_only_dir_exists(self): + """Regression: when relpath is missing, dest used to resolve to the + output DIRECTORY, so os.path.exists(dest) was always True and every + such file was wrongly skipped.""" + url = "http://download.iprox.org/IPX0003578000/a.raw" + with tempfile.TemporaryDirectory() as tmp, \ + patch("pridepy.download.iprox.subprocess.run") as run, \ + patch.object(IproxProvider, "_ascp_binary", return_value="/bin/ascp"): + run.return_value = MagicMock(returncode=0) + IproxProvider.aspera_download( + urls=[url], + output_folder=tmp, + relative_paths=[None], + user="bob", + password="secret", + maximum_bandwidth="100M", + skip_if_downloaded_already=True, + ) + run.assert_called_once() + + def test_skip_if_downloaded_already_does_not_skip_zero_byte_file(self): + url = "http://download.iprox.org/IPX0003578000/IPX0003578001/a.raw" + with tempfile.TemporaryDirectory() as tmp, \ + patch("pridepy.download.iprox.subprocess.run") as run, \ + patch.object(IproxProvider, "_ascp_binary", return_value="/bin/ascp"): + dest_dir = os.path.join(tmp, "IPX0003578001") + os.makedirs(dest_dir, exist_ok=True) + dest_file = os.path.join(dest_dir, "a.raw") + open(dest_file, "w").close() # 0-byte partial file + run.return_value = MagicMock(returncode=0) + + IproxProvider.aspera_download( + urls=[url], + output_folder=tmp, + relative_paths=["IPX0003578001/a.raw"], + user="bob", + password="secret", + maximum_bandwidth="100M", + skip_if_downloaded_already=True, + ) + run.assert_called_once() + + def test_traversal_relpath_does_not_escape_output_folder(self): + """A relativePath like '../../etc/x' must not write outside output_folder.""" + url = "http://download.iprox.org/IPX0003578000/a.raw" + with tempfile.TemporaryDirectory() as tmp, \ + patch("pridepy.download.iprox.subprocess.run") as run, \ + patch.object(IproxProvider, "_ascp_binary", return_value="/bin/ascp"): + run.return_value = MagicMock(returncode=0) + IproxProvider.aspera_download( + urls=[url], + output_folder=tmp, + relative_paths=["../../etc/x"], + user="bob", + password="secret", + maximum_bandwidth="100M", + ) + args, kwargs = run.call_args + argv = args[0] + dest = argv[-1] + out_abs = os.path.abspath(tmp) + dest_abs = os.path.abspath(dest) + assert dest_abs == out_abs or dest_abs.startswith(out_abs + os.sep) + + def test_parallel_files_downloads_all_and_aggregates_failures(self): + urls = [ + "http://download.iprox.org/IPX0003578000/a.raw", + "http://download.iprox.org/IPX0003578000/b.raw", + "http://download.iprox.org/IPX0003578000/c.raw", + ] + rels = ["a.raw", "b.raw", "c.raw"] + + def fake_run(argv, check, env): + if argv[-1].endswith("b.raw"): + raise subprocess.CalledProcessError(1, argv) + return MagicMock(returncode=0) + + with tempfile.TemporaryDirectory() as tmp, \ + patch("pridepy.download.iprox.subprocess.run", side_effect=fake_run) as run, \ + patch.object(IproxProvider, "_ascp_binary", return_value="/bin/ascp"): + with pytest.raises(RuntimeError, match="b.raw"): + IproxProvider.aspera_download( + urls=urls, + output_folder=tmp, + relative_paths=rels, + user="bob", + password="secret", + maximum_bandwidth="100M", + parallel_files=2, + ) + assert run.call_count == 3 + class TestPxAsperaRouting(TestCase): def test_px_aspera_routes_to_iprox(self): @@ -104,6 +196,58 @@ def test_px_aspera_routes_to_iprox(self): asp.assert_called_once() assert asp.call_args.kwargs["user"] == "bob" + def test_px_aspera_rejects_spoofed_host(self): + """A URL on a lookalike host (substring match, not exact) must NOT be + routed to iProX Aspera.""" + from pridepy.download.proteomexchange import ProteomeXchangeProvider + prov = ProteomeXchangeProvider() + rec = { + "publicFileLocations": [ + {"name": "FTP Protocol", + "value": "http://download.iprox.org.evil.example/IPX1/a.raw"} + ], + "relativePath": "a.raw", + } + with patch.object(prov, "list_files", return_value=[rec]), \ + patch("pridepy.download.iprox.IproxProvider.aspera_download") as asp: + with pytest.raises(ValueError, match="no iProX-hosted files"): + prov.download_from_accession_or_url( + "PXD000001", "/tmp/x", protocol="aspera", + iprox_user="bob", iprox_password="secret", + ) + asp.assert_not_called() + + def test_px_aspera_warns_on_mixed_dataset(self): + """Non-iProX files in a mixed dataset are dropped from the aspera + transfer; a warning should be logged naming how many were skipped.""" + from pridepy.download.proteomexchange import ProteomeXchangeProvider + prov = ProteomeXchangeProvider() + records = [ + { + "publicFileLocations": [ + {"name": "FTP Protocol", + "value": "http://download.iprox.org/IPX1/a.raw"} + ], + "relativePath": "a.raw", + }, + { + "publicFileLocations": [ + {"name": "FTP Protocol", + "value": "ftp://massive-ftp.ucsd.edu/MSV1/b.raw"} + ], + "relativePath": "b.raw", + }, + ] + with patch.object(prov, "list_files", return_value=records), \ + patch("pridepy.download.iprox.IproxProvider.aspera_download") as asp, \ + self.assertLogs(level="WARNING") as log_ctx: + prov.download_from_accession_or_url( + "PXD000001", "/tmp/x", protocol="aspera", + iprox_user="bob", iprox_password="secret", + ) + asp.assert_called_once() + assert any("not" in m.lower() and "iprox" in m.lower() for m in log_ctx.output) + def test_px_aspera_flattens_relative_paths_by_default(self): """Aspera branch should honor flatten=True like the HTTP/FTP path: dataset subtree paths collapse to deduplicated basenames.""" From 2eb6b23c2076aec0caf02da39732ac9e92b77421 Mon Sep 17 00:00:00 2001 From: Yasset Perez-Riverol Date: Thu, 2 Jul 2026 08:10:20 +0100 Subject: [PATCH 5/9] fix(download): iProX Aspera uses key auth + --file-list batch (match working ascp), replace password with --aspera-key The shipped implementation authenticated with ASPERA_SCP_PASS and issued one ascp per file against user@host:/data/iprox/, which does not work against iProX. Switch to the verified working invocation: key-based auth (-i ) and a single batched `ascp --mode recv --file-list` session per dataset, with file-list entries being exactly the URL path (no /data/iprox prefix). Password auth is dropped entirely; --iprox-user and the new --aspera-key (env IPROX_ASPERA_KEY) are required for --protocol aspera. Aspera downloads always preserve the iProX directory tree (ascp --file-list recreates the remote path layout), so flatten no longer applies to that path. --- docs/usage.md | 26 ++- pridepy/download/client.py | 4 +- pridepy/download/iprox.py | 151 ++++---------- pridepy/download/proteomexchange.py | 22 +- pridepy/pridepy.py | 23 ++- pridepy/tests/test_iprox_aspera.py | 304 ++++++++++------------------ 6 files changed, 185 insertions(+), 345 deletions(-) diff --git a/docs/usage.md b/docs/usage.md index c1d7eb8..d7b8359 100644 --- a/docs/usage.md +++ b/docs/usage.md @@ -244,11 +244,12 @@ pridepy download-px-raw-files \ | `-t, --threads` | Parallel HTTP Range threads per file (1–32) for fast per-file downloads | `1` | | `--skip-if-downloaded-already` | Skip files already present locally | off | | `--preserve-structure` | Recreate the dataset's subdirectory layout under the output folder | off | -| `--iprox-user` | iProX account username (with `--protocol aspera`; env fallback: `IPROX_USER`) | — | +| `--iprox-user` | Your registered iProX username (required with `--protocol aspera`; env fallback: `IPROX_USER`) | — | +| `--aspera-key` | Path to your Aspera private key (required with `--protocol aspera`; env fallback: `IPROX_ASPERA_KEY`) | — | -The iProX Aspera password is never accepted as a command-line flag. Set the -`IPROX_ASPERA_PASSWORD` environment variable, or omit it and `pridepy` will -prompt for it securely (hidden input) when `--protocol aspera` is used. +iProX Aspera uses key-based authentication only — there is no password +option. `--aspera-key` must point at the private key you registered when +setting up your Aspera client. ### Fast downloads: parallel files and per-file segments @@ -279,20 +280,25 @@ pridepy download-px-raw-files \ ### Fast downloads with iProX Aspera (account required) iProX offers Aspera for very large bulk transfers. Aspera is faster than HTTP -on high-bandwidth connections but requires an iProX account. Combine -`--protocol aspera` with `--iprox-user`; the password is read from -`IPROX_ASPERA_PASSWORD` or prompted for securely (never passed as a flag): +on high-bandwidth connections but requires an iProX account and a registered +Aspera private key (no password auth). Combine `--protocol aspera` with +`--iprox-user` and `--aspera-key`: ```bash -# Download via iProX Aspera with 8-file parallelism -IPROX_ASPERA_PASSWORD=your_password pridepy download-px-raw-files \ +# Download via iProX Aspera +pridepy download-px-raw-files \ -a PXD077178 \ -o ./out \ --protocol aspera \ --iprox-user your_username \ - -w 8 + --aspera-key /path/to/your/aspera_key ``` +Aspera transfers a single batched session per dataset (`ascp --file-list`) +and always preserves the iProX `IPX.../IPX.../` source directory tree under +the output folder — `--preserve-structure` / flattening does not apply to +the Aspera path. + ### Go directly to the hosting repository (native MassIVE / JPOST / iProX accessions) Datasets that do not have a ProteomeXchange accession — or where you already diff --git a/pridepy/download/client.py b/pridepy/download/client.py index 6707810..8fe2fa0 100644 --- a/pridepy/download/client.py +++ b/pridepy/download/client.py @@ -322,7 +322,7 @@ def download_px_raw_files( download_threads: int = 1, protocol: str = "ftp", iprox_user: Optional[str] = None, - iprox_password: Optional[str] = None, + aspera_key: Optional[str] = None, ) -> None: """Delegate to :meth:`ProteomeXchangeProvider.download_from_accession_or_url`.""" return ProteomeXchangeProvider().download_from_accession_or_url( @@ -334,5 +334,5 @@ def download_px_raw_files( download_threads=download_threads, protocol=protocol, iprox_user=iprox_user, - iprox_password=iprox_password, + aspera_key=aspera_key, ) diff --git a/pridepy/download/iprox.py b/pridepy/download/iprox.py index 608fd22..12fdb17 100644 --- a/pridepy/download/iprox.py +++ b/pridepy/download/iprox.py @@ -15,7 +15,7 @@ import os import re import subprocess -from concurrent.futures import ThreadPoolExecutor, as_completed +import tempfile import defusedxml.ElementTree as ET from typing import ClassVar, Dict, List, Optional from urllib.parse import urlparse @@ -25,7 +25,6 @@ from pridepy.download import registry from pridepy.download.base import Provider from pridepy.download.jpost import JpostProvider -from pridepy.download.transport import _safe_join @registry.register @@ -39,7 +38,6 @@ class IproxProvider(Provider): ) ASPERA_HOST: ClassVar[str] = "download.iprox.org" ASPERA_PORT: ClassVar[str] = "33001" - ASPERA_ROOT: ClassVar[str] = "/data/iprox" # iProX PX XML uses the same PSI-MS cvParam "name" values as JPOST PROXI, # so we reuse JpostProvider's category map. PX_CATEGORY_MAP: ClassVar[Dict[str, str]] = JpostProvider.PROXI_CATEGORY_MAP @@ -57,127 +55,60 @@ def _ascp_binary() -> str: from pridepy.download.pride import PrideProvider return PrideProvider.get_ascp_binary() - @classmethod - def _aspera_download_one( - cls, - ascp: str, - url: str, - relpath: Optional[str], - output_folder: str, - user: str, - password: str, - maximum_bandwidth: str, - skip_if_downloaded_already: bool, - env: Dict[str, str], - ) -> Optional[str]: - """Download a single URL via ascp. Returns ``url`` on failure, else None.""" - path = urlparse(url).path.lstrip("/") # e.g. IPX.../.../a.raw - source = f"{user}@{cls.ASPERA_HOST}:{cls.ASPERA_ROOT}/{path}" - if relpath: - dest = _safe_join(output_folder, relpath) - else: - dest = os.path.join(output_folder, os.path.basename(urlparse(url).path)) - dest_parent = os.path.dirname(dest) or output_folder - os.makedirs(dest_parent, exist_ok=True) - if ( - skip_if_downloaded_already - and os.path.isfile(dest) - and os.path.getsize(dest) > 0 - ): - logging.info(f"Skipping download as file already exists: {dest}") - return None - argv = [ - ascp, "-QT", "-P", cls.ASPERA_PORT, "-l", maximum_bandwidth, - "-k", "2", source, dest, - ] - logging.info( - "Aspera: %s -> %s", source.replace(password, "***"), dest - ) - try: - subprocess.run(argv, check=True, env=env) - return None - except subprocess.CalledProcessError as e: - logging.error(f"iProX Aspera failed for {url}: {e}") - return url - @classmethod def aspera_download( cls, urls: List[str], output_folder: str, - relative_paths: List[Optional[str]], user: Optional[str], - password: Optional[str], - maximum_bandwidth: str = "100M", - skip_if_downloaded_already: bool = False, - parallel_files: int = 1, + key_path: Optional[str], + maximum_bandwidth: str = "500M", ) -> None: - """Download iProX-hosted URLs via ascp on port 33001. + """Download iProX-hosted URLs in one batched ``ascp --file-list`` session. - Requires iProX account credentials; the password is passed to the - subprocess through ASPERA_SCP_PASS (never argv). When - ``parallel_files`` > 1, transfers run concurrently: each ``ascp`` - invocation is its own subprocess writing its own destination file, so - this is safe. + Uses key-based auth (``-i ``) against ``download.iprox.org`` + on port 33001; password auth is not supported. ``ascp --mode recv + --file-list`` always recreates the remote ``/IPX.../IPX.../`` source + tree under ``output_folder`` — this transfer path does not support + flattening. """ - if not user or not password: + if not user: raise ValueError( - "iProX Aspera requires credentials: pass --iprox-user and set " - "IPROX_ASPERA_PASSWORD (or answer the password prompt), or use " - "the default parallel HTTP transport instead." + "iProX Aspera requires --iprox-user (your registered iProX " + "username)." + ) + if not key_path or not os.path.isfile(key_path): + raise ValueError( + "iProX Aspera requires --aspera-key (the path to your " + "registered Aspera private key). HTTP is the default " + "alternative if you don't have one." ) ascp = cls._ascp_binary() - env = dict(os.environ) - env["ASPERA_SCP_PASS"] = password + remote_paths = [urlparse(u).path for u in urls] os.makedirs(output_folder, exist_ok=True) - failed: List[str] = [] - workers = max(1, min(parallel_files, len(urls))) - if workers > 1: - with ThreadPoolExecutor(max_workers=workers) as executor: - future_to_url = { - executor.submit( - cls._aspera_download_one, - ascp, - url, - relative_paths[idx] if idx < len(relative_paths) else None, - output_folder, - user, - password, - maximum_bandwidth, - skip_if_downloaded_already, - env, - ): url - for idx, url in enumerate(urls) - } - for future in as_completed(future_to_url): - url = future_to_url[future] - try: - result = future.result() - if result is not None: - failed.append(result) - except Exception as e: - logging.error(f"iProX Aspera failed for {url}: {e}") - failed.append(url) - else: - for idx, url in enumerate(urls): - relpath = relative_paths[idx] if idx < len(relative_paths) else None - result = cls._aspera_download_one( - ascp, - url, - relpath, - output_folder, - user, - password, - maximum_bandwidth, - skip_if_downloaded_already, - env, - ) - if result is not None: - failed.append(result) - if failed: - raise RuntimeError( - f"iProX Aspera download failed for {len(failed)} file(s): {failed}" + fd, list_path = tempfile.mkstemp(prefix="iprox_aspera_", suffix=".txt", text=True) + try: + with os.fdopen(fd, "w") as fh: + for path in remote_paths: + fh.write(path + "\n") + argv = [ + ascp, "-T", "-l", maximum_bandwidth, "-P", cls.ASPERA_PORT, + "-k", "1", "-i", key_path, "--mode", "recv", + "--host", cls.ASPERA_HOST, "--file-list", list_path, + "--user", user, output_folder, + ] + logging.info( + "iProX Aspera: transferring %d file(s) via ascp --file-list", + len(remote_paths), ) + try: + subprocess.run(argv, check=True) + except subprocess.CalledProcessError as e: + raise RuntimeError( + f"iProX Aspera transfer failed (exit {e.returncode})" + ) from e + finally: + os.remove(list_path) @staticmethod def _get_public_root(accession: str) -> str: diff --git a/pridepy/download/proteomexchange.py b/pridepy/download/proteomexchange.py index 81bf508..89cca0f 100644 --- a/pridepy/download/proteomexchange.py +++ b/pridepy/download/proteomexchange.py @@ -30,7 +30,6 @@ from urllib.parse import urlparse from pridepy.download.base import Provider -from pridepy.download.util import flatten_relative_paths from pridepy.util.api_handling import Util @@ -175,7 +174,7 @@ def download_from_accession_or_url( download_threads: int = 1, protocol: str = "ftp", iprox_user: Optional[str] = None, - iprox_password: Optional[str] = None, + aspera_key: Optional[str] = None, ) -> None: """End-to-end: resolve XML, list files, partition by scheme, download. @@ -189,7 +188,8 @@ def download_from_accession_or_url( When ``protocol == "aspera"``, iProX-hosted files are routed through :meth:`IproxProvider.aspera_download` instead of the HTTP/FTP path - (opt-in, requires ``iprox_user``/``iprox_password``). + (opt-in, requires ``iprox_user``/``aspera_key``; key-based auth only, + and the transfer always preserves the source directory tree). """ records = self.list_files(px_id_or_url) if not records: @@ -198,13 +198,12 @@ def download_from_accession_or_url( if protocol.lower() == "aspera": from pridepy.download.iprox import IproxProvider - iprox_urls, rels = [], [] + iprox_urls = [] for r in records: loc = self.get_download_url(r, protocol) host = (urlparse(loc).hostname or "").lower() if host == "download.iprox.org": iprox_urls.append(loc) - rels.append(r.get("relativePath")) if not iprox_urls: raise ValueError( "Aspera requested but no iProX-hosted files found in this dataset." @@ -218,22 +217,11 @@ def download_from_accession_or_url( len(records) - len(iprox_urls), len(records), ) - if flatten: - sources = [ - rel if rel else urlparse(url).path - for url, rel in zip(iprox_urls, rels) - ] - dest_rels: List[Optional[str]] = flatten_relative_paths(sources) - else: - dest_rels = rels IproxProvider.aspera_download( urls=iprox_urls, output_folder=output_folder, - relative_paths=dest_rels, user=iprox_user, - password=iprox_password, - skip_if_downloaded_already=skip_if_downloaded_already, - parallel_files=parallel_files, + key_path=aspera_key, ) return diff --git a/pridepy/pridepy.py b/pridepy/pridepy.py index 590e089..8d61581 100644 --- a/pridepy/pridepy.py +++ b/pridepy/pridepy.py @@ -1,7 +1,6 @@ #!/usr/bin/env python3 import asyncio import logging -import os from typing import Optional import click @@ -396,10 +395,17 @@ def download_file_by_name( envvar="IPROX_USER", default=None, type=str, - help="iProX account username. Only used with --protocol aspera. The " - "password is never accepted as a command-line flag: it is read from " - "the IPROX_ASPERA_PASSWORD environment variable, or prompted for " - "securely (hidden input) if not set.", + help="Your registered iProX username. Required with --protocol aspera.", +) +@click.option( + "--aspera-key", + "aspera_key", + envvar="IPROX_ASPERA_KEY", + default=None, + type=str, + help="Path to your Aspera private key for iProX (required with " + "--protocol aspera; the key you registered when setting up your " + "Aspera client).", ) def download_px_raw_files( accession: str, @@ -410,15 +416,12 @@ def download_px_raw_files( parallel_files: int = 1, preserve_structure: bool = False, iprox_user: Optional[str] = None, + aspera_key: Optional[str] = None, ): """CLI wrapper to download raw files via ProteomeXchange XML.""" files = Files() logging.info(f"PX accession/URL: {accession}") - password = os.environ.get("IPROX_ASPERA_PASSWORD") - if protocol.lower() == "aspera" and not password: - password = click.prompt("iProX Aspera password", hide_input=True) - files.download_px_raw_files( accession, output_folder, @@ -428,7 +431,7 @@ def download_px_raw_files( download_threads=download_threads, parallel_files=parallel_files, iprox_user=iprox_user, - iprox_password=password, + aspera_key=aspera_key, ) diff --git a/pridepy/tests/test_iprox_aspera.py b/pridepy/tests/test_iprox_aspera.py index 6fa6bab..a846450 100644 --- a/pridepy/tests/test_iprox_aspera.py +++ b/pridepy/tests/test_iprox_aspera.py @@ -1,4 +1,4 @@ -"""iProX Aspera command construction + credential handling.""" +"""iProX Aspera command construction + key-auth handling.""" import os import re import subprocess @@ -11,169 +11,129 @@ from pridepy.download.iprox import IproxProvider +def _make_key_file(tmp): + key_path = os.path.join(tmp, "aspera.key") + with open(key_path, "w") as f: + f.write("fake-private-key") + return key_path + + class TestIproxAspera(TestCase): - def test_builds_ascp_source_and_env(self): - url = "http://download.iprox.org/IPX0003578000/IPX0003578001/a.raw" + def test_builds_ascp_key_based_file_list_argv(self): + urls = [ + "http://download.iprox.org/IPX0003474000/IPX0003474001/a.raw", + "http://download.iprox.org/IPX0002031000/IPX0002031001/b.raw", + ] + captured_list_contents = {} + + def fake_run(argv, **kwargs): + list_idx = argv.index("--file-list") + 1 + list_path = argv[list_idx] + with open(list_path) as f: + captured_list_contents["lines"] = f.read().splitlines() + return MagicMock(returncode=0) + with tempfile.TemporaryDirectory() as tmp, \ - patch("pridepy.download.iprox.subprocess.run") as run, \ + patch("pridepy.download.iprox.subprocess.run", side_effect=fake_run) as run, \ patch.object(IproxProvider, "_ascp_binary", return_value="/bin/ascp"): - run.return_value = MagicMock(returncode=0) + key_path = _make_key_file(tmp) + output_folder = os.path.join(tmp, "out") IproxProvider.aspera_download( - urls=[url], - output_folder=tmp, - relative_paths=["IPX0003578001/a.raw"], - user="bob", - password="secret", - maximum_bandwidth="100M", + urls=urls, + output_folder=output_folder, + user="daicx", + key_path=key_path, + maximum_bandwidth="500M", ) + args, kwargs = run.call_args argv = args[0] - assert argv[0] == "/bin/ascp" - assert "33001" in argv - assert "bob@download.iprox.org:/data/iprox/IPX0003578000/IPX0003578001/a.raw" in argv - # password only via env, never argv - assert "secret" not in argv - assert kwargs["env"]["ASPERA_SCP_PASS"] == "secret" - - def test_missing_credentials_raises(self): - with pytest.raises(ValueError, match="credentials"): - IproxProvider.aspera_download( - urls=["http://download.iprox.org/IPX1/a.raw"], - output_folder="/tmp/x", - relative_paths=["a.raw"], - user=None, - password=None, - ) - def test_failed_transfer_raises_runtime_error(self): - url = "http://download.iprox.org/IPX0003578000/IPX0003578001/a.raw" - with tempfile.TemporaryDirectory() as tmp, \ - patch("pridepy.download.iprox.subprocess.run") as run, \ - patch.object(IproxProvider, "_ascp_binary", return_value="/bin/ascp"): - run.side_effect = subprocess.CalledProcessError(1, ["ascp"]) - with pytest.raises(RuntimeError, match=re.escape(url)): - IproxProvider.aspera_download( - urls=[url], - output_folder=tmp, - relative_paths=["IPX0003578001/a.raw"], - user="bob", - password="secret", - maximum_bandwidth="100M", - ) + assert argv[0] == "/bin/ascp" + assert "-T" in argv + assert argv[argv.index("-l") + 1] == "500M" + assert argv[argv.index("-P") + 1] == "33001" + assert argv[argv.index("-k") + 1] == "1" + assert argv[argv.index("-i") + 1] == key_path + assert argv[argv.index("--mode") + 1] == "recv" + assert argv[argv.index("--host") + 1] == "download.iprox.org" + assert argv[argv.index("--user") + 1] == "daicx" + assert argv[-1] == output_folder + + # no password/env credential handling at all + assert "env" not in kwargs + assert "ASPERA_SCP_PASS" not in argv + assert not any("secret" in str(a) for a in argv) + + # file-list contains exactly the URL paths, one per line + assert captured_list_contents["lines"] == [ + "/IPX0003474000/IPX0003474001/a.raw", + "/IPX0002031000/IPX0002031001/b.raw", + ] - def test_skip_if_downloaded_already_skips_existing_file(self): + def test_temp_file_list_is_removed_after_run(self): url = "http://download.iprox.org/IPX0003578000/IPX0003578001/a.raw" - with tempfile.TemporaryDirectory() as tmp, \ - patch("pridepy.download.iprox.subprocess.run") as run, \ - patch.object(IproxProvider, "_ascp_binary", return_value="/bin/ascp"): - dest_dir = os.path.join(tmp, "IPX0003578001") - os.makedirs(dest_dir, exist_ok=True) - dest_file = os.path.join(dest_dir, "a.raw") - with open(dest_file, "w") as f: - f.write("already here") + captured = {} - IproxProvider.aspera_download( - urls=[url], - output_folder=tmp, - relative_paths=["IPX0003578001/a.raw"], - user="bob", - password="secret", - maximum_bandwidth="100M", - skip_if_downloaded_already=True, - ) - run.assert_not_called() + def fake_run(argv, **kwargs): + captured["list_path"] = argv[argv.index("--file-list") + 1] + return MagicMock(returncode=0) - def test_skip_if_downloaded_already_does_not_skip_when_only_dir_exists(self): - """Regression: when relpath is missing, dest used to resolve to the - output DIRECTORY, so os.path.exists(dest) was always True and every - such file was wrongly skipped.""" - url = "http://download.iprox.org/IPX0003578000/a.raw" with tempfile.TemporaryDirectory() as tmp, \ - patch("pridepy.download.iprox.subprocess.run") as run, \ + patch("pridepy.download.iprox.subprocess.run", side_effect=fake_run), \ patch.object(IproxProvider, "_ascp_binary", return_value="/bin/ascp"): - run.return_value = MagicMock(returncode=0) + key_path = _make_key_file(tmp) IproxProvider.aspera_download( urls=[url], - output_folder=tmp, - relative_paths=[None], - user="bob", - password="secret", - maximum_bandwidth="100M", - skip_if_downloaded_already=True, + output_folder=os.path.join(tmp, "out"), + user="daicx", + key_path=key_path, ) - run.assert_called_once() + assert not os.path.exists(captured["list_path"]) - def test_skip_if_downloaded_already_does_not_skip_zero_byte_file(self): - url = "http://download.iprox.org/IPX0003578000/IPX0003578001/a.raw" - with tempfile.TemporaryDirectory() as tmp, \ - patch("pridepy.download.iprox.subprocess.run") as run, \ - patch.object(IproxProvider, "_ascp_binary", return_value="/bin/ascp"): - dest_dir = os.path.join(tmp, "IPX0003578001") - os.makedirs(dest_dir, exist_ok=True) - dest_file = os.path.join(dest_dir, "a.raw") - open(dest_file, "w").close() # 0-byte partial file - run.return_value = MagicMock(returncode=0) + def test_missing_user_raises(self): + with tempfile.TemporaryDirectory() as tmp: + key_path = _make_key_file(tmp) + with pytest.raises(ValueError, match="--iprox-user"): + IproxProvider.aspera_download( + urls=["http://download.iprox.org/IPX1/a.raw"], + output_folder=os.path.join(tmp, "out"), + user=None, + key_path=key_path, + ) + def test_missing_key_raises(self): + with pytest.raises(ValueError, match="--aspera-key"): IproxProvider.aspera_download( - urls=[url], - output_folder=tmp, - relative_paths=["IPX0003578001/a.raw"], - user="bob", - password="secret", - maximum_bandwidth="100M", - skip_if_downloaded_already=True, + urls=["http://download.iprox.org/IPX1/a.raw"], + output_folder="/tmp/x", + user="daicx", + key_path=None, ) - run.assert_called_once() - def test_traversal_relpath_does_not_escape_output_folder(self): - """A relativePath like '../../etc/x' must not write outside output_folder.""" - url = "http://download.iprox.org/IPX0003578000/a.raw" - with tempfile.TemporaryDirectory() as tmp, \ - patch("pridepy.download.iprox.subprocess.run") as run, \ - patch.object(IproxProvider, "_ascp_binary", return_value="/bin/ascp"): - run.return_value = MagicMock(returncode=0) + def test_nonexistent_key_path_raises(self): + with pytest.raises(ValueError, match="--aspera-key"): IproxProvider.aspera_download( - urls=[url], - output_folder=tmp, - relative_paths=["../../etc/x"], - user="bob", - password="secret", - maximum_bandwidth="100M", + urls=["http://download.iprox.org/IPX1/a.raw"], + output_folder="/tmp/x", + user="daicx", + key_path="/no/such/key/file", ) - args, kwargs = run.call_args - argv = args[0] - dest = argv[-1] - out_abs = os.path.abspath(tmp) - dest_abs = os.path.abspath(dest) - assert dest_abs == out_abs or dest_abs.startswith(out_abs + os.sep) - - def test_parallel_files_downloads_all_and_aggregates_failures(self): - urls = [ - "http://download.iprox.org/IPX0003578000/a.raw", - "http://download.iprox.org/IPX0003578000/b.raw", - "http://download.iprox.org/IPX0003578000/c.raw", - ] - rels = ["a.raw", "b.raw", "c.raw"] - - def fake_run(argv, check, env): - if argv[-1].endswith("b.raw"): - raise subprocess.CalledProcessError(1, argv) - return MagicMock(returncode=0) + def test_failed_transfer_raises_runtime_error(self): + url = "http://download.iprox.org/IPX0003578000/IPX0003578001/a.raw" with tempfile.TemporaryDirectory() as tmp, \ - patch("pridepy.download.iprox.subprocess.run", side_effect=fake_run) as run, \ + patch("pridepy.download.iprox.subprocess.run") as run, \ patch.object(IproxProvider, "_ascp_binary", return_value="/bin/ascp"): - with pytest.raises(RuntimeError, match="b.raw"): + key_path = _make_key_file(tmp) + run.side_effect = subprocess.CalledProcessError(1, ["ascp"]) + with pytest.raises(RuntimeError, match=re.escape("exit 1")): IproxProvider.aspera_download( - urls=urls, - output_folder=tmp, - relative_paths=rels, - user="bob", - password="secret", - maximum_bandwidth="100M", - parallel_files=2, + urls=[url], + output_folder=os.path.join(tmp, "out"), + user="daicx", + key_path=key_path, ) - assert run.call_count == 3 class TestPxAsperaRouting(TestCase): @@ -187,14 +147,17 @@ def test_px_aspera_routes_to_iprox(self): ], "relativePath": "IPX2/a.raw", } - with patch.object(prov, "list_files", return_value=[rec]), \ - patch("pridepy.download.iprox.IproxProvider.aspera_download") as asp: - prov.download_from_accession_or_url( - "PXD000001", "/tmp/x", protocol="aspera", - iprox_user="bob", iprox_password="secret", - ) - asp.assert_called_once() - assert asp.call_args.kwargs["user"] == "bob" + with tempfile.TemporaryDirectory() as tmp: + key_path = _make_key_file(tmp) + with patch.object(prov, "list_files", return_value=[rec]), \ + patch("pridepy.download.iprox.IproxProvider.aspera_download") as asp: + prov.download_from_accession_or_url( + "PXD000001", "/tmp/x", protocol="aspera", + iprox_user="daicx", aspera_key=key_path, + ) + asp.assert_called_once() + assert asp.call_args.kwargs["user"] == "daicx" + assert asp.call_args.kwargs["key_path"] == key_path def test_px_aspera_rejects_spoofed_host(self): """A URL on a lookalike host (substring match, not exact) must NOT be @@ -213,7 +176,7 @@ def test_px_aspera_rejects_spoofed_host(self): with pytest.raises(ValueError, match="no iProX-hosted files"): prov.download_from_accession_or_url( "PXD000001", "/tmp/x", protocol="aspera", - iprox_user="bob", iprox_password="secret", + iprox_user="daicx", aspera_key="/some/key", ) asp.assert_not_called() @@ -243,58 +206,7 @@ def test_px_aspera_warns_on_mixed_dataset(self): self.assertLogs(level="WARNING") as log_ctx: prov.download_from_accession_or_url( "PXD000001", "/tmp/x", protocol="aspera", - iprox_user="bob", iprox_password="secret", + iprox_user="daicx", aspera_key="/some/key", ) asp.assert_called_once() assert any("not" in m.lower() and "iprox" in m.lower() for m in log_ctx.output) - - def test_px_aspera_flattens_relative_paths_by_default(self): - """Aspera branch should honor flatten=True like the HTTP/FTP path: - dataset subtree paths collapse to deduplicated basenames.""" - from pridepy.download.proteomexchange import ProteomeXchangeProvider - prov = ProteomeXchangeProvider() - records = [ - { - "publicFileLocations": [ - {"name": "FTP Protocol", - "value": "http://download.iprox.org/IPX1/run1/a.raw"} - ], - "relativePath": "run1/a.raw", - }, - { - "publicFileLocations": [ - {"name": "FTP Protocol", - "value": "http://download.iprox.org/IPX1/run2/a.raw"} - ], - "relativePath": "run2/a.raw", - }, - ] - with patch.object(prov, "list_files", return_value=records), \ - patch("pridepy.download.iprox.IproxProvider.aspera_download") as asp: - prov.download_from_accession_or_url( - "PXD000001", "/tmp/x", protocol="aspera", flatten=True, - iprox_user="bob", iprox_password="secret", - ) - asp.assert_called_once() - rels = asp.call_args.kwargs["relative_paths"] - # Flattened + de-duped basenames, no subdirectories preserved. - assert set(rels) == {"a.raw", "a_1.raw"} - - def test_px_aspera_preserves_structure_when_not_flattened(self): - from pridepy.download.proteomexchange import ProteomeXchangeProvider - prov = ProteomeXchangeProvider() - rec = { - "publicFileLocations": [ - {"name": "FTP Protocol", - "value": "http://download.iprox.org/IPX1/IPX2/a.raw"} - ], - "relativePath": "IPX2/a.raw", - } - with patch.object(prov, "list_files", return_value=[rec]), \ - patch("pridepy.download.iprox.IproxProvider.aspera_download") as asp: - prov.download_from_accession_or_url( - "PXD000001", "/tmp/x", protocol="aspera", flatten=False, - iprox_user="bob", iprox_password="secret", - ) - asp.assert_called_once() - assert asp.call_args.kwargs["relative_paths"] == ["IPX2/a.raw"] From c1c80ff7f6fbc485b4eb5dd79e9e3735fd68178f Mon Sep 17 00:00:00 2001 From: Yasset Perez-Riverol Date: Thu, 2 Jul 2026 08:13:20 +0100 Subject: [PATCH 6/9] feat(download): default iProX Aspera to bundled public key (--aspera-key now optional; zero-config with just --iprox-user) --- pridepy/download/iprox.py | 24 ++++++++++++++++++++---- pridepy/pridepy.py | 7 ++++--- pridepy/tests/test_iprox_aspera.py | 25 +++++++++++++++++-------- 3 files changed, 41 insertions(+), 15 deletions(-) diff --git a/pridepy/download/iprox.py b/pridepy/download/iprox.py index 12fdb17..dbf8ef4 100644 --- a/pridepy/download/iprox.py +++ b/pridepy/download/iprox.py @@ -55,6 +55,20 @@ def _ascp_binary() -> str: from pridepy.download.pride import PrideProvider return PrideProvider.get_ascp_binary() + @staticmethod + def _default_aspera_key() -> str: + """Path to the public Aspera key bundled with pridepy. + + iProX accepts the standard public Aspera key, so Aspera works with + no key setup: users only supply ``--iprox-user``. ``--aspera-key`` + overrides this when a site requires a different registered key. + """ + import importlib.resources + key = importlib.resources.files("pridepy").joinpath( + "aspera/key/asperaweb_id_dsa.openssh" + ) + return os.path.abspath(key) + @classmethod def aspera_download( cls, @@ -77,11 +91,13 @@ def aspera_download( "iProX Aspera requires --iprox-user (your registered iProX " "username)." ) - if not key_path or not os.path.isfile(key_path): + if not key_path: + key_path = cls._default_aspera_key() + if not os.path.isfile(key_path): raise ValueError( - "iProX Aspera requires --aspera-key (the path to your " - "registered Aspera private key). HTTP is the default " - "alternative if you don't have one." + f"iProX Aspera key not found: {key_path}. Pass --aspera-key " + " to your registered Aspera private key, or use the " + "default HTTP transport." ) ascp = cls._ascp_binary() remote_paths = [urlparse(u).path for u in urls] diff --git a/pridepy/pridepy.py b/pridepy/pridepy.py index 8d61581..426e78c 100644 --- a/pridepy/pridepy.py +++ b/pridepy/pridepy.py @@ -403,9 +403,10 @@ def download_file_by_name( envvar="IPROX_ASPERA_KEY", default=None, type=str, - help="Path to your Aspera private key for iProX (required with " - "--protocol aspera; the key you registered when setting up your " - "Aspera client).", + help="Path to an Aspera private key for iProX (optional; only with " + "--protocol aspera). Defaults to the public Aspera key bundled with " + "pridepy, so normally you only need --iprox-user. Override this if your " + "site requires a specific registered key.", ) def download_px_raw_files( accession: str, diff --git a/pridepy/tests/test_iprox_aspera.py b/pridepy/tests/test_iprox_aspera.py index a846450..1c328af 100644 --- a/pridepy/tests/test_iprox_aspera.py +++ b/pridepy/tests/test_iprox_aspera.py @@ -102,14 +102,23 @@ def test_missing_user_raises(self): key_path=key_path, ) - def test_missing_key_raises(self): - with pytest.raises(ValueError, match="--aspera-key"): - IproxProvider.aspera_download( - urls=["http://download.iprox.org/IPX1/a.raw"], - output_folder="/tmp/x", - user="daicx", - key_path=None, - ) + def test_missing_key_defaults_to_bundled_key(self): + """key_path=None falls back to pridepy's bundled Aspera key, so + Aspera needs no key setup — only --iprox-user.""" + with tempfile.TemporaryDirectory() as tmp: + with patch("pridepy.download.iprox.subprocess.run") as run, \ + patch.object(IproxProvider, "_ascp_binary", return_value="/bin/ascp"): + run.return_value = MagicMock(returncode=0) + IproxProvider.aspera_download( + urls=["http://download.iprox.org/IPX1/a.raw"], + output_folder=tmp, + user="daicx", + key_path=None, + ) + argv = run.call_args.args[0] + key_used = argv[argv.index("-i") + 1] + assert key_used.endswith("aspera/key/asperaweb_id_dsa.openssh") + assert os.path.isfile(key_used) def test_nonexistent_key_path_raises(self): with pytest.raises(ValueError, match="--aspera-key"): From 23145627b61695b10cb642073255b3edd5553afa Mon Sep 17 00:00:00 2001 From: Yasset Perez-Riverol Date: Thu, 2 Jul 2026 09:23:17 +0100 Subject: [PATCH 7/9] fix(download): iProX Aspera password auth via env/prompt on the working --file-list form; key optional; no interactive hang --- docs/usage.md | 37 +++++++----- pridepy/download/client.py | 2 + pridepy/download/iprox.py | 51 ++++++++--------- pridepy/download/proteomexchange.py | 7 ++- pridepy/pridepy.py | 14 +++-- pridepy/tests/test_iprox_aspera.py | 87 +++++++++++++++++++++++------ 6 files changed, 136 insertions(+), 62 deletions(-) diff --git a/docs/usage.md b/docs/usage.md index d7b8359..a2be905 100644 --- a/docs/usage.md +++ b/docs/usage.md @@ -245,11 +245,15 @@ pridepy download-px-raw-files \ | `--skip-if-downloaded-already` | Skip files already present locally | off | | `--preserve-structure` | Recreate the dataset's subdirectory layout under the output folder | off | | `--iprox-user` | Your registered iProX username (required with `--protocol aspera`; env fallback: `IPROX_USER`) | — | -| `--aspera-key` | Path to your Aspera private key (required with `--protocol aspera`; env fallback: `IPROX_ASPERA_KEY`) | — | +| `--aspera-key` | Path to an Aspera private key for iProX (optional with `--protocol aspera`; env fallback: `IPROX_ASPERA_KEY`) | — | -iProX Aspera uses key-based authentication only — there is no password -option. `--aspera-key` must point at the private key you registered when -setting up your Aspera client. +iProX Aspera defaults to password authentication (your iProX account +password), supplied via the `IPROX_ASPERA_PASSWORD` env var or an +interactive hidden prompt. Pass `--aspera-key` instead if you have a +registered Aspera private key. Exactly one credential is required — +`pridepy` fails fast rather than letting `ascp` block on an interactive +prompt, so batch/sbatch jobs must set `IPROX_ASPERA_PASSWORD` (or +`--aspera-key`) up front. ### Fast downloads: parallel files and per-file segments @@ -280,20 +284,25 @@ pridepy download-px-raw-files \ ### Fast downloads with iProX Aspera (account required) iProX offers Aspera for very large bulk transfers. Aspera is faster than HTTP -on high-bandwidth connections but requires an iProX account and a registered -Aspera private key (no password auth). Combine `--protocol aspera` with -`--iprox-user` and `--aspera-key`: +on high-bandwidth connections but requires an iProX account. Combine +`--protocol aspera` with `--iprox-user` and a credential — either the +`IPROX_ASPERA_PASSWORD` env var (password auth, the default) or +`--aspera-key` (a registered Aspera private key): ```bash -# Download via iProX Aspera -pridepy download-px-raw-files \ - -a PXD077178 \ - -o ./out \ - --protocol aspera \ - --iprox-user your_username \ - --aspera-key /path/to/your/aspera_key +# password auth (env var — safe for sbatch; no prompt/hang) +IPROX_ASPERA_PASSWORD=... pridepy download-px-raw-files -a PXD077178 -o ./out --protocol aspera --iprox-user + +# or with a registered key +pridepy download-px-raw-files -a PXD077178 -o ./out --protocol aspera --iprox-user --aspera-key /path/to/key ``` +If run interactively without `IPROX_ASPERA_PASSWORD` or `--aspera-key`, +`pridepy` prompts for the password (hidden input). In non-interactive/batch +contexts (e.g. `sbatch`) you must set `IPROX_ASPERA_PASSWORD` or pass +`--aspera-key` up front — otherwise the command errors immediately instead +of hanging on a prompt. + Aspera transfers a single batched session per dataset (`ascp --file-list`) and always preserves the iProX `IPX.../IPX.../` source directory tree under the output folder — `--preserve-structure` / flattening does not apply to diff --git a/pridepy/download/client.py b/pridepy/download/client.py index 8fe2fa0..144ad30 100644 --- a/pridepy/download/client.py +++ b/pridepy/download/client.py @@ -323,6 +323,7 @@ def download_px_raw_files( protocol: str = "ftp", iprox_user: Optional[str] = None, aspera_key: Optional[str] = None, + aspera_password: Optional[str] = None, ) -> None: """Delegate to :meth:`ProteomeXchangeProvider.download_from_accession_or_url`.""" return ProteomeXchangeProvider().download_from_accession_or_url( @@ -335,4 +336,5 @@ def download_px_raw_files( protocol=protocol, iprox_user=iprox_user, aspera_key=aspera_key, + aspera_password=aspera_password, ) diff --git a/pridepy/download/iprox.py b/pridepy/download/iprox.py index dbf8ef4..322c8e5 100644 --- a/pridepy/download/iprox.py +++ b/pridepy/download/iprox.py @@ -55,33 +55,24 @@ def _ascp_binary() -> str: from pridepy.download.pride import PrideProvider return PrideProvider.get_ascp_binary() - @staticmethod - def _default_aspera_key() -> str: - """Path to the public Aspera key bundled with pridepy. - - iProX accepts the standard public Aspera key, so Aspera works with - no key setup: users only supply ``--iprox-user``. ``--aspera-key`` - overrides this when a site requires a different registered key. - """ - import importlib.resources - key = importlib.resources.files("pridepy").joinpath( - "aspera/key/asperaweb_id_dsa.openssh" - ) - return os.path.abspath(key) - @classmethod def aspera_download( cls, urls: List[str], output_folder: str, user: Optional[str], - key_path: Optional[str], + key_path: Optional[str] = None, + password: Optional[str] = None, maximum_bandwidth: str = "500M", ) -> None: """Download iProX-hosted URLs in one batched ``ascp --file-list`` session. - Uses key-based auth (``-i ``) against ``download.iprox.org`` - on port 33001; password auth is not supported. ``ascp --mode recv + Auth resolution is fail-fast and never lets ``ascp`` fall back to an + interactive ``Password:`` prompt (which would hang a batch/sbatch + job): pass ``key_path`` for key-based auth (``-i ``), or + ``password`` for password auth (via the ``ASPERA_SCP_PASS`` + env var — iProX's own account password, not a key). Exactly one of + the two must be supplied by the caller. ``ascp --mode recv --file-list`` always recreates the remote ``/IPX.../IPX.../`` source tree under ``output_folder`` — this transfer path does not support flattening. @@ -91,13 +82,21 @@ def aspera_download( "iProX Aspera requires --iprox-user (your registered iProX " "username)." ) - if not key_path: - key_path = cls._default_aspera_key() - if not os.path.isfile(key_path): + env = dict(os.environ) + if key_path: + if not os.path.isfile(key_path): + raise ValueError( + f"iProX Aspera key not found: {key_path}. Pass " + "--aspera-key to your registered Aspera private " + "key, or use the default HTTP transport." + ) + elif password: + env["ASPERA_SCP_PASS"] = password + else: raise ValueError( - f"iProX Aspera key not found: {key_path}. Pass --aspera-key " - " to your registered Aspera private key, or use the " - "default HTTP transport." + "iProX Aspera needs a credential: set IPROX_ASPERA_PASSWORD " + "(your iProX account password) or pass --aspera-key . " + "HTTP is the default alternative." ) ascp = cls._ascp_binary() remote_paths = [urlparse(u).path for u in urls] @@ -109,7 +108,9 @@ def aspera_download( fh.write(path + "\n") argv = [ ascp, "-T", "-l", maximum_bandwidth, "-P", cls.ASPERA_PORT, - "-k", "1", "-i", key_path, "--mode", "recv", + "-k", "1", + ] + (["-i", key_path] if key_path else []) + [ + "--mode", "recv", "--host", cls.ASPERA_HOST, "--file-list", list_path, "--user", user, output_folder, ] @@ -118,7 +119,7 @@ def aspera_download( len(remote_paths), ) try: - subprocess.run(argv, check=True) + subprocess.run(argv, check=True, env=env, stdin=subprocess.DEVNULL) except subprocess.CalledProcessError as e: raise RuntimeError( f"iProX Aspera transfer failed (exit {e.returncode})" diff --git a/pridepy/download/proteomexchange.py b/pridepy/download/proteomexchange.py index 89cca0f..9b82703 100644 --- a/pridepy/download/proteomexchange.py +++ b/pridepy/download/proteomexchange.py @@ -175,6 +175,7 @@ def download_from_accession_or_url( protocol: str = "ftp", iprox_user: Optional[str] = None, aspera_key: Optional[str] = None, + aspera_password: Optional[str] = None, ) -> None: """End-to-end: resolve XML, list files, partition by scheme, download. @@ -188,8 +189,9 @@ def download_from_accession_or_url( When ``protocol == "aspera"``, iProX-hosted files are routed through :meth:`IproxProvider.aspera_download` instead of the HTTP/FTP path - (opt-in, requires ``iprox_user``/``aspera_key``; key-based auth only, - and the transfer always preserves the source directory tree). + (opt-in, requires ``iprox_user`` plus either ``aspera_key`` or + ``aspera_password``; the transfer always preserves the source + directory tree). """ records = self.list_files(px_id_or_url) if not records: @@ -222,6 +224,7 @@ def download_from_accession_or_url( output_folder=output_folder, user=iprox_user, key_path=aspera_key, + password=aspera_password, ) return diff --git a/pridepy/pridepy.py b/pridepy/pridepy.py index 426e78c..b737459 100644 --- a/pridepy/pridepy.py +++ b/pridepy/pridepy.py @@ -1,6 +1,8 @@ #!/usr/bin/env python3 import asyncio import logging +import os +import sys from typing import Optional import click @@ -403,10 +405,9 @@ def download_file_by_name( envvar="IPROX_ASPERA_KEY", default=None, type=str, - help="Path to an Aspera private key for iProX (optional; only with " - "--protocol aspera). Defaults to the public Aspera key bundled with " - "pridepy, so normally you only need --iprox-user. Override this if your " - "site requires a specific registered key.", + help="Optional Aspera private key for iProX (only with --protocol " + "aspera). If omitted, password auth is used via the " + "IPROX_ASPERA_PASSWORD env var or a secure prompt.", ) def download_px_raw_files( accession: str, @@ -423,6 +424,10 @@ def download_px_raw_files( files = Files() logging.info(f"PX accession/URL: {accession}") + aspera_password = os.environ.get("IPROX_ASPERA_PASSWORD") + if protocol.lower() == "aspera" and not aspera_key and not aspera_password and sys.stdin.isatty(): + aspera_password = click.prompt("iProX Aspera password", hide_input=True) + files.download_px_raw_files( accession, output_folder, @@ -433,6 +438,7 @@ def download_px_raw_files( parallel_files=parallel_files, iprox_user=iprox_user, aspera_key=aspera_key, + aspera_password=aspera_password, ) diff --git a/pridepy/tests/test_iprox_aspera.py b/pridepy/tests/test_iprox_aspera.py index 1c328af..9df029c 100644 --- a/pridepy/tests/test_iprox_aspera.py +++ b/pridepy/tests/test_iprox_aspera.py @@ -1,4 +1,4 @@ -"""iProX Aspera command construction + key-auth handling.""" +"""iProX Aspera command construction + credential handling.""" import os import re import subprocess @@ -60,17 +60,54 @@ def fake_run(argv, **kwargs): assert argv[argv.index("--user") + 1] == "daicx" assert argv[-1] == output_folder - # no password/env credential handling at all - assert "env" not in kwargs + # key-based auth: no ASPERA_SCP_PASS set in the subprocess env + assert "ASPERA_SCP_PASS" not in kwargs.get("env", {}) assert "ASPERA_SCP_PASS" not in argv assert not any("secret" in str(a) for a in argv) + # stdin is always closed so a rejected/missing credential errors + # instead of blocking on an interactive Password: prompt + assert kwargs.get("stdin") == subprocess.DEVNULL + # file-list contains exactly the URL paths, one per line assert captured_list_contents["lines"] == [ "/IPX0003474000/IPX0003474001/a.raw", "/IPX0002031000/IPX0002031001/b.raw", ] + def test_builds_ascp_password_based_file_list_argv(self): + urls = ["http://download.iprox.org/IPX0003474000/IPX0003474001/a.raw"] + + with tempfile.TemporaryDirectory() as tmp, \ + patch("pridepy.download.iprox.subprocess.run") as run, \ + patch.object(IproxProvider, "_ascp_binary", return_value="/bin/ascp"): + run.return_value = MagicMock(returncode=0) + output_folder = os.path.join(tmp, "out") + IproxProvider.aspera_download( + urls=urls, + output_folder=output_folder, + user="daicx", + password="secret", + maximum_bandwidth="500M", + ) + + args, kwargs = run.call_args + argv = args[0] + + # password auth: no -i flag at all + assert "-i" not in argv + assert argv[argv.index("--mode") + 1] == "recv" + assert argv[argv.index("--host") + 1] == "download.iprox.org" + assert argv[argv.index("--user") + 1] == "daicx" + assert argv[-1] == output_folder + + # credential goes through the env, never on argv + assert kwargs.get("env", {}).get("ASPERA_SCP_PASS") == "secret" + assert not any("secret" in str(a) for a in argv) + + # stdin closed so ascp can't fall back to an interactive prompt + assert kwargs.get("stdin") == subprocess.DEVNULL + def test_temp_file_list_is_removed_after_run(self): url = "http://download.iprox.org/IPX0003578000/IPX0003578001/a.raw" captured = {} @@ -102,23 +139,17 @@ def test_missing_user_raises(self): key_path=key_path, ) - def test_missing_key_defaults_to_bundled_key(self): - """key_path=None falls back to pridepy's bundled Aspera key, so - Aspera needs no key setup — only --iprox-user.""" - with tempfile.TemporaryDirectory() as tmp: - with patch("pridepy.download.iprox.subprocess.run") as run, \ - patch.object(IproxProvider, "_ascp_binary", return_value="/bin/ascp"): - run.return_value = MagicMock(returncode=0) + def test_missing_credential_raises(self): + """Neither key nor password supplied: fail fast, never call ascp.""" + with tempfile.TemporaryDirectory() as tmp, \ + patch("pridepy.download.iprox.subprocess.run") as run: + with pytest.raises(ValueError, match="IPROX_ASPERA_PASSWORD"): IproxProvider.aspera_download( urls=["http://download.iprox.org/IPX1/a.raw"], - output_folder=tmp, + output_folder=os.path.join(tmp, "out"), user="daicx", - key_path=None, ) - argv = run.call_args.args[0] - key_used = argv[argv.index("-i") + 1] - assert key_used.endswith("aspera/key/asperaweb_id_dsa.openssh") - assert os.path.isfile(key_used) + run.assert_not_called() def test_nonexistent_key_path_raises(self): with pytest.raises(ValueError, match="--aspera-key"): @@ -146,7 +177,7 @@ def test_failed_transfer_raises_runtime_error(self): class TestPxAsperaRouting(TestCase): - def test_px_aspera_routes_to_iprox(self): + def test_px_aspera_routes_to_iprox_with_key(self): from pridepy.download.proteomexchange import ProteomeXchangeProvider prov = ProteomeXchangeProvider() rec = { @@ -167,6 +198,28 @@ def test_px_aspera_routes_to_iprox(self): asp.assert_called_once() assert asp.call_args.kwargs["user"] == "daicx" assert asp.call_args.kwargs["key_path"] == key_path + assert asp.call_args.kwargs["password"] is None + + def test_px_aspera_routes_to_iprox_with_password(self): + from pridepy.download.proteomexchange import ProteomeXchangeProvider + prov = ProteomeXchangeProvider() + rec = { + "publicFileLocations": [ + {"name": "FTP Protocol", + "value": "http://download.iprox.org/IPX1/IPX2/a.raw"} + ], + "relativePath": "IPX2/a.raw", + } + with patch.object(prov, "list_files", return_value=[rec]), \ + patch("pridepy.download.iprox.IproxProvider.aspera_download") as asp: + prov.download_from_accession_or_url( + "PXD000001", "/tmp/x", protocol="aspera", + iprox_user="daicx", aspera_password="secret", + ) + asp.assert_called_once() + assert asp.call_args.kwargs["user"] == "daicx" + assert asp.call_args.kwargs["key_path"] is None + assert asp.call_args.kwargs["password"] == "secret" def test_px_aspera_rejects_spoofed_host(self): """A URL on a lookalike host (substring match, not exact) must NOT be From 539f02ebf77fcc5ebf09979907f30c355f89f9d9 Mon Sep 17 00:00:00 2001 From: Yasset Perez-Riverol Date: Wed, 29 Jul 2026 16:20:39 +0100 Subject: [PATCH 8/9] feat(download): add EBI-internal FIRE S3 protocol (--protocol fire) FTP/HTTPS/Globus fetches fail for some datasets. Add a `fire` protocol that downloads over the EBI-internal FIRE S3 endpoint (hl.fire.sdo.ebi.ac.uk), which is reachable only inside EBI infrastructure but is far more reliable there. It reuses the existing S3 mechanism (unsigned, bucket pride-public, object key = archive-relative path YYYY/MM/ACCESSION/filename) against the internal endpoint instead of the public hh.fire endpoint used by `s3`. - pride.py: FIRE_S3_URL constant (env-overridable via PRIDEPY_FIRE_ENDPOINT); download_files_from_s3() gains an endpoint_url arg and derives the FTP URL robustly via _get_download_url(); fire routed in _batch_download_by_protocol. - `fire` is requested-only: tried first then falls back to the public protocols, and is never folded into another protocol's fallback (unreachable outside EBI, so no wasted retries there). - CLI: `fire` added to --protocol choices across subcommands + help text. - tests: internal-endpoint selection + S3 key derivation, requested-only order. Equivalent to: aws --no-sign-request --endpoint-url https://hl.fire.sdo.ebi.ac.uk \ s3 cp s3://pride-public/2015/08/PXD002137/ . --- pridepy/download/pride.py | 59 +++++++++++++++++++++++------- pridepy/pridepy.py | 24 +++++++++--- pridepy/tests/test_review_fixes.py | 36 ++++++++++++++++++ 3 files changed, 101 insertions(+), 18 deletions(-) diff --git a/pridepy/download/pride.py b/pridepy/download/pride.py index 80f9348..c9a0d66 100644 --- a/pridepy/download/pride.py +++ b/pridepy/download/pride.py @@ -53,6 +53,14 @@ class PrideProvider(Provider): ARCHIVE_FTP_URL_PREFIX: ClassVar[str] = "ftp://ftp.pride.ebi.ac.uk/" ARCHIVE_HTTPS_URL_PREFIX: ClassVar[str] = "https://ftp.pride.ebi.ac.uk/" S3_URL: ClassVar[str] = "https://hh.fire.sdo.ebi.ac.uk" + # EBI-internal FIRE S3 endpoint. Only reachable from within EBI + # infrastructure (compute/login nodes), where it is markedly faster and + # more reliable than the public FTP/HTTPS/Globus paths — which is the + # whole point of the ``fire`` protocol. Overridable via the + # ``PRIDEPY_FIRE_ENDPOINT`` env var for sites with a different host. + FIRE_S3_URL: ClassVar[str] = os.environ.get( + "PRIDEPY_FIRE_ENDPOINT", "https://hl.fire.sdo.ebi.ac.uk" + ) S3_BUCKET: ClassVar[str] = "pride-public" PROTOCOL_ORDER: ClassVar[List[str]] = ["aspera", "s3", "ftp", "globus"] @@ -138,7 +146,16 @@ def get_submitted_file_path_prefix(self, accession): def _protocol_sequence(protocol: str) -> List[str]: """ Build the ordered list of protocols to try for a requested download mode. + + ``fire`` (the EBI-internal FIRE S3 endpoint) is only ever tried when it + is explicitly requested — it is never folded into another protocol's + fallback chain, because it is unreachable outside EBI and would just + waste retries there. When requested, it is tried first and then falls + back to the public protocols so a run started outside EBI still + completes. """ + if protocol == "fire": + return ["fire"] + PrideProvider.PROTOCOL_ORDER if protocol not in PrideProvider.PROTOCOL_ORDER: return [] return [protocol] + [p for p in PrideProvider.PROTOCOL_ORDER if p != protocol] @@ -218,7 +235,9 @@ def _get_download_url(file_record: Dict, protocol: str) -> str: PrideProvider.ARCHIVE_HTTPS_URL_PREFIX, 1, ) - if protocol == "s3": + if protocol in ("s3", "fire"): + # Both S3 modes derive the object key from the FTP path; they + # differ only in the FIRE endpoint host (external vs EBI-internal). return ftp_url raise ValueError(f"Unsupported protocol: {protocol}") @@ -511,17 +530,26 @@ def download_files_from_globus( @staticmethod def download_files_from_s3( - file_list_json: List[Dict], output_folder: str, skip_if_downloaded_already + file_list_json: List[Dict], + output_folder: str, + skip_if_downloaded_already, + endpoint_url: Optional[str] = None, ): """ - Download files using S3 transfer URL with a progress bar and retry logic. + Download files from a FIRE S3 endpoint with a progress bar and retry logic. + :param file_list_json: file list in JSON format :param output_folder: folder to download the files :param skip_if_downloaded_already: Boolean value to skip the download if the file has already been downloaded. + :param endpoint_url: FIRE S3 endpoint. Defaults to the public + :attr:`S3_URL` (``hh.fire``); pass :attr:`FIRE_S3_URL` (``hl.fire``) + for the EBI-internal ``fire`` protocol. """ if not os.path.isdir(output_folder): os.makedirs(output_folder, exist_ok=True) + endpoint_url = endpoint_url or PrideProvider.S3_URL + # Retry and timeout config retry_config = Config( retries={"max_attempts": 5, "mode": "standard"}, @@ -533,19 +561,21 @@ def download_files_from_s3( s3_resource = boto3.resource( "s3", config=retry_config, - endpoint_url=PrideProvider.S3_URL, + endpoint_url=endpoint_url, ) bucket = s3_resource.Bucket(PrideProvider.S3_BUCKET) + logging.info( + "Downloading %d file(s) from FIRE S3 endpoint %s (bucket %s)", + len(file_list_json), endpoint_url, PrideProvider.S3_BUCKET, + ) failed: List[str] = [] for file in file_list_json: try: - # Determine S3 or FTP path - download_url = ( - file["publicFileLocations"][0]["value"] - if file["publicFileLocations"][0]["name"] == "FTP Protocol" - else file["publicFileLocations"][1]["value"] - ) + # Resolve the canonical FTP URL, then map it to the S3 object + # key: pride-public mirrors the archive layout, so the key is + # the archive-relative path (YYYY/MM/ACCESSION/filename). + download_url = PrideProvider._get_download_url(file, "ftp") ftp_base_url = "ftp://ftp.pride.ebi.ac.uk/pride/data/archive/" s3_path = download_url.replace(ftp_base_url, "") @@ -731,11 +761,14 @@ def _batch_download_by_protocol( download_threads=download_threads, ) return - if protocol == "s3": + if protocol in ("s3", "fire"): PrideProvider.download_files_from_s3( file_list, output_folder, skip_if_downloaded_already=skip_if_downloaded_already, + endpoint_url=( + PrideProvider.FIRE_S3_URL if protocol == "fire" else PrideProvider.S3_URL + ), ) return raise ValueError(f"Unsupported protocol: {protocol}") @@ -925,9 +958,9 @@ def _download_files_batch( :param aspera_maximum_bandwidth: parameter in Aspera sets the maximum bandwidth for the transfer. :param skip_if_downloaded_already: Boolean value to skip the download if the file has already been downloaded. """ - protocols_supported = ["ftp", "aspera", "globus", "s3"] + protocols_supported = ["ftp", "aspera", "globus", "s3", "fire"] if protocol not in protocols_supported: - logging.error("Protocol should be one of ftp, aspera, globus, s3") + logging.error("Protocol should be one of ftp, aspera, globus, s3, fire") return os.makedirs(output_folder, exist_ok=True) diff --git a/pridepy/pridepy.py b/pridepy/pridepy.py index 17723cf..bfeb27a 100644 --- a/pridepy/pridepy.py +++ b/pridepy/pridepy.py @@ -6,7 +6,9 @@ from pridepy.pdc import download_pdc_files as run_pdc_download from pridepy.project.project import Project -PROTOCOL_CHOICES = click.Choice(["ftp", "aspera", "globus", "s3"], case_sensitive=False) +PROTOCOL_CHOICES = click.Choice( + ["ftp", "aspera", "globus", "s3", "fire"], case_sensitive=False +) @click.group() @@ -25,7 +27,10 @@ def main(): "--protocol", default="ftp", type=PROTOCOL_CHOICES, - help="Protocol to use for download: ftp, aspera, globus, s3. Default is ftp with fallback enabled.", + help="Protocol to use for download: ftp, aspera, globus, s3, fire. " + "'fire' uses the EBI-internal FIRE S3 endpoint (only reachable inside EBI " + "infrastructure; falls back to the public protocols elsewhere). " + "Default is ftp with fallback enabled.", ) @click.option( "-o", @@ -119,7 +124,10 @@ def download_all_public_raw_files( "--protocol", default="ftp", type=PROTOCOL_CHOICES, - help="Protocol to use for download: ftp, aspera, globus, s3. Default is ftp with fallback enabled.", + help="Protocol to use for download: ftp, aspera, globus, s3, fire. " + "'fire' uses the EBI-internal FIRE S3 endpoint (only reachable inside EBI " + "infrastructure; falls back to the public protocols elsewhere). " + "Default is ftp with fallback enabled.", ) @click.option( "-o", @@ -232,7 +240,10 @@ def download_all_public_category_files( "--protocol", default="ftp", type=PROTOCOL_CHOICES, - help="Protocol to use for download: ftp, aspera, globus, s3. Default is ftp with fallback enabled.", + help="Protocol to use for download: ftp, aspera, globus, s3, fire. " + "'fire' uses the EBI-internal FIRE S3 endpoint (only reachable inside EBI " + "infrastructure; falls back to the public protocols elsewhere). " + "Default is ftp with fallback enabled.", ) @click.option("-f", "--file-name", required=True, help="fileName to be downloaded") @click.option( @@ -553,7 +564,10 @@ def _read_url_arguments(url_list_path, urls_csv=None): "--protocol", default="ftp", type=PROTOCOL_CHOICES, - help="Protocol to use for download: ftp, aspera, globus, s3. Default is ftp with fallback enabled.", + help="Protocol to use for download: ftp, aspera, globus, s3, fire. " + "'fire' uses the EBI-internal FIRE S3 endpoint (only reachable inside EBI " + "infrastructure; falls back to the public protocols elsewhere). " + "Default is ftp with fallback enabled.", ) @click.option( "-F", diff --git a/pridepy/tests/test_review_fixes.py b/pridepy/tests/test_review_fixes.py index 242941c..73ef10d 100644 --- a/pridepy/tests/test_review_fixes.py +++ b/pridepy/tests/test_review_fixes.py @@ -142,6 +142,42 @@ def test_s3_batch_raises_when_a_file_fails(self): records, tmp_dir, skip_if_downloaded_already=False ) + def test_fire_protocol_uses_internal_endpoint_and_derives_key(self): + """`fire` must hit the EBI-internal FIRE endpoint and map the FTP path + to the correct pride-public S3 object key.""" + records = [_pride_record("a.raw", accession="PXD002137", date="2015/08")] + mock_obj = Mock() + mock_obj.content_length = 10 + mock_bucket = Mock() + mock_bucket.Object.return_value = mock_obj + mock_resource = Mock() + mock_resource.Bucket.return_value = mock_bucket + with tempfile.TemporaryDirectory() as tmp_dir: + with patch( + "pridepy.download.pride.boto3.resource", return_value=mock_resource + ) as mock_boto: + PrideProvider._batch_download_by_protocol( + records, + tmp_dir, + protocol="fire", + skip_if_downloaded_already=False, + aspera_maximum_bandwidth="100M", + ) + # boto3.resource was created against the internal hl.fire endpoint. + assert mock_boto.call_args.kwargs["endpoint_url"] == PrideProvider.FIRE_S3_URL + assert "hl.fire.sdo.ebi.ac.uk" in PrideProvider.FIRE_S3_URL + # The object key is the archive-relative path, no ftp:// prefix. + mock_bucket.Object.assert_called_once_with("2015/08/PXD002137/a.raw") + + def test_fire_protocol_sequence_requested_only(self): + """`fire` is tried first when requested, then the public protocols; + it is never folded into another protocol's fallback chain.""" + assert PrideProvider._protocol_sequence("fire") == [ + "fire", "aspera", "s3", "ftp", "globus", + ] + assert "fire" not in PrideProvider._protocol_sequence("ftp") + assert "fire" not in PrideProvider._protocol_sequence("s3") + def test_download_files_forwards_protocol_to_get_download_url(self): seen = [] From d78a130f4759c77918d366765d26fe20af6b003f Mon Sep 17 00:00:00 2001 From: Yasset Perez-Riverol Date: Wed, 29 Jul 2026 16:34:20 +0100 Subject: [PATCH 9/9] fix(download): don't re-attempt fire in Phase-2 fallback A FIRE failure is endpoint-level (the EBI-internal host is unreachable), so it fails identically for every file; boto3 already retries each file 5x within the Phase-1 batch. Retrying fire per-file in Phase-2 just burned retries on connections that cannot succeed. Exclude fire from the per-file fallback so those files go straight to the public protocols. Addresses CodeRabbit review. --- pridepy/download/pride.py | 9 +++++++-- pridepy/tests/test_review_fixes.py | 26 ++++++++++++++++++++++++++ 2 files changed, 33 insertions(+), 2 deletions(-) diff --git a/pridepy/download/pride.py b/pridepy/download/pride.py index c9a0d66..d75f478 100644 --- a/pridepy/download/pride.py +++ b/pridepy/download/pride.py @@ -976,8 +976,13 @@ def _download_files_batch( protocol_sequence = PrideProvider._protocol_sequence(protocol) primary_protocol = protocol_sequence[0] - # Retry with the primary protocol first, then fall back to others - fallback_sequence = protocol_sequence + # Retry with the primary protocol first, then fall back to others. + # ``fire`` is excluded from the per-file fallback: a FIRE failure is an + # endpoint-level condition (the EBI-internal host is unreachable), so it + # fails identically for every file. Re-attempting it per file in Phase 2 + # would just burn retries on connections that cannot succeed — send those + # files straight to the public fallback protocols instead. + fallback_sequence = [p for p in protocol_sequence if p != "fire"] # Phase 1: batch download with the requested protocol. Reuses a single # FTP/S3 connection for all files (the previous behaviour) instead of diff --git a/pridepy/tests/test_review_fixes.py b/pridepy/tests/test_review_fixes.py index 73ef10d..2458d40 100644 --- a/pridepy/tests/test_review_fixes.py +++ b/pridepy/tests/test_review_fixes.py @@ -178,6 +178,32 @@ def test_fire_protocol_sequence_requested_only(self): assert "fire" not in PrideProvider._protocol_sequence("ftp") assert "fire" not in PrideProvider._protocol_sequence("s3") + def test_fire_not_retried_in_phase2_fallback(self): + """After a FIRE (endpoint-level) failure, the per-file Phase-2 fallback + must go straight to the public protocols and never re-attempt fire.""" + record = _pride_record("a.raw", accession="PXD002137", date="2015/08") + captured = {} + + def _fake_fallback(*, file_record, output_folder, protocol_sequence, **kw): + captured["seq"] = protocol_sequence + return True # pretend a public protocol succeeded + + with tempfile.TemporaryDirectory() as tmp_dir: + with patch.object(PrideProvider, "_batch_download_by_protocol"), \ + patch("pridepy.download.pride._provider_util.validate_download", + return_value=(False, "missing")), \ + patch.object(PrideProvider, "_download_with_fallback", + side_effect=_fake_fallback): + PrideProvider._download_files_batch( + file_list_json=[record], + accession="PXD002137", + output_folder=tmp_dir, + skip_if_downloaded_already=False, + protocol="fire", + ) + assert captured["seq"] == ["aspera", "s3", "ftp", "globus"] + assert "fire" not in captured["seq"] + def test_download_files_forwards_protocol_to_get_download_url(self): seen = []