refactor: Support container_registry in modules

[edmundmiller]

Motivated by: https://aws.amazon.com/blogs/hpc/biocontainers-are-now-available-in-amazon-ecr-public-gallery/

They don't have mulled images being pushed yet however, but this should support other urls as well so it's a nice change.

[edmundmiller]

  Unable to find image 'public.ecr.aws/biocontainers/mulled-v2-ffbf83a6b0ab6ec567a336cf349b80637135bca3:676c5bcfe34af6097728fea60fb7ea83f94a4a5f-0' locally
  docker: Error response from daemon: repository public.ecr.aws/biocontainers/mulled-v2-ffbf83a6b0ab6ec567a336cf349b80637135bca3 not found: name unknown: The repository with name 'mulled-v2-ffbf83a6b0ab6ec567a336cf349b80637135bca3' does not exist in the registry with id 'biocontainers'.
  See 'docker run --help'.

:(

[grst]

Is this accessible from outside aws?

[apeltzer]

When looking at the ECR gallery, @Emiller88 and I were unable to find the mulled containers there - need to ask AWS folks on whether this is not mirrored (yet?).

@delagoya - can you share some insights on this?

[mahesh-panchal]

My suggestion is to use a profiles to set a parameter to supply the container registry and then simplify the ternary expression.

profiles {
    singularity {
         singularity.enabled = true
         params.container_registry = 'https://depot.galaxyproject.org/singularity'
     }
     docker {
         docker.enabled = true
         params.container_registry = 'quay.io/biocontainers'
     }
     aws {
          params.container_registry = 'public.ecr.aws'
     }
}

and then the process tag looks like:

process TOOL_SUBTOOL {
    container "${ (params.container_registry ?: 'quay.io' ) + 'image:version--build' }"
}

[edmundmiller]

My suggestion is to use a profiles to set a parameter to supply the container registry and then simplify the ternary expression.

This is definitely the cleanest. I think it still keeps that the modules "just work" but if they want to support singularity in a non-nf-core pipeline you'll have to add the singularity profile, if you don't want to convert.

[mahesh-panchal]

Also one can just override from the command-line or -params-file if they want to use the docker registry from singularity or use a private registry.

[edmundmiller]

Also one can just override from the command-line or -params-file if they want to use the docker registry from singularity or use a private registry.

Great point! I think that solves it.

[edmundmiller]

@mahesh-panchal Thoughts on:

"${ ... }"

vs. just a closure? {}

[mahesh-panchal]

@mahesh-panchal Thoughts on:

"${ ... }"

vs. just a closure? {}

Container needs a string in this context I think.

[edmundmiller]

Container needs a string in this context I think.

https://github.com/nf-core/modules/pull/2291/commits/98f8156fb1171cc9bb0b585516db6cc15d5c2a54

This works :shrug:

[mahesh-panchal]

If the string quotes are not needed, then you don't need the { } either.

container (params.container_registry ?: 'quay.io' ) + 'mulled-v2-ffbf83a6b0ab6ec567a336cf349b80637135bca3:676c5bcfe34af6097728fea60fb7ea83f94a4a5f-0' 

[edmundmiller]

If the string quotes are not needed, then you don't need the { } either.

No dice

Unknown process directive: `plus`

Did you mean of these?
        cpus

 -- Check script 'tests/modules/nf-core/bowtie/align/../../../../../modules/nf-core/bowtie/align/main.nf' at line: 6 or see '.nextflow.log' file for more details

[edmundmiller]

    def image = "mulled-v2-ffbf83a6b0ab6ec567a336cf349b80637135bca3:676c5bcfe34af6097728fea60fb7ea83f94a4a5f-0"
    conda (params.enable_conda ? 'bioconda::bowtie=1.3.0 bioconda::samtools=1.15.1' : null)
    container { (params.container_registry ?: 'quay.io' ) + image }

Thoughts? I think this is clean.

[mahesh-panchal]

Maybe then

If the string quotes are not needed, then you don't need the { } either.

No dice

Unknown process directive: `plus`

Did you mean of these?
        cpus

 -- Check script 'tests/modules/nf-core/bowtie/align/../../../../../modules/nf-core/bowtie/align/main.nf' at line: 6 or see '.nextflow.log' file for more details

Hmm. Maybe then just ( ) is needed instead of { }. This may be a parsing context thing. I don't see why a closure is needed here.

[grst]

If the string quotes are not needed, then you don't need the { } either.

I think one of the two is needed when using custom config files with -c custom.config to delay the evaluation until the params are ready. But I heard -c is discouraged nowadays anyway? And is there any difference between "${}" compared to { } as far as the effect is concerned?

[mahesh-panchal]

    def image = "mulled-v2-ffbf83a6b0ab6ec567a336cf349b80637135bca3:676c5bcfe34af6097728fea60fb7ea83f94a4a5f-0"
    conda (params.enable_conda ? 'bioconda::bowtie=1.3.0 bioconda::samtools=1.15.1' : null)
    container { (params.container_registry ?: 'quay.io' ) + image }

Thoughts? I think this is clean.

I'm alright with it. The issue now is to think what this breaks. like pre-image download in nf-core tools.

[apeltzer]

Opened an issue here too - it will also "break" e.g. nf-core modules create in terms of that the biocontainers API will return a queried string with a full path to the container to use --> we can fix that to make it conveniently split base_url and the image id matching what is implemented in this PR, but should not forget to do so once this is merged ;-) -- see: https://github.com/nf-core/tools/issues/1948

[grst]

container { (params.container_registry ?: 'quay.io' ) + image }

Slash after quay.io missing?

[edmundmiller]

container { (params.container_registry ?: 'quay.io' ) + image }

Slash after quay.io missing?

It's in the test config for now. Idk which place is better to put it, probably in the module?

Also thoughts on quay.io vs quay.io/biocontainers as a default?

[mahesh-panchal]

If the string quotes are not needed, then you don't need the { } either.

I think one of the two is needed when using custom config files with -c custom.config to delay the evaluation until the params are ready. But I heard -c is discouraged nowadays anyway? And is there any difference between "${}" compared to { } as far as the effect is concerned?

The "${ }" is a groovy expression inside a GString, { } is a closure/anonymous function (and in the context above a function that's just returning a string). The deferring of evaluation is only relevant from configuration files, so since this is from the script context, it does not have an effect.

[mahesh-panchal]

Also thoughts on quay.io vs quay.io/biocontainers as a default?

The default should be whatever makes a full path. I didn't check it, so I guess it should be quay.io/biocontainers

[mahesh-panchal]

It's in the test config for now. Idk which place is better to put it, probably in the module?

I think the best place is preceding the image name in the module. Most things can handle double slash, but break if it's not there.

[edmundmiller]

@mahesh-panchal I think this is good: https://github.com/nf-core/modules/commit/0e40ada55d714fee5f366bf4cdbb7cdbaae4f028

[edmundmiller]

Thoughts on reorging the conda and containers?

    def image = "/mulled-v2-ffbf83a6b0ab6ec567a336cf349b80637135bca3:676c5bcfe34af6097728fea60fb7ea83f94a4a5f-0"
    container { (params.container_registry ?: 'quay.io/biocontainers') + image }
    conda (params.enable_conda ? 'bioconda::bowtie=1.3.0 bioconda::samtools=1.15.1' : null)

It's not alphabetical :upside_down_face:

[edmundmiller]

Also parens work:

    container ((params.container_registry ?: 'quay.io/biocontainers') + image)

[mahesh-panchal]

Thoughts on reorging the conda and containers?

    def image = "/mulled-v2-ffbf83a6b0ab6ec567a336cf349b80637135bca3:676c5bcfe34af6097728fea60fb7ea83f94a4a5f-0"
    container { (params.container_registry ?: 'quay.io/biocontainers') + image }
    conda (params.enable_conda ? 'bioconda::bowtie=1.3.0 bioconda::samtools=1.15.1' : null)

It's not alphabetical 🙃

Works for me. Better to keep related parts together.

[mahesh-panchal]

Would it make adjusting the tooling easier if the code were:

    def container_image = "mulled-v2-ffbf83a6b0ab6ec567a336cf349b80637135bca3:676c5bcfe34af6097728fea60fb7ea83f94a4a5f-0"
    container [ params.container_registry ?: 'quay.io/biocontainers' , container_image ].join('/')
    conda (params.enable_conda ? 'bioconda::bowtie=1.3.0 bioconda::samtools=1.15.1' : null)

[edmundmiller]

Ooo I like that. I wonder if we can get https://docs.renovatebot.com/ working and drown ourselves in automated image update PRs (we could set them to automerge if the tests pass and it's not a major version bump)

[apeltzer]

If we do this change entirely, that means also everyone has to use a separate singularity profile to keep pipelines from converting docker -> singularity images. Something to document to tell users upfront what is happening as well as to bear in mind for potential crashes when some (unaware) users now have to rely on the docker->singularity path again. Had quite a number of people with e.g. squashfs not installed issues in the past 👍🏻

Otherwise looking good to me in general (didn't go through 600+ modules).

[edmundmiller]

They could just manually set the container image if they wanted the docker image right?