Conversation
…ownload-px-raw-files (protocol/threads/parallel)
…-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.
… host, honor -w) + concurrency cap + hidden password prompt
Faster iProX/ProteomeXchange downloads: expose -w parallel files, wire download-px-raw-files, opt-in iProX Aspera
Contributor
|
Warning Review limit reached
Next review available in: 42 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThis PR adds Aspera-based fast downloads for iProX datasets, introduces ChangesiProX Aspera and parallel download plumbing
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR upgrades pridepy’s bulk-download capabilities by adding across-file parallelism (--parallel-files) and integrating iProX Aspera transfers for faster ProteomeXchange (PX) downloads, with corresponding CLI, transport-layer, and documentation updates.
Changes:
- Add
--parallel-files/-wplumbing through CLI → client → provider → transport, plus a combined HTTP connection cap. - Add iProX Aspera support (ascp command construction, credential handling, and PX routing when
--protocol asperais selected). - Expand/adjust tests and usage documentation for parallelism and Aspera.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pridepy/tests/test_iprox_aspera.py | New tests for iProX Aspera command/env construction, routing, flattening, and safety checks. |
| pridepy/tests/test_download_resilience.py | Adds a regression test ensuring the combined connection cap clamps per-file threads. |
| pridepy/tests/test_cli_flatten.py | Updates CLI tests to assert parallel_files default/flag behavior across commands. |
| pridepy/pridepy.py | Adds CLI flags for --parallel-files, PX --protocol, and iProX credential handling/prompting. |
| pridepy/download/transport.py | Introduces MAX_TOTAL_HTTP_CONNECTIONS and clamps HTTP concurrency. |
| pridepy/download/proteomexchange.py | Routes PX downloads to iProX Aspera when requested; supports flattening in that branch. |
| pridepy/download/iprox.py | Implements iProX Aspera transfer execution (including parallel subprocess runs) and safe destination handling. |
| pridepy/download/client.py | Propagates new PX download parameters through the client API. |
| docs/usage.md | Documents new PX CLI options, parallelism guidance, and iProX Aspera password handling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
PR Summary by QodoSpeed up PX/iProX downloads with parallelism and opt-in Aspera support
AI Description
Diagram
High-Level Assessment
Files changed (9)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (5)
docs/usage.md (2)
253-257: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider noting the internal concurrency cap.
transport.download_http_urlsclampsdownload_threadswhenparallel_files × download_threadsexceeds an internal cap (seeMAX_TOTAL_HTTP_CONNECTIONS), so actual concurrency won't scale linearly past that point. A brief caveat here would prevent user confusion when e.g.-w 32 -t 32doesn't yield 1024 real connections.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/usage.md` around lines 253 - 257, Update the “Fast downloads” section in usage.md to mention the internal concurrency limit enforced by transport.download_http_urls. Note that download_threads is clamped when parallel_files × download_threads exceeds MAX_TOTAL_HTTP_CONNECTIONS, so actual concurrency may stop scaling linearly; keep the guidance brief and tie it to the existing parallel_files × threads explanation.
242-242: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winProtocol table overstates globus/s3 support for
download-px-raw-files.For this command, only
ftpandasperaare actually implemented —base.Provider.download_files(used for the non-aspera path) just warns and falls through to whatever URL scheme is in the PX XML ifglobus/s3is requested; there's no genuine multi-protocol fallback here (unlike PRIDE's_protocol_sequence). Consider clarifying thatglobus/s3aren't meaningfully supported for PX/iProX downloads to avoid confusing users.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/usage.md` at line 242, Clarify the `download-px-raw-files` protocol documentation so it does not imply real `globus` or `s3` support. Update the protocol table entry in `docs/usage.md` to reflect that the command only meaningfully supports `ftp` and `aspera`, and note that the non-aspera path in `base.Provider.download_files` does not implement a true fallback sequence for other protocols.pridepy/pridepy.py (1)
66-74: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
--parallel-files/-woption definition across 5 commands.Same default, range, and help text repeated verbatim. Consider extracting a shared decorator (e.g.
parallel_files_option = click.option("-w", "--parallel-files", "parallel_files", default=1, type=click.IntRange(1, 32), help=...)) and applying it via@parallel_files_optionat each call site, so future tweaks to bounds/wording only need one edit.Also applies to: 179-187, 377-385, 681-689, 783-791
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pridepy/pridepy.py` around lines 66 - 74, The same `--parallel-files/-w` option is duplicated across multiple commands, so update `pridepy.py` to define a shared `click.option` decorator (for example a reusable `parallel_files_option`) with the common default, `IntRange`, and help text, then apply it to each command that currently declares the option. Keep the existing `parallel_files` parameter name and ensure all affected command functions use the shared decorator so future changes only need one edit.pridepy/download/proteomexchange.py (1)
199-238: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winiProX Aspera path has no bandwidth-throttling control.
IproxProvider.aspera_downloadacceptsmaximum_bandwidth(default"100M"), but neitherdownload_from_accession_or_urlnorClient.download_px_raw_filesnor thedownload-px-raw-filesCLI expose a way to override it — every PX-driven iProX Aspera transfer is capped at 100M regardless of user intent. Given Aspera is specifically pitched for "very large bulk transfers," bandwidth control is a reasonable expectation.♻️ Sketch of the plumbing needed
def download_from_accession_or_url( self, px_id_or_url: str, output_folder: str, skip_if_downloaded_already: bool = True, flatten: bool = True, parallel_files: int = 1, download_threads: int = 1, protocol: str = "ftp", + aspera_maximum_bandwidth: str = "100M", iprox_user: Optional[str] = None, iprox_password: Optional[str] = None, ) -> None: ... IproxProvider.aspera_download( urls=iprox_urls, output_folder=output_folder, relative_paths=dest_rels, user=iprox_user, password=iprox_password, + maximum_bandwidth=aspera_maximum_bandwidth, skip_if_downloaded_already=skip_if_downloaded_already, parallel_files=parallel_files, )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pridepy/download/proteomexchange.py` around lines 199 - 238, The iProX Aspera download path currently hardcodes the default bandwidth limit because `download_from_accession_or_url` and the PX download plumbing never pass a bandwidth value through to `IproxProvider.aspera_download`. Add a `maximum_bandwidth` option to the PX download entry points (including `Client.download_px_raw_files` and the `download-px-raw-files` CLI), thread it through `download_from_accession_or_url`, and pass it into `IproxProvider.aspera_download` so users can override the default 100M cap; keep the existing default unchanged for backward compatibility.pridepy/tests/test_download_resilience.py (1)
250-282: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGood coverage of the intended clamp path; consider adding a case where
workersalone exceeds the cap.This test only exercises
parallel_files=32with 3 URLs (workers=3), which stays well underMAX_TOTAL_HTTP_CONNECTIONS. It doesn't cover the scenario wherelen(http_urls)andparallel_filesare both large enough thatworkersitself exceeds the cap (see the corresponding comment ontransport.pylines 852-865) — in that casedownload_threadsfloors to 1 but total connections can still exceed 64. Adding a test with e.g. 100 URLs andparallel_files=100would catch that gap once fixed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pridepy/tests/test_download_resilience.py` around lines 250 - 282, The current test only verifies the clamp when `parallel_files` is high but the effective `workers` count stays below the connection cap. Add a second assertion in `test_download_http_urls_clamps_combined_connection_cap` that uses many URLs and a large `parallel_files` value so `workers` itself exceeds `transport.MAX_TOTAL_HTTP_CONNECTIONS`, and verify `download_http_urls` clamps total connections appropriately (not just per-thread `download_threads`) while still emitting the warning.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pridepy/download/iprox.py`:
- Around line 82-97: The skip logic in `download`/`cls.download` treats any
non-empty `dest` as complete, which can incorrectly skip a failed partial Aspera
transfer. Update the `subprocess.run(argv, check=True, env=env)` flow so `ascp`
writes to a temporary/partial target and only promotes it to `dest` after
success, or add a remote-size/complete-file validation before the
`os.path.isfile(dest)` skip check. Keep the fix localized around the
`skip_if_downloaded_already` block and the Aspera command construction.
- Around line 74-75: Validate the input in IproxProvider.aspera_download()
before constructing the Aspera source path: ensure the parsed URL has the
expected iProX scheme/host, and reject any path containing traversal segments
like “..”. Update the logic around urlparse(url).path and the source string so
only normalized paths under the intended /data/iprox/<accession> subtree are
accepted before calling ascp.
- Around line 134-136: The worker count used before the ThreadPoolExecutor/ascp
launch is only bounded by len(urls), so user-controlled parallel_files can still
create too many subprocesses. Add a small provider-level maximum in the worker
calculation around the ThreadPoolExecutor block in pridepy/download/iprox.py,
clamp workers to that cap, and emit a log when the requested parallel_files
value is reduced.
In `@pridepy/download/transport.py`:
- Around line 852-865: The connection cap logic in Client.download_http_urls
only clamps download_threads, so callers can still exceed
MAX_TOTAL_HTTP_CONNECTIONS when workers is very large. Update the worker
calculation near workers and clamped_threads to bound workers as well (or
otherwise clamp the workers x download_threads product from both sides) before
starting downloads, keeping the final total HTTP connections within
MAX_TOTAL_HTTP_CONNECTIONS.
In `@pridepy/pridepy.py`:
- Around line 404-421: The password prompt path in download_px_raw_files is
running even when iprox_user was never provided. Update the aspera branch in
download_px_raw_files to validate iprox_user before calling click.prompt, and
fail fast with the existing credential error path if it is missing. Keep the fix
localized to the download_px_raw_files flow and its handoff to
IproxProvider.aspera_download so the prompt only appears when a user can
actually authenticate.
In `@pridepy/tests/test_iprox_aspera.py`:
- Line 143: Clean up the Ruff warnings in the new tests by fixing the unused
tuple unpack from run.call_args in the test that inspects the mock invocation:
assign the unused value to a dummy name (for example in the assertion around
run.call_args). Also update the pytest.raises(..., match=...) pattern in the
affected test(s) to escape the literal dot so the regex matches the exact error
text instead of treating the dot as a wildcard.
- Around line 279-281: The test in test_iprox_aspera should assert the order of
relative_paths instead of converting to a set, since its mapping to urls is
positional and a swapped pairing would be missed. Update the assertion near
asp.call_args.kwargs["relative_paths"] to compare the flattened, de-duped
basenames in sequence, preserving the existing no-subdirectories expectation
while checking the exact order.
---
Nitpick comments:
In `@docs/usage.md`:
- Around line 253-257: Update the “Fast downloads” section in usage.md to
mention the internal concurrency limit enforced by transport.download_http_urls.
Note that download_threads is clamped when parallel_files × download_threads
exceeds MAX_TOTAL_HTTP_CONNECTIONS, so actual concurrency may stop scaling
linearly; keep the guidance brief and tie it to the existing parallel_files ×
threads explanation.
- Line 242: Clarify the `download-px-raw-files` protocol documentation so it
does not imply real `globus` or `s3` support. Update the protocol table entry in
`docs/usage.md` to reflect that the command only meaningfully supports `ftp` and
`aspera`, and note that the non-aspera path in `base.Provider.download_files`
does not implement a true fallback sequence for other protocols.
In `@pridepy/download/proteomexchange.py`:
- Around line 199-238: The iProX Aspera download path currently hardcodes the
default bandwidth limit because `download_from_accession_or_url` and the PX
download plumbing never pass a bandwidth value through to
`IproxProvider.aspera_download`. Add a `maximum_bandwidth` option to the PX
download entry points (including `Client.download_px_raw_files` and the
`download-px-raw-files` CLI), thread it through
`download_from_accession_or_url`, and pass it into
`IproxProvider.aspera_download` so users can override the default 100M cap; keep
the existing default unchanged for backward compatibility.
In `@pridepy/pridepy.py`:
- Around line 66-74: The same `--parallel-files/-w` option is duplicated across
multiple commands, so update `pridepy.py` to define a shared `click.option`
decorator (for example a reusable `parallel_files_option`) with the common
default, `IntRange`, and help text, then apply it to each command that currently
declares the option. Keep the existing `parallel_files` parameter name and
ensure all affected command functions use the shared decorator so future changes
only need one edit.
In `@pridepy/tests/test_download_resilience.py`:
- Around line 250-282: The current test only verifies the clamp when
`parallel_files` is high but the effective `workers` count stays below the
connection cap. Add a second assertion in
`test_download_http_urls_clamps_combined_connection_cap` that uses many URLs and
a large `parallel_files` value so `workers` itself exceeds
`transport.MAX_TOTAL_HTTP_CONNECTIONS`, and verify `download_http_urls` clamps
total connections appropriately (not just per-thread `download_threads`) while
still emitting the warning.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6f054396-3341-4f94-889a-8434aeef2736
📒 Files selected for processing (9)
docs/usage.mdpridepy/download/client.pypridepy/download/iprox.pypridepy/download/proteomexchange.pypridepy/download/transport.pypridepy/pridepy.pypridepy/tests/test_cli_flatten.pypridepy/tests/test_download_resilience.pypridepy/tests/test_iprox_aspera.py
Comment on lines
852
to
+865
| 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 |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Files of interest =="
git ls-files pridepy/download/transport.py pridepy.py pridepy | sed -n '1,200p'
echo
echo "== transport.py around target lines =="
nl -ba pridepy/download/transport.py | sed -n '820,900p'
echo
echo "== Search for cap-related symbols and entry points =="
rg -n "MAX_TOTAL_HTTP_CONNECTIONS|parallel_files|download_threads|workers|download_http_urls|argparse|click|typer|fire|--parallel|--threads|-w|-t" pridepy . -g '!**/.git/**'Repository: PRIDE-Archive/pridepy
Length of output: 3029
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
path = Path("pridepy/download/transport.py")
lines = path.read_text().splitlines()
for start, end in [(1,120), (820,900)]:
print(f"== {path} lines {start}-{end} ==")
for i in range(start, min(end, len(lines)) + 1):
print(f"{i:4d}: {lines[i-1]}")
print()
PY
echo "== Search CLI entry points and validations =="
python3 - <<'PY'
from pathlib import Path
import re
targets = [
Path("pridepy/pridepy.py"),
Path("pridepy/download/client.py"),
Path("pridepy/download/base.py"),
Path("pridepy/download/by_url.py"),
Path("pridepy/tests/test_cli_flatten.py"),
]
for p in targets:
if p.exists():
print(f"\n## {p}")
text = p.read_text().splitlines()
for i, line in enumerate(text, 1):
if re.search(r'parallel_files|download_threads|MAX_TOTAL_HTTP_CONNECTIONS|--parallel|--threads|-w\b|-t\b|argparse|click|typer|fire', line):
print(f"{i:4d}: {line}")
PY
echo
echo "== Broader symbol search =="
rg -n "parallel_files|download_threads|MAX_TOTAL_HTTP_CONNECTIONS|download_http_urls|--parallel|--threads|-w\\b|-t\\b|argparse|click|typer|fire" pridepy -g '!**/.git/**'Repository: PRIDE-Archive/pridepy
Length of output: 46710
Clamp workers as well as download_threads
click.IntRange(1, 32) protects the CLI, but direct Client.download_http_urls callers can still pass parallel_files > 64. In that case clamped_threads drops to 1 and the total HTTP connections can still exceed MAX_TOTAL_HTTP_CONNECTIONS. Bound workers too, or clamp the product from both sides.
🔧 Proposed fix
- workers = max(1, min(parallel_files, len(http_urls)))
+ workers = max(1, min(parallel_files, len(http_urls), MAX_TOTAL_HTTP_CONNECTIONS))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| 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 | |
| workers = max(1, min(parallel_files, len(http_urls), MAX_TOTAL_HTTP_CONNECTIONS)) | |
| 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 |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pridepy/download/transport.py` around lines 852 - 865, The connection cap
logic in Client.download_http_urls only clamps download_threads, so callers can
still exceed MAX_TOTAL_HTTP_CONNECTIONS when workers is very large. Update the
worker calculation near workers and clamped_threads to bound workers as well (or
otherwise clamp the workers x download_threads product from both sides) before
starting downloads, keeping the final total HTTP connections within
MAX_TOTAL_HTTP_CONNECTIONS.
Comment on lines
404
to
+421
| 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, | ||
| iprox_user: 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) | ||
|
|
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Password prompt fires even when --iprox-user was never provided.
If protocol == "aspera" and IPROX_ASPERA_PASSWORD isn't set, click.prompt runs unconditionally — even if iprox_user is None. The user is prompted for a password only to hit IproxProvider.aspera_download's credential-validation error afterward. Fail fast before prompting.
🛡️ Proposed fix
password = os.environ.get("IPROX_ASPERA_PASSWORD")
- if protocol.lower() == "aspera" and not password:
+ if protocol.lower() == "aspera" and not iprox_user:
+ raise click.UsageError("--iprox-user is required with --protocol aspera.")
+ if protocol.lower() == "aspera" and not password:
password = click.prompt("iProX Aspera password", hide_input=True)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| 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, | |
| iprox_user: 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) | |
| 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, | |
| iprox_user: 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 iprox_user: | |
| raise click.UsageError("--iprox-user is required with --protocol aspera.") | |
| if protocol.lower() == "aspera" and not password: | |
| password = click.prompt("iProX Aspera password", hide_input=True) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pridepy/pridepy.py` around lines 404 - 421, The password prompt path in
download_px_raw_files is running even when iprox_user was never provided. Update
the aspera branch in download_px_raw_files to validate iprox_user before calling
click.prompt, and fail fast with the existing credential error path if it is
missing. Keep the fix localized to the download_px_raw_files flow and its
handoff to IproxProvider.aspera_download so the prompt only appears when a user
can actually authenticate.
Contributor
Code Review by Qodo
1. Aspera prompt can hang
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
…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/<path>, which does not work against iProX. Switch to the verified working invocation: key-based auth (-i <key>) 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.
…key now optional; zero-config with just --iprox-user)
…ng --file-list form; key optional; no interactive hang
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/<file> .
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.
feat(download): add EBI-internal FIRE S3 protocol (--protocol fire)
fix(iProX Aspera): batched --file-list form (match working ascp); password (env/prompt) or --aspera-key; no batch hang
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| ) | ||
| # 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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request introduces significant improvements to the
pridepytool, focusing on enhanced download performance and expanded protocol support for bulk dataset retrieval, especially from iProX. The main changes add parallel file downloads, per-file HTTP range threading, and robust Aspera (fast transfer) support for iProX datasets, along with corresponding updates to CLI options and documentation.Major feature additions and enhancements:
Parallel and segmented downloads:
--parallel-files/-w) and for using multiple threads per file (--threads/-t), allowing faster and more efficient bulk downloads. The total number of concurrent HTTP connections is capped for stability. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12]Aspera protocol integration for iProX:
--protocol aspera) as a fast transfer option for iProX datasets, including secure password handling and support for parallel Aspera transfers. The CLI and download logic now route iProX downloads via Aspera when requested and credentials are provided. [1] [2] [3] [4]User interface and documentation updates:
CLI and usage documentation:
docs/usage.md) with detailed instructions and examples for fast downloads using parallelism and Aspera. Also clarified iProX password handling for Aspera. [1] [2] [3] [4]Internal API and codebase improvements:
Refactoring and extensibility:
These changes collectively make
pridepymuch more capable for power users and large-scale dataset retrieval, while keeping the tool user-friendly and robust.Summary by CodeRabbit
New Features
Bug Fixes
Documentation