fix(iProX Aspera): batched --file-list form (match working ascp); password (env/prompt) or --aspera-key; no batch hang#117
Open
ypriverol wants to merge 3 commits into
Open
fix(iProX Aspera): batched --file-list form (match working ascp); password (env/prompt) or --aspera-key; no batch hang#117ypriverol wants to merge 3 commits into
ypriverol wants to merge 3 commits into
Conversation
…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)
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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
PR Summary by QodoFix iProX Aspera downloads: key auth + batched --file-list (drop password)
AI Description
Diagram
High-Level Assessment
Files changed (6)
|
Contributor
Code Review by Qodo
1. Aspera ignores skip flag
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
199
to
209
| 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." |
Contributor
There was a problem hiding this comment.
2. Aspera ignores skip flag 🐞 Bug ≡ Correctness
When protocol == "aspera", skip_if_downloaded_already is not forwarded and cannot be honored because IproxProvider.aspera_download() no longer accepts it. This can cause re-downloading large datasets even when the CLI flag requests skipping already-downloaded files.
Agent Prompt
## Issue description
The CLI exposes `--skip-if-downloaded-already`, and `Files.download_px_raw_files()` forwards it into `ProteomeXchangeProvider.download_from_accession_or_url()`. However, the Aspera branch ignores it and routes into `IproxProvider.aspera_download()` which no longer supports skipping.
## Issue Context
Aspera now uses a single `ascp --file-list` session and preserves remote directory structure under `output_folder`, so the expected local destination for a remote path like `/IPX.../file.raw` is typically `os.path.join(output_folder, remote_path.lstrip("/"))`.
## Fix Focus Areas
- pridepy/download/proteomexchange.py[199-226]
- pridepy/download/iprox.py[72-128]
## Suggested change
- Add a `skip_if_downloaded_already: bool` parameter back to `IproxProvider.aspera_download()`.
- In the Aspera branch in `ProteomeXchangeProvider.download_from_accession_or_url()`, forward `skip_if_downloaded_already`.
- Implement skipping by filtering `remote_paths` before writing the file-list (exclude entries whose destination file already exists and has non-zero size), and no-op if all paths are skipped.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+105
to
+127
| 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) |
Contributor
There was a problem hiding this comment.
3. Tempfile cleanup masks errors 🐞 Bug ☼ Reliability
IproxProvider.aspera_download() unconditionally calls os.remove(list_path) in a finally block without guarding against FileNotFoundError. If the file is unexpectedly missing, that cleanup exception will override/mask the real transfer failure (or success).
Agent Prompt
## Issue description
The `finally: os.remove(list_path)` cleanup can raise `FileNotFoundError` and mask the original exception (including the `RuntimeError` raised for `subprocess.CalledProcessError`).
## Issue Context
This is an edge case, but it is a standard robustness pattern to ensure cleanup never hides the primary failure signal.
## Fix Focus Areas
- pridepy/download/iprox.py[105-127]
## Suggested change
Wrap `os.remove(list_path)` in `try/except FileNotFoundError:` (optionally log at debug level), so the original error is preserved.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
…ng --file-list form; key optional; no interactive hang
ypriverol
changed the title
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
1 participant
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.
Why
The iProX Aspera support merged in #115 didn't work: it authenticated with a password via
ASPERA_SCP_PASSand used theuser@host:/data/iprox/<path> destform, oneascpper file — iProX rejects that. A verified-working manual invocation is the batched--file-list/--mode recv --host --userform:where each file-list line is the remote path ==
urlparse(url).pathof our records, e.g./IPX0003474000/IPX0003474001/<file>.raw(no/data/iproxprefix). The invocation form was the bug, not the auth method — password auth works fine with this form (confirmed end-to-end againstdownload.iprox.org).What changed
IproxProvider.aspera_downloadrewritten to the working batched--file-listform (oneascpsession for the whole dataset; FASP parallelizes internally). Temp file-list always cleaned up.IPROX_ASPERA_PASSWORDenv var (→ASPERA_SCP_PASS), or a hidden interactive prompt when on a TTY. No plaintext password CLI flag (per the earlier security review).--aspera-key <path>optional alternative (-i key) for users with a registered key.ValueError(does not fall through toascp's interactivePassword:prompt).ascpruns withstdin=subprocess.DEVNULLso a missing/rejected credential errors immediately instead of blocking onPassword:— critical for sbatch jobs.ascp --file-listrecreates the/IPX…/IPX…/tree under the output dir, so the Aspera path preserves structure (flatten N/A) — documented.Usage
Interactively (TTY), if
IPROX_ASPERA_PASSWORDisn't set you're prompted (hidden). In batch you must set the env var (or use--aspera-key) or it errors fast. HTTP parallel (-w) remains the account-free default.Tests
test_iprox_aspera.py: both auth modes (key →-i, noASPERA_SCP_PASS; password →ASPERA_SCP_PASSset, no-i, password never on argv),stdin=DEVNULLasserted, missing-credential →ValueError(ascp not called), missing-user / nonexistent-key errors, failed transfer →RuntimeError, file-list contents = URL paths, and PX aspera routing forwards key/password. Full suite: 167 passed, 4 skipped.Verified
The batched form + password auth was confirmed working end-to-end against
download.iprox.org(99-file dataset). The generic bundled Aspera key is not accepted by iProX, which is why password (or a site-registered key via--aspera-key) is the path here.