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.

Sign up for GitHub

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

Jump to bottom

Using process.containerOptions instead of docker.runOptions #1431

Merged
merged 7 commits into from
Mar 8, 2024
Merged

Using process.containerOptions instead of docker.runOptions #1431

merged 7 commits into from
Mar 8, 2024

Conversation

asp8200
Copy link
Contributor

@asp8200 asp8200 commented Mar 7, 2024 •
edited
Loading

Using process.containerOptions instead of docker.runOptions.

Clearing the containerOptions for the SPARK modules - regardless of which container engine is used. (It should be no problem for Singularity, but unclear whether it is a problem for kubernetes.

I ran the following tests of Singularity+Spark locally:

nextflow run main.nf -profile test_cache,use_gatk_spark,singularity --outdir results
nextflow run main.nf -profile test_cache,use_gatk_spark,singularity --skip_tools fastqc,markduplicates_report,mosdepth,multiqc,samtools --outdir results
nextflow run main.nf -profile test_cache,use_gatk_spark,singularity --save_output_as_bam --outdir results

@github-actions GitHub Actions
Copy link

github-actions bot commented Mar 7, 2024 •
edited
Loading

nf-core lint overall result: Passed ✅ ⚠️

Posted for pipeline commit 1c35f21

+| ✅ 181 tests passed       |+
#| ❔  14 tests were ignored |#
!| ❗   4 tests had warnings |!

❗ Test warnings:

  • pipeline_todos - TODO string in main.nf: Optionally add in-text citation tools to this list.
  • pipeline_todos - TODO string in main.nf: Optionally add bibliographic entries to this list.
  • pipeline_todos - TODO string in main.nf: Only uncomment below if logic in toolCitationText/toolBibliographyText has been filled!
  • pipeline_todos - TODO string in WorkflowSarek.groovy: Optionally add in-text citation tools to this list.

❔ Tests ignored:

  • files_exist - File is ignored: .github/workflows/awsfulltest.yml
  • files_exist - File is ignored: .github/workflows/awstest.yml
  • files_exist - File is ignored: conf/modules.config
  • files_exist - File is ignored: lib/WorkflowMain.groovy
  • files_exist - File is ignored: lib/NfcoreTemplate.groovy
  • files_exist - File is ignored: lib/WorkflowSarek.groovy
  • files_unchanged - File ignored due to lint config: .github/PULL_REQUEST_TEMPLATE.md
  • files_unchanged - File ignored due to lint config: assets/nf-core-sarek_logo_light.png
  • files_unchanged - File ignored due to lint config: docs/images/nf-core-sarek_logo_light.png
  • files_unchanged - File ignored due to lint config: docs/images/nf-core-sarek_logo_dark.png
  • files_unchanged - File ignored due to lint config: .gitignore or .prettierignore or pyproject.toml
  • actions_ci - actions_ci
  • actions_awstest - 'awstest.yml' workflow not found: /home/runner/work/sarek/sarek/.github/workflows/awstest.yml
  • template_strings - template_strings

✅ Tests passed:

Run details

  • nf-core/tools version 2.13.1
  • Run at 2024-03-08 16:43:06

@kenibrewer
Copy link
Member

I think introducing a new parameter params.docker_enabled has the potential to introduce a lot of confusion to users of the workflow especially ones who aren't super experienced in Nextflow. If I set params.docker_enabled = true than I would expect that I don't need to add -profile docker as well.

I think the second solution you proposed adheres better to expected usage patterns and the separation of scientific logic and configuration. It would be possible to implement those changes via a module patch right?

Also, am I correct in assuming that the goal of this PR is to address #1406 , #1404 , and #1362 ?

@asp8200
Copy link
Contributor Author

asp8200 commented Mar 7, 2024 •
edited
Loading

I think introducing a new parameter params.docker_enabled has the potential to introduce a lot of confusion to users of the workflow especially ones who aren't super experienced in Nextflow. If I set params.docker_enabled = true than I would expect that I don't need to add -profile docker as well.

I think the second solution you proposed adheres better to expected usage patterns and the separation of scientific logic and configuration. It would be possible to implement those changes via a module patch right?

Also, am I correct in assuming that the goal of this PR is to address #1406 , #1404 , and #1362 ?

I also don't like the introduction of the new parameter params.docker_enabled. I'm kind of hoping somebody will explain me how to access docker.enabled or workflow.containerEngine in this kind of if-clause in a config-file, but perhaps it is just not possible? 🤔

As you mention, we can also decide to go with this solution and make a module patch.

@kenibrewer
Copy link
Member

I think you might be having trouble accessing that config variable because of the hierarchy that configs are resolved in (https://www.nextflow.io/docs/latest/config.html#configuration-file). At the moment in time when this module specific configuration is resolved, docker.enabled hasn't been set yet.

@asp8200
Copy link
Contributor Author

asp8200 commented Mar 7, 2024 •
edited
Loading

I think you might be having trouble accessing that config variable because of the hierarchy that configs are resolved in (https://www.nextflow.io/docs/latest/config.html#configuration-file). At the moment in time when this module specific configuration is resolved, docker.enabled hasn't been set yet.

Hmmm ... That might be so 🤔 However, I see in the case of profile=docker we have docker.enabled set to true in line 189 of nextflow.config while, for instance, markduplicates.config is being imported in line 370 of nextflow.config, so I would expect docker.enabled to be set to true when markduplicates.config is being parsed/resolved.

@kenibrewer
Copy link
Member

kenibrewer commented Mar 7, 2024 •
edited
Loading

Hmmm ... That might be so 🤔 However, I see in the case of profile=docker we have docker.enabled set to true in line 189 of nextflow.config while, for instance, markduplicates.config is being imported in line 370 of nextflow.config, so I would expect docker.enabled to be set to true when markduplicates.config is being parsed/resolved.

docker.enabled isn't actually set to true on line 189. That line is part of defining a profile but doesn't actually set any configuration on its own. I think that's an important distinction. I'm not sure exactly at what stage in the config resolution process the config from-profile docker gets applied, but it seems pretty likely to me that this config resolution timeline is the root cause.

@asp8200
Copy link
Contributor Author

asp8200 commented Mar 7, 2024

Hmmm ... That might be so 🤔 However, I see in the case of profile=docker we have docker.enabled set to true in line 189 of nextflow.config while, for instance, markduplicates.config is being imported in line 370 of nextflow.config, so I would expect docker.enabled to be set to true when markduplicates.config is being parsed/resolved.

docker.enabled isn't actually set to true on line 189. That line is part of defining a profile but doesn't actually set any configuration on its own. I think that's an important distinction. I'm not sure exactly at what stage in the config resolution process the config from-profile docker gets applied, but it seems pretty likely to me that this config resolution timeline is the root cause.

But params.docker_enabled has been set to true by the profile when markduplicates.config is passed/resolved 🤔

@asp8200
Copy link
Contributor Author

asp8200 commented Mar 7, 2024

I should add that workflow.containerEngine is accessible in the config in the following kind of conditional:

withName: 'GATK4SPARK_MARKDUPLICATES' {
    containerOptions = { (workflow.containerEngine == 'docker') ? '' : '<what_to_do_here?>' }

In the case workflow.containerEngine != 'docker', I don't want to change the containerOptions. How can that be achieved? 🤔

I don't suppose something like the following would work:

    containerOptions = { (workflow.containerEngine == 'docker') ? '' : containerOptions  }

@asp8200
Getting rid of the dummy variable params.docker_enabled. Clearing con…
3386da7 
…tainerOptions for SPARK-modules for any kind of container engine.
maxulysse
maxulysse reviewed Mar 8, 2024
View reviewed changes
Copy link
Member

@maxulysse maxulysse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking food

@asp8200
Copy link
Contributor Author

asp8200 commented Mar 8, 2024

looking food

Are you also looking for lunch? I haven't had my lunch yet.

@kenibrewer
Copy link
Member

Sorry I was off base on the config resolution. Good investigation though!

@asp8200
Copy link
Contributor Author

asp8200 commented Mar 8, 2024

Sorry I was off base on the config resolution. Good investigation though!

Thanks for joining in on the discussion, @kenibrewer .

@maxulysse maxulysse marked this pull request as ready for review March 8, 2024 14:15
kenibrewer
kenibrewer previously approved these changes Mar 8, 2024
View reviewed changes
maxulysse
maxulysse reviewed Mar 8, 2024
View reviewed changes
nextflow.config Outdated Show resolved Hide resolved
FriederikeHanssen
FriederikeHanssen reviewed Mar 8, 2024
View reviewed changes
Copy link
Contributor

@FriederikeHanssen FriederikeHanssen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add some docs crosslinking the warning about kubernetes and GLS?

@maxulysse maxulysse mentioned this pull request Mar 8, 2024
Open
@asp8200 asp8200 merged commit 2e06e65 into nf-core:dev Mar 8, 2024
29 checks passed
@asp8200 asp8200 deleted the fix_docker_runoptions_II branch June 4, 2024 10:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FriederikeHanssen FriederikeHanssen FriederikeHanssen approved these changes

@maxulysse maxulysse maxulysse approved these changes

@kenibrewer kenibrewer kenibrewer left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@asp8200 @kenibrewer @maxulysse @FriederikeHanssen