-
Notifications
You must be signed in to change notification settings - Fork 191
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
Open
MatthiasZepper
wants to merge
9
commits into
nf-core:dev
Choose a base branch
from
MatthiasZepper:seqera_containers_patch2
base: dev
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Download: Seqera containers Patch 2 #3293
MatthiasZepper
wants to merge
9
commits into
nf-core:dev
from
MatthiasZepper:seqera_containers_patch2
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
…wise a race condition emerges.
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. |
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