use md5sum to check download integrity with vdb-validate as fallback

[suhrig]

PR checklist

As explained in ncbi/sra-tools#896, vdb-validate does not detect file corruption if the prefetched files do not contain MD5 checksums. It has happened to me many times that downloaded files turn out to be corrupt, if I use the option force_sratools_download (now --download_method sratools). What is worse is that extracting the files using fasterq-dump does not always result in an error even if the file is corrupt. It is even conceivable that the extracted FastQ file looks perfectly intact with only some bases or quality values being changed. As such, the error may go completely unnoticed.

This PR fixes this by (1) fetching the md5sum from the SRA Data Locator API and (2) performing a manual md5sum check. The current method, vdb-validate is only used anymore if the md5sum cannot be obtained from the API.

[suhrig]

@Midnighter: Do you need any further input from my side before you can start with the review? The pull request is ready for review IMO.

[Midnighter]

Thank you for uncovering these problems and suggesting these changes. I see two main issues with the PR:

1. We also need container images that provide the right dependencies.
2. As you rightfully point out in your comment vdb-validate does not detect file corruption ncbi/sra-tools#896 (comment), it would be ideal to be able to check if md5sums are present before making another network request.

[suhrig]

Thanks for all the useful feedback!

it would be ideal to be able to check if md5sums are present before making another network request.

I will nudge the developers once more about this. I agree this would be ideal.

[suhrig]

Hi @Midnighter,

I have implemented your requested changes.

We also need container images that provide the right dependencies.

I no longer need jq, since I use grep to search for the valid checksum, which results in much simpler code than using jq would. So the only additional tool is curl, which is part of the sratools container already. Do we still need an additional container?

As you rightfully point out in your comment ncbi/sra-tools#896 (comment), it would be ideal to be able to check if md5sums are present before making another network request.

It turns out the maintainers had added feature to confirm that the output of vdb-validate is reliable. I am making use of this in my new code submission. curl is only used as fallback now.

[Midnighter]

I no longer need jq, since I use grep to search for the valid checksum, which results in much simpler code than using jq would. So the only additional tool is curl, which is part of the sratools container already. Do we still need an additional container?

No, that's fine then.

Changes look good to me! Really happy that you managed to avoid the curl call when it's not necessary.

My only nitpick is, that I prefer long form options in scripts, e.g., grep --fixed-strings rather than grep -F because it's typically much more clear what the intention is then. I tend to forget myself, what the short form means, so it might also help future you 😉. It's certainly fine to merge like this, too, though!

[suhrig]

long form options in scripts

Sure, I will adapt the script accordingly!

[suhrig]

I have implemented the suggestions. I also added an error message when the manual MD5 sum check failed, because I realized it may be confusing when the process just fails silently.

Do you know why the automated checks won't launch? They have been stuck in "awaiting approval" state from the start. I cannot merge the changes otherwise.

[Midnighter]

Very nice!

[suhrig]

I have satisfied the requirements of the failed editorconfig check. However, as a result of this the auto-merge feature was disabled and the PR is stuck in awaiting approval again. Can you kindly explain what you did to fix this so I can help myself in the future?

[Midnighter]

You need to have certain permissions on the repository. I'm not exactly sure which role. Probably at least write permissions.

[suhrig]

Hi @Midnighter. Sorry to bother you again. There was a hidden failed test which I did not notice, because all the ticks in the overview were green. A few MD5 sums of files which nf-test checked for integrity needed updating.

As a result of my new commit, the checks are in pending state again. :-/ I don't know why this happens or how to fix this. So I have to ask you (hopefully only) once more to work your magic? Thanks!

Also, GitHub added a new reviewer automatically. I did not do this.

[Midnighter]

No worries, not your fault.

[suhrig]

One more push please? I used long options for grep as you suggested, but not all implementations of grep support them, which is why a test failed. I had to revert to short options for grep, therefore.

[Midnighter]

One more push please? I used long options for grep as you suggested, but not all implementations of grep support them, which is why a test failed. I had to revert to short options for grep, therefore.

It's so annoying when the accepted options for these standard tools are different between Mac's Unix and Linux. Or was it the conda version?

[suhrig]

options for these standard tools are different between Mac's Unix and Linux. Or was it the conda version?

It looks like the busybox implementation is used for the tests.

The tests have completed successfully. 🥳 Do you have permission to merge the changes or shall we wait for the review from @drpatelh? I think GitHub added him as a reviewer when I modified the MD5 sums of the nf-test files, presumably because he is the owner of those files.

[Midnighter]

Strange, I had set it to auto merge already... not sure why that didn't happen.

[suhrig]

Many thanks for all your help!