Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Download: Add docker prefix for absolute container URIs as well. #2576

Merged
merged 2 commits into from
Dec 18, 2023

Conversation

MatthiasZepper
Copy link
Member

@MatthiasZepper MatthiasZepper commented Dec 14, 2023

This PR adds the docker:// prefix to absolute URIs as well.

While this will result in an error for prebuild Singularity images (and was thus once omitted deliberately), practical experience has demonstrated that the conversion of a Docker OCI is more common in nf-core pipelines.

It is a trade-off between Docker and Singularity support, however essentially all container-specifications I could find in custom, local modules refer to Docker images. Therefore, this change should increase the number of "just works" downloads.

It is also accompanied by two more tests that pull a tiny 3.5MB Docker OCI and convert it with singularity build.

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

Copy link

codecov bot commented Dec 15, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (4300381) 75.05% compared to head (f031814) 75.07%.
Report is 14 commits behind head on dev.

❗ Current head f031814 differs from pull request most recent head 0241790. Consider uploading reports for the commit 0241790 to get more accurate results

Files Patch % Lines
nf_core/download.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #2576      +/-   ##
==========================================
+ Coverage   75.05%   75.07%   +0.01%     
==========================================
  Files          85       85              
  Lines        9374     9367       -7     
==========================================
- Hits         7036     7032       -4     
+ Misses       2338     2335       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ewels
Copy link
Member

ewels commented Dec 15, 2023

While this will result in an error for prebuild Singularity images (and was thus once omitted deliberately)

Can you give an example of what you mean here?

@MatthiasZepper
Copy link
Member Author

MatthiasZepper commented Dec 15, 2023

While this will result in an error for prebuild Singularity images (and was thus once omitted deliberately)

Can you give an example of what you mean here?

What I meant was, that the vast majority of containers that nf-core download will need to get fall within the realm of Bioconda and will be downloaded via http:// or may be pulled and converted to SIF from Dockers OCI format. For those, we have the library flag -l and we can account for different registries like quay.io.

For the few cases with absolute URIs, I didn't want to decide for the pipeline developer whether they want to directly pull a Singularity image with the shub:// protocol or use the docker:// prefix for OCI conversion. I expected them to provide the correct protocol if they insist on specifying an absolute URI of a custom image against nf-core guidelines. However, practise has shown that nobody did, so those containers did not download successfully.

Ultimately, I will probably have to write a test that runs nf-core download when a PR to master indicates an imminent pipeline release, so a pipeline maintainer can see if their new pipeline version will download correctly or not.

@MatthiasZepper MatthiasZepper added this to the 2.11 milestone Dec 18, 2023
Copy link
Member

@mirpedrol mirpedrol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@MatthiasZepper MatthiasZepper merged commit 056036a into nf-core:dev Dec 18, 2023
19 checks passed
@MatthiasZepper MatthiasZepper deleted the Download_Docker_prefix branch December 18, 2023 17:34
@edmundmiller edmundmiller added the download nf-core download label Jun 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
download nf-core download
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants