Skip to content
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

Seqera containers: POC 4 #46

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

ewels
Copy link
Member

@ewels ewels commented Jun 17, 2024

Seqera Containers: Proof of concept No. 4

Hardcode container URIs at module level - in a config file

Note

Only works here because this pipeline is already importing the module-level nextflow.config files here:

includeConfig '../../modules/local/seqera_runs_dump/nextflow.config'
includeConfig '../../modules/local/plot_run_gantt/nextflow.config'
includeConfig '../../modules/nf-core/multiqc/nextflow.config'

Flow is as follows:

  1. nextflow.config
  2. imports workflows/nf_aggregate/nextflow.config
  3. imports modules/***/***/nextflow.config

Workflow for generating:

  • Keep conda spec in modules up to date
  • When updating modules in the modules repo run nextflow inspect commands to regenerate module-level config files with pinned container names
  • When adding a shared module to an nf-core pipeline, automatically add includeConfig statements for module-level config file.

Workflow for usage: (same as current except different profile names)

  • Online
    • Use nextflow run -profile docker or docker,docker_arm, singularity,singularity_arm
  • Offline
    • Use nf-core download as done currently, may need some kind of profile change, not sure.

Put the config into separated files at module level to keep them out of the way.

@ewels ewels mentioned this pull request Jun 17, 2024
@bentsherman
Copy link
Member

Perfect 👌

@ewels ewels mentioned this pull request Jun 18, 2024
@MatthiasZepper
Copy link

MatthiasZepper commented Jul 10, 2024

When preparing my Bytesize talk about nf-core download and pondering about a future integration with Seqera Containers, I independently reached a similar conclusion: The container path should be in a config.

I was more thinking along the line that nf-core download could gather the conda/PyPI definitions from the modules, use the same (Wave?) API that is used for the frontend of Seqera Containers to get the correct container paths, download them and write all URIs into a single config (like POC1) that would need to be applied to the pipeline when running it offline.

This has two advantages:

  • The symlink mess in the $NXF_SINGULARITY_CACHEDIR can be avoided entirely, because nf-core download writes an explicit config with exactly the values it uses as final output names of the containers.
  • Obtaining the container URIs at download-time would not require messing around with profiles, because architecture and container runtime of the destination system are typically known to the downloader and could be provided as CLI arguments.

On the other hand, this also has two downsides:

  • Multi-revision downloads become more complex, since a separate config needs to be managed for each. So when adding a pipeline to Platform, one basically has to specify revision-specific configs. But one could directly consider this when finally implementing revision-specific schema support ;-)
  • Consolidation of multiple configs is not exactly straightforward and having a seperate container image config complicates matters.

Along this line, something puzzles me with your proposed POC here: If one heeds the red Danger box in the Nextflow docs that reads "When using the profiles feature in your config file, do NOT set attributes in the same scope both inside and outside a profiles context", your proposed solution here would basically block the process scope far down in the module specific configs. Therefore, setting further process attributes in parent or custom configs would be conflicting, no?

@bentsherman
Copy link
Member

That danger box is essentially a bug in the config parser that will be fixed by the new (strict) config parser (nextflow-io/nextflow#4744). So we might need to merge that feature first, although to be honest, I have never been able to reproduce that bug consistently, it seems to happen only in very specific cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants