Fix the CI conda cache, the Snakefile f-strings, and blastp calls that fail but exit zero - #109
Merged
Merged
Conversation
Two rules built an output path from an f-string that mixed an
interpolated value with an escaped snakemake wildcard:
f"{ANALYSIS_NAME}_aggregated_features_{{plotting_mode}}.html"
The doubled braces are there so that the f-string renders the literal
`{plotting_mode}` that snakemake needs as a wildcard. That is easy to
misread, and a formatter can silently change what it means: running
`make format` under Python 3.12 or later rewrites the doubled braces to
single ones, which turns the wildcard into an interpolation of a name
that does not exist. (Python 3.12 changed how f-strings are tokenized,
see PEP 701; the versions of snakefmt and black pinned here predate it.)
Concatenating a plain string avoids the escaping entirely, produces a
byte-identical path, and cannot be rewritten this way. Verified that the
DAG still resolves the same targets, with the wildcards expanding to
`..._aggregated_features_pca_umap.html` and
`..._P60709_distribution_analysis.svg` as before, and that
`snakefmt --check` now passes under both Python 3.9 (which CI uses) and
Python 3.12+, where it previously wanted to rewrite the file.
Note that the doubled braces elsewhere in the Snakefile are not affected:
they appear in a plain (non-f) string and in a shell block, neither of
which a formatter rewrites.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WLpJZQ4W4XsL9NUUjjsry9
The `restore-keys` never matched anything: they begin with "snakemake-conda-" while the key begins with "conda-". Correcting the prefix would be worse than removing them. A `restore-keys` match still leaves `cache-hit` false, so the env creation step runs regardless, and that step begins by deleting `.snakemake/conda`. Making the prefix match would therefore download a stale multi-gigabyte cache only to discard it -- strictly slower than the broken prefix that never matched anything. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WLpJZQ4W4XsL9NUUjjsry9
When the remote server refuses to queue a request, `blastp -remote` reports the error on
stderr, writes an empty results file, and still exits zero:
$ blastp -remote -db nr ... ; echo $?
Error: [blastp] bad_request: Could not queue request: DB operation failed.
0
`_run_blast_remote` printed that stderr but returned the zero exit code unchanged, so the
caller treated the failure as a success. The word-size backoff was therefore never
attempted -- which is the situation it exists for -- and the empty results file surfaced
later in `extract_blast_hits.py` as a misleading "no hits were returned".
Failure is now determined by `blast_call_failed`, which also treats an error on stderr as
a failure. An empty results file is deliberately not treated as a failure, because a query
that legitimately has no hits produces one too.
This affects the `remote` backend only; the `www` backend does not shell out to blastp.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WLpJZQ4W4XsL9NUUjjsry9
Keep both the blastp exit-zero failure detection from this branch and the www timeout/error-page fixes from #107.
There was a problem hiding this comment.
Pull request overview
This PR addresses a set of CI + pipeline reliability fixes: it improves GitHub Actions conda caching behavior, hardens BLAST remote execution to treat “stderr Error:” as a failure even when blastp -remote exits 0 (so retry/backoff can run), and updates Snakefile output path construction to avoid Python f-string brace parsing issues that affect snakefmt.
Changes:
- Remove ineffective (and potentially harmful)
restore-keysusage for the Snakemake conda env cache in CI. - Add
blast_call_failed()and use it to convertblastp -remote“Error:” on stderr into a non-zero failure signal even when the process exits 0. - Replace Snakefile f-strings containing Snakemake wildcards with string concatenation to avoid PEP 701 tokenization/formatting issues.
- Add unit tests covering the “exit 0 but stderr contains Error:” behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
Snakefile |
Avoid f-strings for outputs containing Snakemake wildcards to prevent formatter/interpreter brace rewriting issues. |
ProteinCartography/blast_utils.py |
Introduce blast_call_failed() and ensure remote BLAST failures are detected even when exit code is 0. |
ProteinCartography/tests/test_blast_utils.py |
Add unit tests for blast_call_failed() and expected stderr/returncode combinations. |
.github/workflows/test.yml |
Remove mismatching/ineffective conda cache restore-keys and document why. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| stderr = result.stderr | ||
| if isinstance(stderr, bytes): | ||
| stderr = stderr.decode(errors="replace") |
| # back as a non-failure and the caller's word-size backoff never runs. Surface it as the | ||
| # failure it is, keeping the original stderr for the caller's error message. | ||
| if blast_call_failed(result) and result.returncode == 0: | ||
| print("[blast] blastp reported an error despite exiting zero; treating as a failure") |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (1)
ProteinCartography/blast_utils.py:612
- The new diagnostic
printhere doesn’t useflush=True, unlike the surrounding logging. In CI/non-interactive runs stdout can be buffered, so this message may appear late or be missing if the process is terminated; please flush for consistent observability.
print("[blast] blastp reported an error despite exiting zero; treating as a failure")
return BlastResult(returncode=1, stdout=result.stdout, stderr=result.stderr)
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.
Summary
mainafter Make the blast timeout a real bound, and stop treating an error page as a finished search #107, keeping both the www timeout/error-page fixes and this PR'sblastp -remoteexit-zero detection.restore-keysthat never matched (snakemake-conda-vsconda-) and would only download a stale cache to delete it.blastp -remotestderrError:as a failure even when the process exits 0, so the word-size backoff actually runs.Original work by @mrubash1 in #108.
Test plan
Made with Cursor