Update module sratools prefetch #305
Merged
Conversation
This file contains 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
This PR is against the
|
|
maxulysse
reviewed
Apr 8, 2024
subworkflows/nf-core/fastq_download_prefetch_fasterqdump_sratools/tests/main.nf.test.snap
Outdated
Show resolved
Hide resolved
…ols/tests/main.nf.test.snap
|
Hi @maxulysse, can I help you with the additional checks? All I can say is I have run this several times now without any issues. Regards, Sebastian |
maxulysse
reviewed
May 6, 2024
Comment on lines
+50
to
+59
| # check file integrity using vdb-validate or (when archive contains no checksums) md5sum | ||
| vdb-validate !{id} > vdb-validate_result.txt 2>&1 || exit 1 | ||
| if grep -q "checksums missing" vdb-validate_result.txt; then | ||
| VALID_MD5SUMS=$(curl --silent --fail --location --retry 3 --retry-delay 60 'https://locate.ncbi.nlm.nih.gov/sdl/2/retrieve?filetype=run&acc=!{id}') | ||
| LOCAL_MD5SUMS=$(md5sum !{id}/* | cut -f1 -d' ') | ||
| if ! grep -q -F -f <(echo "$LOCAL_MD5SUMS") <(echo "$VALID_MD5SUMS"); then | ||
| echo "MD5 sum check failed" 1>&2 | ||
| exit 1 | ||
| fi | ||
| fi |
There was a problem hiding this comment.
Only issue I can see is that we're no longer doing vdb-validate over !{id.}sralite.
But I do love the added md5 sum possibility.
Can you keep the sralite part in this logic?
There was a problem hiding this comment.
This code is no longer needed. It was needed in old version of sratools. But the latest version places .sralite files in a subdirectory named after the SRA ID just like regular .sra files. prefetch dowloads the dataset which gave rise to this code just fine (SRR1806585 , see issue #162) and vdb-validate checks it properly. fasterq-dump still fails at a later step as mentioned in issue #162, but that's because it uses an outdated version of sratools (2.11). I guess it makes sense to update fasterq-dump as part of this PR as well?
There was a problem hiding this comment.
I see, then we're good into merging this PR.
I'd rather we don't update fasterq-dump for now as it would be re-creating other issues: #221
Midnighter
approved these changes
May 6, 2024
There was a problem hiding this comment.
From my side it looks good, but @maxulysse 's comment is a good question, of course.
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 PR fixes #285. Downloads using sratools often resulted in corrupt files, because the tool to check the file integrity,
vdb-validateis not always reliable. This PR updates the modulesratools/prefetch, which includes a fix which performs a manual MD5 sum check in cases wherevdb-validateis unreliable.PR checklist
nf-core lint).nextflow run . -profile test,docker --outdir <OUTDIR>).nextflow run . -profile debug,test,docker --outdir <OUTDIR>).CHANGELOG.mdis updated.