nf-core / hlatyping

Precision HLA typing from next-generation sequencing data
https://nf-co.re/hlatyping
MIT License
62 stars 30 forks source link

Container image not pulled from shub on profile `singularity` #12

Closed sven1103 closed 6 years ago

sven1103 commented 6 years ago

Running the command

nextflow run nf-core/hlatyping -profile singularity,test --outdir $PWD/results

locally, does not pull the container from Singularity Hub, but Docker Hub and result in extensive CPU usage in order to create the sqash Filesystem.

Expected behaviour: Given the profile singularity, the container should be pulled from Singularity Hub directly.

sven1103 commented 6 years ago

It seems that defining process.container = 'shub://nf-core/hlatyping' int the ./conf/singularity.config does not have an effect. However, when changing this to params.container = 'shub://nf-core/hlatyping', the Singularity image is pulled from Singularity Hub, as expected.

apeltzer commented 6 years ago

Had to look it up too, but I guess thats the reason: https://www.nextflow.io/docs/latest/config.html#configuration-file

nextflow.config will be loaded first => as this already defines the container variable, this won't get overriden by singularity.conf if I'm not mistaken. The difference is between params and process: The params are depending on the order of the profiles imported, whereas the scopes are already set in the nextflow.config of hlatyping and cannot be overriden that easily.

Still having fever, so might not be correct what I'm thinking out loud here...

apeltzer commented 6 years ago

I remember that Phil and me already had some issues to setting things right when coming up with this "link pipeline to container" thing...

ewels commented 6 years ago

I think this is because the test profile loads the base.config again, which sets process.container. So this overwrites what was initially set in singularity.config.

Using -profile test,singularity may work, as the singularity profile is applied second. However, then base.config will be applied on top of the test profile again and will probably break stuff.

This is one of the reasons for keeping the singularity profiles super slim - only enabling singularity / docker / conda.

sven1103 commented 6 years ago

This is one of the reasons for keeping the singularity profiles super slim - only enabling singularity / docker / conda.

True, but using the -profile singularity option should pull from Singularity Hub imho, and not go the expensive way over Docker Hub. This is especially important, if people use the singularity profile on HPC and launch Nextflow from the head/login node. Don't want to let squashfs affect the performance of this node...

Any opinions here?

ewels commented 6 years ago

Yup, agreed..

sven1103 commented 6 years ago

Now we just need a way to properly solve it :D Defining params.container again in the singularity.config works, but I am unsure, if this is the best way to go

ewels commented 6 years ago

I can't remember why we set params.container now. Any reason why we can't just set process.container separately and explicitly in the docker and singularity configs?

I don't think that we can have -with-docker and -with-singularity work without arguments if we want separate defaults. But maybe that's fine...

sven1103 commented 6 years ago

I can't remember why we set params.container now. Any reason why we can't just set process.container separately and explicitly in the docker and singularity configs?

Me neither. I don't see why this should be an issue, if people load the profiles.

I don't think that we can have -with-docker and -with-singularity work without arguments if we want separate defaults.

I would see this option in the case of manual deployment of a container (no internet connection, for example). Then the user has to provide the proper container, and they can do so with the nf-core tools download option anyway.

sven1103 commented 6 years ago

solved in #15

ewels commented 6 years ago

It is? I can't see what you changed there in relation to this issue..?