Skip to content

Download: Trim explicit registries#2403

Merged
MatthiasZepper merged 7 commits into
nf-core:devfrom
MatthiasZepper:DockerRegistryBug
Sep 1, 2023
Merged

Download: Trim explicit registries#2403
MatthiasZepper merged 7 commits into
nf-core:devfrom
MatthiasZepper:DockerRegistryBug

Conversation

@MatthiasZepper

@MatthiasZepper MatthiasZepper commented Aug 18, 2023

Copy link
Copy Markdown
Member

Issue

Some pipelines respectively modules use full-length URIs such as quay.io/qiime2/core:2022.11 when referencing the Docker container image.

Such URIs were not properly accounted for when the new -l / --container-library feature for tools 2.9 was devised. This prevented the user from successfully pulling the images, because the registry was duplicated in the final URI as is exemplified with nf-core download ampliseq -r '2.6.1' -s 'singularity' -t:

INFO     Pulling of "quay.io/qiime2/core:2022.11" failed.
         Please troubleshoot the command
         "singularity pull --name
         /crex/proj/sllstore2017079/private/zepper/nextflow/download_pipelines/ampliseq/singularity-images/quay.io-qii
         me2-core-2022.11.img docker://quay.io/quay.io/qiime2/core:2022.11" manually.

Approach

In general, three possible solutions to this problem exist:

  1. Ignore the provided -l / --container-library arguments for full length URIs.
  2. Exclusively considering the provided -l / --container-library arguments while ignoring any module-specified registry.
  3. Trimming the registry from the module, but adding it to the list of -l / --container-library provided ones.

All in all, option 2 was chosen.

If changes were required after pipeline publication, for example due to the migration to another registry, option 1 would have prevented any. Option 3 would have posed the challenge of interfering with the items to loop over during the loop execution itself, with merely questionable benefits for the UX. Although it would have allowed for more instant successful downloads than any other approach, silently adding another container registry would also have been dangerous, as it would have enabled URI hijacking for other modules in a pipeline.

Therefore, option 2 was ultimately implemented. It eliminates a possible registry automatically to avoid duplications and fixes most download issues, since far the most explicit registries in nf-core modules are quay.io and docker.io. An INFO message with the module-specified registry is printed to the screen, so that a user can retry the download with said registry as explicit argument.

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 Bot commented Aug 21, 2023 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #2403 (7573c90) into dev (ba395fe) will decrease coverage by 0.17%.
The diff coverage is 20.83%.

@@            Coverage Diff             @@
##              dev    #2403      +/-   ##
==========================================
- Coverage   71.23%   71.07%   -0.17%     
==========================================
  Files          87       87              
  Lines        9415     9431      +16     
==========================================
- Hits         6707     6703       -4     
- Misses       2708     2728      +20
Files Changed Coverage Δ
nf_core/download.py 59.35% <20.83%> (-0.86%) ⬇️

... and 1 file with indirect coverage changes

@adamrtalbot

adamrtalbot commented Aug 22, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

docker pull uses option 1. Do we want to be consistent with that or do our own thing? I agree 3 is rubbish and this might be the best option.

Also, can we mix and match registries? [edit] Oh I see, by running twice.

@MatthiasZepper

MatthiasZepper commented Aug 22, 2023

Copy link
Copy Markdown
Member Author

What do you mean by mix and match registries?

An absolute URI is not automatically tried (unless that registry was anyway specified with e.g. -l docker.io), but a message is generated with the suggestion to add that registry for a future, second run. An alternative would be to try the original registry first and then use the truncated version later. If you anyway plan to refactor and modularize the code, then this might be an option.

@adamrtalbot

adamrtalbot commented Aug 30, 2023

Copy link
Copy Markdown
Contributor

I would personally prefer option 1 because this is consistent with the docker API, but then again I don't use Singularity. The code itself looks fine.

@MatthiasZepper

MatthiasZepper commented Aug 30, 2023

Copy link
Copy Markdown
Member Author

@mirpedrol @mashehu - do you have a preference for either of the two?

@mirpedrol

mirpedrol commented Aug 30, 2023

Copy link
Copy Markdown
Member

Option 1 looks more intuitive to me, modules with absolute URIs will usually be from a registry different from quai.io (at least in nf-core modules).
Do you think the case of a registry change will happen often?

@adamrtalbot adamrtalbot self-requested a review August 31, 2023 11:21
@MatthiasZepper

MatthiasZepper commented Aug 31, 2023

Copy link
Copy Markdown
Member Author

So, I have now implemented the requested changes to nf-core download. Now absolute URIs are just used without further changes or consideration of eventually specified --container-library. My rationale was that the ability to override paths specified in legacy software might be a good thing, but I see how consistency with other applications also matters for intuitive usability.

Additionally, I have also added the CHANGELOG.md to ignored paths in the Pytest Github Action, so they are not always run. While rebasing, I realized that the Actions have been refactored quite a lot, since I initially created my feature branch, so it might be that there are more actions which also would not need to run for simple Changelog changes. But since I do not know the rationale and purpose of the new workflows, I will leave them as they are.

@adamrtalbot

adamrtalbot commented Aug 31, 2023

Copy link
Copy Markdown
Contributor

Nice work (again!)

mirpedrol
mirpedrol approved these changes Sep 1, 2023
View reviewed changes

@mirpedrol mirpedrol left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM! Thank you!

Comment thread CHANGELOG.md Outdated
@MatthiasZepper @mirpedrol
Undo prettier's sins in CHANGELOG.md
7573c90 
Co-authored-by: Júlia Mir Pedrol <mirp.julia@gmail.com>
@MatthiasZepper MatthiasZepper merged commit f71a22a into nf-core:dev Sep 1, 2023
@MatthiasZepper MatthiasZepper deleted the DockerRegistryBug branch September 1, 2023 18:24
@MatthiasZepper MatthiasZepper mentioned this pull request Feb 14, 2024
Closed
@MatthiasZepper MatthiasZepper mentioned this pull request Jul 14, 2025
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mirpedrol mirpedrol mirpedrol approved these changes

@adamrtalbot adamrtalbot adamrtalbot approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@MatthiasZepper @adamrtalbot @mirpedrol