Download: Improved container priority and resolution

[MatthiasZepper]

This PR improves the code in Download that detects and resolves container images in modules and configs.

Previously, a loop was used to prioritize http:// downloads over docker URIs. For example, from

container "${ workflow.containerEngine == 'singularity' && !task.ext.singularity_pull_docker_container ?
   'https://depot.galaxyproject.org/singularity/fastqc:0.11.9--0' :
   'biocontainers/fastqc:0.11.9--0' }"

the strings 'singularity', 'https://depot.galaxyproject.org/singularity/fastqc:0.11.9--0' and 'biocontainers/fastqc:0.11.9--0' were extracted into a list. Then we iterated over this list and broke the loop as soon as a match with a URL regex or Docker regex was found.

This approach was flaky, because 'singularity' or 'apptainer' are valid Docker URIs. Thus, unless it was explicitly ignored as a false positive, it prevented further evaluation and detection of the real image paths. Now, evaluation continues in any case, which should make it more resilient for further syntax updates. Furthermore, the old approach also relied on the convention that the URL would always be listed before the Docker URI.

The old approach failed entirely for early DSL2, where a separate container declaration was used:

  if (workflow.containerEngine == 'singularity' && !params.singularity_pull_docker_container) {
      container "https://depot.galaxyproject.org/singularity/fastqc:0.11.9--0"
  } else {
      container "quay.io/biocontainers/fastqc:0.11.9--0"
  }

For each container declaration, a new loop was created, such that it was not possible to prioritize the direct download over the Docker URI for early DSL2. Therefore, each image was once downloaded and once pulled.

This PR now changes the approach to first gather all Docker URIs and download URLs from all modules and configs into a joint, global list. Subsequently, a new helper function prioritize_direct_download() removes all Docker URIs from the list for which also direct download URLs are contained. Consolidating container URIs/URLs from different modules is now possible for the first time.

In addition to the improved function, a new test has been devised that runs on mock modules in the tests/data directory. So any future module standard should be reflected by a mock module in that folder in the future.

PR checklist

• This comment contains a description of changes (with reason)
• CHANGELOG.md is updated
• If you've fixed a bug or added code that should be tested, add tests!
• Documentation in docs is updated

[codecov]

Codecov Report

Merging #2364 (e459c66) into dev (d21770d) will increase coverage by 0.04%.
The diff coverage is 97.14%.

@@            Coverage Diff             @@
##              dev    #2364      +/-   ##
==========================================
+ Coverage   72.76%   72.80%   +0.04%     
==========================================
  Files          78       78              
  Lines        8889     8888       -1     
==========================================
+ Hits         6468     6471       +3     
+ Misses       2421     2417       -4     

Impacted Files	Coverage Δ
nf_core/download.py	60.21% <97.14%> (+0.73%)	⬆️

... and 2 files with indirect coverage changes

[mirpedrol]

LGTM, only some small comments

[mirpedrol]

Just so I understand, what's the difference between cleaned_matches and valid_containers? Are they obtained with the same regex?

[MatthiasZepper]

cleaned matches is the list (line 784) that collects all valid_containers from the loop over the raw findings (line 796).

[mirpedrol]

Should this be warning? It doesn't stop execution, no?

[MatthiasZepper]

Indeed. Until 2.8, the download would exit and fail if a container image download failed. But that meant that pipelines could not be downloaded with container images, if just one image failed for whatever reason. I briefly discussed this with mashehu, and we agreed that the friendlier user experience would be to continue the download.

Like so, the user only has to manually troubleshoot one image download, in contrast to somehow obtaining 40 images or so one by one. Therefore, this changed behaviour was implemented with release 2.9.

[mirpedrol]

Can we add a message? For example:

Suggested change

	log.info(c)
	log.info(f"Evaluating container {c}")

[MatthiasZepper]

Ehm...no, because that log.info() statement was never meant to stay in there, as it just clutters the output. Excellent catch, I have deleted it now!

That was a leftover of a debugging process, when I failed to understand why certain images showed up in the container_list even if they were not submitted to the function. This happened because the function retained the state from a previous execution, due to my oversight regarding the distinction between Class and Static methods. I assumed the behaviour of a Static method without explicitly defining it as one.)

[mirpedrol]

This test will be very useful, thanks!

[MatthiasZepper]

You're welcome!

It will benefit me as well, as I won't be surprised by a sudden new module syntax. Once we settle on a new template syntax and modify the modules create and lint accordingly, we can also directly include one in this test to ensure that downloads can also resolve the container paths accurately.