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

Use module config #1112

Closed
wants to merge 7 commits into from
Closed

Use module config #1112

wants to merge 7 commits into from

Conversation

bentsherman
Copy link

This PR refactors the modules config to be local to each module, using nextflow-io/nextflow#4510. So the config for a process now resides in a module.config next to the workflow that calls the process.

Some notes:

  • Definitely needs to be validated to make sure that the various configs are applied with the correct priority. Note that pipeline config overrides workflow config overrides process config, etc.

  • Doesn't really change much if we're putting the process config next to the calling workflow. Most of the modules.config still resides in one big file next to the rnaseq workflow module.

  • Doesn't really solve the problem of if statements in the config file. I believe they were added to avoid the config selector warnings when a process selector points to a process that isn't used in a particular pipeline run.

    Paolo originally proposed a more programmatic syntax in which the module config is just a map and the process invocation must reference. I decided to go with process selectors first because the syntax is familiar and it provides resolution from different scopes (pipeline > workflow > process).

Based on these findings, maybe a better convention would be to put the config for each process in the corresponding process module.

@ewels @drpatelh let me know what you think!

PR checklist

  • Make sure your code lints (nf-core lint).
  • Ensure the test suite passes (nextflow run . -profile test,docker --outdir <OUTDIR>).
  • Usage Documentation in docs/usage.md is updated.
  • Output Documentation in docs/output.md is updated.
  • CHANGELOG.md is updated.
  • README.md is updated (including new tool citations and authors/contributors).

@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 13, 2023 •
edited
Loading

nf-core lint overall result: Passed ✅ ⚠️

Posted for pipeline commit 4448c3d

+| ✅ 170 tests passed       |+
#| ❔   7 tests were ignored |#
!| ❗   7 tests had warnings |!

❗ Test warnings:

  • files_exist - File not found: assets/multiqc_config.yml
  • files_exist - File not found: .github/workflows/awstest.yml
  • files_exist - File not found: .github/workflows/awsfulltest.yml
  • pipeline_todos - TODO string in methods_description_template.yml: #Update the HTML below to your preferred methods description, e.g. add publication citation for this pipeline
  • 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!

❔ Tests ignored:

  • files_exist - File is ignored: conf/modules.config
  • nextflow_config - Config default ignored: params.ribo_database_manifest
  • files_unchanged - File ignored due to lint config: assets/email_template.html
  • files_unchanged - File ignored due to lint config: assets/email_template.txt
  • files_unchanged - File ignored due to lint config: .gitignore or .prettierignore or pyproject.toml
  • actions_awstest - 'awstest.yml' workflow not found: /home/runner/work/rnaseq/rnaseq/.github/workflows/awstest.yml
  • multiqc_config - 'assets/multiqc_config.yml' not found

✅ Tests passed:

Run details

  • nf-core/tools version 2.13
  • Run at 2024-02-27 19:38:58

@drpatelh
Copy link
Member

Thanks @bentsherman !! 🙇🏽 We have refactored all of the module / subworkflow locations on dev now which would have saved moving the files into their own directories. Will make your changes clearer if the PR is rebased against dev.

Can we call the file nextflow.config instead of modules.config as this file will be shipped with modules / subworkflows / workflows too.

Doesn't really solve the problem of if statements in the config file. I believe they were added to avoid the config selector warnings when a process selector points to a process that isn't used in a particular pipeline run.

We did agree on a convention / standard for this but I can't remember what it was now. Maybe that we would suppress those warnings by default and have a special profile where they are printed for developers.

@bentsherman
Use module config
4218fd7 
Signed-off-by: Ben Sherman <[email protected]>
@bentsherman bentsherman changed the base branch from master to dev November 14, 2023 21:14
@nf-core nf-core deleted a comment from github-actions bot Nov 14, 2023
@bentsherman
Copy link
Author

Thanks, I was avoiding dev because I thought it might not be stable, but it seems to be fine.

Can we call the file nextflow.config instead of modules.config as this file will be shipped with modules / subworkflows / workflows too.

The thing is that the module config is special because it assumes the process scope (check the PR files to see what I mean), and I think the file name should reflect that. If we call it nextflow.config then people might think it's a regular config file when it isn't. And then nf-core has a narrower definition of "module" than Nextflow, where any script can be a module. Maybe I should call it process.config to denote that it only allows process settings.

If we want to make the file name arbitrary, then the module script will have to include the config file directly. We could add support for includeConfig to scripts and have that function as the module config. Need to think about it some more

@bentsherman
Copy link
Author

Doesn't really solve the problem of if statements in the config file. I believe they were added to avoid the config selector warnings when a process selector points to a process that isn't used in a particular pipeline run.

Actually I was wrong about this. There were no config selector warnings even though I removed all of the if statements. This is because the module config is not applied to the process until the process is actually invoked. So we are good to go in that regard 👍

@bentsherman
Copy link
Author

I tried to move the module config closer to the process modules rather than the workflows that invoke them. There is an art to this as it depends on what you consider to be sensible defaults for a process. Where two workflows call the same process with different config, I try to intuit which config is the "default" config and move that one. But the maintainers should really go through the config and decide how they want to organize the module configs.

@bentsherman bentsherman mentioned this pull request Jan 4, 2024
Open
@bentsherman
Copy link
Author

Closing in favor of the approach proposed here: nf-core/fetchngs#308

@bentsherman bentsherman deleted the module-config branch April 29, 2024 03:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@bentsherman @drpatelh