Skip to content

Support modules with exec: blocks#3633

Merged
mirpedrol merged 4 commits into
devfrom
module_exec
Aug 1, 2025
Merged

Support modules with exec: blocks#3633
mirpedrol merged 4 commits into
devfrom
module_exec

Conversation

@muffato

@muffato muffato commented Jun 20, 2025

Copy link
Copy Markdown
Member

With the current version, linting a module that has an exec: block yields the following error

╭─ [✗] 1 Module Test Failed ───────────────────────────────────────────────────╮
│                      ╷                          ╷                            │
│ Module name          │ File path                │ Test message               │
│╶─────────────────────┼──────────────────────────┼───────────────────────────╴│
│ hiccramalign/chunks  │ modules/sanger-tol/hicc… │ when: condition has too    │
│                      │                          │ many lines                 │
│                      ╵                          ╵                            │
╰──────────────────────────────────────────────────────────────────────────────╯

This is because the parser is not aware of exec: blocks.

In this PR I simply add code to recognise exec: blocks and check that they don't exist together with script: or shell:

Best,
Matthieu

PR checklist

  • This comment contains a description of changes (with reason)
  • CHANGELOG.md is updated
  • If you've fixed a bug or added code that should be tested, add tests!
  • Documentation in docs is updated

@codecov

codecov Bot commented Jun 20, 2025 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 50.00000% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.29%. Comparing base (9bff2b9) to head (2872af9).
⚠️ Report is 5 commits behind head on dev.

Files with missing lines Patch % Lines
nf_core/modules/lint/main_nf.py 50.00% 4 Missing ⚠️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@muffato muffato mentioned this pull request Jul 15, 2025
Merged
mirpedrol
mirpedrol approved these changes Jul 30, 2025
View reviewed changes

@mirpedrol mirpedrol left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Code changes look good to me, thanks!
But I would like to have a final approve from someone from @nf-core/maintainers to make sure that I am not missing something, and we don't want to allow an exec block in nf-core modules.

@jfy133

jfy133 commented Jul 30, 2025

Copy link
Copy Markdown
Member

I think there is no explicit reason not to support it but :

  1. make sure that it is only recommend to use in certain contexts (e.g. not heavy jobs, as it runs on the head node)
  2. we need to ensure that the structure of the block itself is consistent... (harshil align etc)

For now I think it's fine to merge this (exec: is valid Nextflow), but we should have the maintainers team develop a couple extra points for the specs page :)

@jfy133

jfy133 commented Jul 31, 2025

Copy link
Copy Markdown
Member

So far it seems people are half neutral half against supporting within nf-core/modules officially, but I dont think linting should complain about a valid nextflow syntax so again I would merge this now so people can happily have local modules

@SPPearce

SPPearce commented Jul 31, 2025

Copy link
Copy Markdown
Contributor

I agree with James's summary, we should support this in general but an nf-core module would need a convincing argument as to why we need exec there.

@FriederikeHanssen

FriederikeHanssen commented Jul 31, 2025 •
edited by jfy133
Loading

Copy link
Copy Markdown
Contributor

complain about a valid nextflow syntax

agreed, maybe as an improved later on, we can have stricter linting on the nf-core side to at least warn for execs (to Simon's point), but allow disableing this linting in a config file

@SPPearce

SPPearce commented Jul 31, 2025

Copy link
Copy Markdown
Contributor

complain about a valid nextflow syntax

agreed, maybe as an improved later on, we can have stricter linting on the nf-core side to at least warn for execs (to Simon's point), but allow disableing this linting in a config file

I think it is fine in nf-core pipelines, just not in the modules repo personally.

@FriederikeHanssen

FriederikeHanssen commented Jul 31, 2025

Copy link
Copy Markdown
Contributor

ah correct, I meant modules linting only

@mirpedrol

mirpedrol commented Aug 1, 2025

Copy link
Copy Markdown
Member

thanks for all the feedback! I am going to merge this PR then :)

@mirpedrol mirpedrol merged commit 1e3a7da into dev Aug 1, 2025
102 checks passed
@mirpedrol mirpedrol deleted the module_exec branch August 1, 2025 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mirpedrol mirpedrol mirpedrol approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@muffato @jfy133 @SPPearce @FriederikeHanssen @mirpedrol @nf-core-bot