Faster iProX/ProteomeXchange downloads: expose -w parallel files, wire download-px-raw-files, opt-in iProX Aspera

[ypriverol]

Why

Downloading a ProteomeXchange dataset hosted in iProX (e.g. PXD077178 → iProX IPX0003578000, 300+ files) is very slow. The root cause is the client, not the protocol: on dev, download-px-raw-files has no concurrency knobs at all — it downloads strictly one file at a time over a single connection to a long-latency host. iProX's server (download.iprox.org, nginx) does support Accept-Ranges: bytes and there is plenty of aggregate-bandwidth headroom from using more connections.

dev already added per-file Range segmentation (-t/--threads, reused by the PDC subsystem) and has an internal parallel_files (across-file concurrency) plumbed through Provider/transport — but no CLI command ever exposes it, so across-file parallelism is unreachable, and the PX command is untouched.

What this PR does (hybrid — builds on the existing model, no rewrite)

This is intentionally zero-touch on the existing -t/--threads segmentation (_multipart_download and PDC reuse are byte-identical). It only adds the missing pieces:

1. Expose across-file concurrency on the CLI — new -w/--parallel-files (files downloaded in parallel, IntRange(1,32), default 1) on download-all-public-raw-files, download-all-public-category-files, download-files-by-list, download-files-by-url, and download-px-raw-files. It composes with the existing -t/--threads (Range segments per file): peak connections ≈ parallel_files × threads.

2. Wire download-px-raw-files (previously had none) with -w/--parallel-files, -t/--threads, and -p/--protocol, threaded through Client.download_px_raw_files → ProteomeXchangeProvider.download_from_accession_or_url → Provider.download_files. This is the actual fix for the iProX slowness.

3. Opt-in iProX Aspera on download-px-raw-files via --protocol aspera with --iprox-user/--iprox-password (env fallback IPROX_USER / IPROX_ASPERA_PASSWORD). Uses the bundled ascp (-QT -P 33001 -l <bw> -k 2, source user@download.iprox.org:/data/iprox/<path>). Password is passed only via the ASPERA_SCP_PASS env var (never argv), masked in logs; missing credentials fail fast; non-iProX URLs under --protocol aspera raise a clear error (no silent HTTP fallthrough); honors --preserve-structure/flatten. HTTP-parallel remains the default.

Usage

# Parallel across files (recommended, no account needed)
pridepy download-px-raw-files -a PXD077178 -o ./PXD077178 -w 8

# Combine across-file parallelism with per-file Range segments
pridepy download-px-raw-files -a PXD077178 -o ./out -w 8 -t 4

# Aspera (requires an iProX account)
IPROX_ASPERA_PASSWORD=... pridepy download-px-raw-files -a PXD077178 -o ./out \
  --protocol aspera --iprox-user <user>

-w = files in parallel; -t = Range segments per file; they multiply into total connections.

Tests

Full suite: 162 passed, 4 skipped. New test_iprox_aspera.py (argv/env construction, password-never-in-argv, missing-credentials ValueError, PX aspera routing, CalledProcessError → RuntimeError, skip-if-downloaded). test_cli_flatten.py updated to assert the new -w wiring (default parallel_files == 1, -w 8 → 8) instead of the old "not present" checks.

Known follow-ups (Minor, from review — not blocking)

• iprox.py: the ascp subprocess doesn't capture stdout/stderr (cosmetic; consider capture_output=True + logging on failure).
• proteomexchange.py aspera branch: get_download_url raises mid-scan on an empty publicFileLocations (harmless for real iProX XML; could skip gracefully).
• download-px-raw-files exposes no --aspera-maximum-bandwidth (Aspera fixed at 100M).

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 1586022e-2dfa-424d-915f-3045e7759d9b

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/iprox-fast-download

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[qodo-code-review]

PR Summary by Qodo

Expose parallel file downloads (-w) and add opt-in iProX Aspera for PX datasets

✨ Enhancement 🐞 Bug fix 🧪 Tests 📝 Documentation 🕐 40+ Minutes

AI Description

• Expose across-file concurrency via -w/--parallel-files across download CLI commands.
• Wire download-px-raw-files to pass protocol, threads, and parallelism into provider downloads.
• Add opt-in iProX Aspera path for PX downloads with credential/env handling and tests.

Diagram

graph TD
  A["CLI: download-px-raw-files"] --> B["Client.download_px_raw_files"] --> C["ProteomeXchangeProvider.download_from_accession_or_url"]
  C --> D{"protocol == aspera?"}
  D -- "no (ftp/http)" --> E["Provider.download_files"] --> F["transport.download_http_urls"]
  D -- "yes (iProX URLs)" --> G["IproxProvider.aspera_download"] --> H{{"ascp (Aspera)"}}

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Make Aspera a generic transport module

• ➕ Centralizes subprocess invocation, logging, and error handling across providers
• ➕ Easier to add future Aspera knobs (bandwidth, retries, concurrency) consistently
• ➖ More refactor churn now; higher risk of breaking existing PRIDE Aspera behavior
• ➖ Requires designing a shared URL→ascp source mapping interface for multiple providers

2. Reuse PRIDE provider’s multi-protocol fallback for PX downloads

• ➕ Leverages existing, battle-tested protocol selection and fallback patterns
• ➕ Potentially reduces custom branching inside ProteomeXchangeProvider
• ➖ Conceptual mismatch: PX downloads are not inherently PRIDE-hosted; mapping fallback semantics could be confusing
• ➖ Still needs iProX-specific source path construction and credential gating

3. Async/asyncio-based downloader for high concurrency

• ➕ Better scalability than thread pools for many small files
• ➕ Could unify per-file and across-file concurrency with backpressure
• ➖ Large rewrite; much higher review and regression risk
• ➖ Would duplicate existing, working threadpool + Range segmentation design

Recommendation: The current approach (minimal wiring to expose existing parallel_files + an opt-in iProX-only Aspera path) is the best fit for the stated goal: it fixes the practical slowness without rewriting the established HTTP Range segmentation logic. If Aspera support expands beyond iProX, consider extracting the ascp invocation into a shared transport helper to standardize logging, capture stderr on failures, and add bandwidth configuration.

Files changed (7) +529 / -9

Enhancement (4) +227 / -4

client.pyExpose PX download knobs on Client facade +14/-1

Expose PX download knobs on Client facade

• Extends Client.download_px_raw_files with parallel_files, download_threads, protocol, and iProX credential parameters. Forwards these values into ProteomeXchangeProvider.download_from_accession_or_url.

pridepy/download/client.py

iprox.pyAdd iProX Aspera downloader invoking bundled ascp +64/-0

Add iProX Aspera downloader invoking bundled ascp

• Introduces IproxProvider.aspera_download to download iProX-hosted URLs via ascp on port 33001 with a fixed root path. Enforces credential presence and passes the password via ASPERA_SCP_PASS env var (not argv), supports skip-if-downloaded, and aggregates failures into a RuntimeError.

pridepy/download/iprox.py

proteomexchange.pyRoute PX downloads through parallel HTTP or iProX Aspera (opt-in) +49/-3

Route PX downloads through parallel HTTP or iProX Aspera (opt-in)

• Adds parameters for parallel_files, download_threads, protocol, and iProX credentials to the PX end-to-end download method. Implements an aspera branch that filters for iProX-hosted URLs, honors flatten/preserve-structure using flatten_relative_paths, and calls IproxProvider.aspera_download; otherwise continues through the default Provider.download_files path with concurrency passed through.

pridepy/download/proteomexchange.py

pridepy.pyExpose -w parallel files across CLI commands and wire PX options +100/-0

Expose -w parallel files across CLI commands and wire PX options

• Adds a -w/--parallel-files click option to multiple download commands and threads it into Client calls. Updates download-px-raw-files to accept protocol (-p), threads (-t), parallel files (-w), preserve-structure, and iProX credential options with envvar fallbacks.

pridepy/pridepy.py

Tests (2) +251 / -5

test_cli_flatten.pyUpdate CLI wiring tests to assert parallel_files propagation +95/-5

Update CLI wiring tests to assert parallel_files propagation

• Adjusts existing tests to expect default parallel_files==1 and adds cases verifying -w is forwarded for raw/category/list/url commands. Adds a PX CLI test confirming protocol/threads/parallel options are passed through to the client call.

pridepy/tests/test_cli_flatten.py

test_iprox_aspera.pyAdd unit tests for iProX Aspera command construction and PX routing +156/-0

Add unit tests for iProX Aspera command construction and PX routing

• Adds tests that validate ascp argv construction, password-never-in-argv behavior (env only), missing-credential ValueError, subprocess failure mapping to RuntimeError, and skip-if-downloaded short-circuiting. Verifies ProteomeXchangeProvider routes aspera downloads to iProX, and that flatten/preserve-structure behavior is honored for destination paths.

pridepy/tests/test_iprox_aspera.py

Documentation (1) +51 / -0

usage.mdDocument -w parallel files and iProX Aspera usage for PX downloads +51/-0

Document -w parallel files and iProX Aspera usage for PX downloads

• Adds CLI option documentation for protocol selection, parallel file workers (-w), per-file threads (-t), preserve-structure, and iProX credential flags. Includes usage examples and explains the connection multiplier (parallel_files × threads) plus recommended env-var password usage.

docs/usage.md

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Aspera drops non-iProX URLs ✓ Resolved 🐞 Bug ≡ Correctness

Description

ProteomeXchangeProvider.download_from_accession_or_url(protocol='aspera') filters PX records to
only URLs whose string contains download.iprox.org and then returns after downloading those,
silently skipping any other host URLs present in the same PX XML. For mixed-host datasets this can
complete “successfully” while leaving part of the dataset undownloaded and it also uses a non-strict
host check (substring match).

Code

pridepy/download/proteomexchange.py[R199-207]

+        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:

Evidence

PX list_files() yields records for all raw-file URIs, but the new Aspera branch only keeps
download.iprox.org URLs and returns after downloading them, so any other-host URLs in records
are skipped entirely; the host check is also only a substring match.

pridepy/download/proteomexchange.py[138-166]
pridepy/download/proteomexchange.py[199-228]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
When `protocol == "aspera"`, PX downloads currently select only URLs containing `"download.iprox.org"` and ignore the rest, then `return`. This causes partial downloads for PX datasets that contain URLs from multiple hosts. The host check is also a substring match, not a strict `netloc` validation.

### Issue Context
- PX `list_files()` produces one record per raw-file URI from the PX XML, and those URIs can point at different repositories/hosts.
- The Aspera branch currently routes only iProX-hosted files.

### Fix Focus Areas
- pridepy/download/proteomexchange.py[199-227]

### Suggested fix
1. Parse each `loc` with `urlparse(loc)` and require `parsed.netloc == "download.iprox.org"` (optionally allow ports).
2. If **any** record is not iProX-hosted, fail fast with a `ValueError` listing how many (and maybe examples) were excluded, instead of silently skipping.
  - Alternatively, if you want mixed-mode support, explicitly download non-iProX URLs via the existing HTTP/FTP path (still do not silently fall back; log clearly and/or provide a flag).
3. Keep current behavior of raising when *no* iProX URLs are present.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Aspera path escape possible ✓ Resolved 🐞 Bug ⛨ Security

Description

IproxProvider.aspera_download builds destinations with os.path.join(output_folder, relpath)
without the transport layer’s _safe_join defense, so absolute/.. paths in relative_paths can
write outside output_folder. Additionally, if relpath is missing (None / shorter list), dest
becomes output_folder and skip_if_downloaded_already=True will incorrectly skip because the
directory exists.

Code

pridepy/download/iprox.py[R85-94]

+        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

Evidence

The transport layer has a dedicated _safe_join/_dest_path to prevent ../absolute escapes, but
the new Aspera implementation uses raw os.path.join and even falls back to output_folder when
relpath is missing, which makes the subsequent existence check incorrect.

pridepy/download/iprox.py[58-110]
pridepy/download/transport.py[23-54]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The new iProX Aspera downloader constructs the local destination path via `os.path.join(output_folder, relpath)` and doesn’t apply the repo’s existing safe-join logic that prevents `..` traversal / absolute-path escapes. This is a path traversal risk and can also break `--skip-if-downloaded-already` when `relpath` is missing.

### Issue Context
- `pridepy/download/transport.py` already implements `_safe_join()` and `_dest_path()` to defend against malicious or malformed relative paths.
- The HTTP/FTP transports use these helpers; the new Aspera path does not.

### Fix Focus Areas
- pridepy/download/iprox.py[85-94]
- pridepy/download/transport.py[23-54]

### Suggested fix
1. Resolve `dest` using the same logic as the HTTP/FTP path:
  - If `relpath` is falsy, use `os.path.basename(urlparse(url).path)` to create a file path under `output_folder`.
  - If `relpath` is provided, pass it through the safe-join logic (either:
    - import and use `transport._safe_join`/`transport._dest_path`, or
    - copy the safe-join logic locally if you don’t want to use a private helper).
2. Ensure `skip_if_downloaded_already` checks the resolved *file path*, not the directory (`output_folder`).
3. (Optional hardening) Validate `urlparse(url).netloc == ASPERA_HOST` inside `aspera_download` and raise `ValueError` otherwise, so callers can’t accidentally pass non-iProX URLs.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---