-
Notifications
You must be signed in to change notification settings - Fork 192
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: Seqera containers Patch 2 #3293
Merged
MatthiasZepper
merged 11 commits into
nf-core:dev
from
MatthiasZepper:seqera_containers_patch2
Dec 2, 2024
Merged
Download: Seqera containers Patch 2 #3293
MatthiasZepper
merged 11 commits into
nf-core:dev
from
MatthiasZepper:seqera_containers_patch2
Dec 2, 2024
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 was referenced Nov 22, 2024
MatthiasZepper
force-pushed
the
seqera_containers_patch2
branch
from
November 26, 2024 13:52
c4943b2
to
2eca65c
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files☔ View full report in Codecov by Sentry. |
…wise a race condition emerges.
MatthiasZepper
force-pushed
the
seqera_containers_patch2
branch
from
November 29, 2024 12:09
2eca65c
to
66ffaf1
Compare
mashehu
reviewed
Dec 2, 2024
mashehu
reviewed
Dec 2, 2024
mashehu
approved these changes
Dec 2, 2024
mirpedrol
approved these changes
Dec 2, 2024
Co-authored-by: Matthias Hörtenhuber <[email protected]>
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.
The uncoordinated adoption of Seqera Containers in modules before the associated YAML could be devised and supported by tools has broken 'nf-core download' (#3179). #3182 topped up the associated test to make pipeline developers aware, so they could patch the modules with Biocontainer URIs to restore offline support.
With #3244 (Patch 1), I added minimal support for Seqera Containers, but had to include a bypass in the prioritization function to support the Seqera Container's odd
http://
paths, that directly deeplink into the last layer of the Docker OCI images.This change, however, also bypassed the deduplication routine, such that a race condition could arise, if a pipeline used the same container image in multiple modules and the download was performed with multiple processes in parallel. This led to bug #3285 and an issue with the release PR of fastquorum, that is preventable by limiting the number of processes to 1.
This PR (Patch2) now fixes said bug. Additionally, it adds basic support for native Singularity images specified with
oras://
paths, such that the Seqera Containerhttp://
paths can be phased out again.This is desirable, because for every Seqera Container
http://
path module, the associated Docker image is downloaded/converted as well, but never used. For a medium sized pipeline, the tool therefore downloads a dozen or more useless Docker container images and since the conversion to Singularity is slow, this significantly bogs downnf-core download
.The new dedicated Seqera Container consolidation functionality is unfortunately flaky and may be too aggressive, in case multiple revisions of a pipeline are downloaded at the same time. While this is far from ideal, I prefer that issue over a pile of unused containers being downloaded each time. Only more verbose container URIs for Seqera Containers could prevent this from happening, but I think the changes to see that implemented are low.
Ultimately, we should either stick with Bioconda or implement the new container YAML format.
PR checklist
CHANGELOG.md
is updated