Include config modules.config in the wrong place ?

[LouisLeNezet]

Description of the bug

Hi !

With the new version of the template the import of the modules.config files as been moved to the bottom of the nextflow.config file.
However all the test config files are imported above in the nextflow.config and therefore any modification to a process present in both a test.config and in the modules.config would be overwritten by the later.

This does happen in phaseimpute and we just moved it next to base.config.

// Load base.config by default for all pipelines
includeConfig 'conf/base.config'

// Load modules.config for DSL2 module specific options
includeConfig 'conf/modules.config'

What were the rational for this modification ?

Command used and terminal output

System information

No response

[mirpedrol]

Hi @LouisLeNezet, the modules.config file has been included the last for a long time (years). It's not common to modify the modules arguments through the test configs, people usually use params for this. But it's true that it is a possibility, so we should change the location where we include this config, thanks for reporting 🙂

[LouisLeNezet]

You're welcome 😉
The rational for me was that in a test case the files are really small, and the parameters for some of the process might need to be changed to be able to run. In our case it was mostly the window size for the chunking of the chromosomes.
But I stumble across this issue when reviewing another PR and it's a bit tricky to see it as the order of import isn't quite visible.

[mahesh-panchal]

I just saw this. Now the modules.config doesn't pick the params modified by test profiles and such.

[LouisLeNezet]

Shouldn't it be the case ?
The aim of this PR was that the modules.config do not overwrite the test.config.

[mahesh-panchal]

The reason is here: https://nfcore.slack.com/archives/C043FMKUNLB/p1710950665642739?thread_ts=1710949949.559169&cid=C043FMKUNLB

Basically, if a params is set after the modules.config value is resolved, it won't get reassigned.

[mahesh-panchal]

Forcing any reference to a params to also use a closure should ensure your way keeps working but it's going to break anything using params that doesn't support a closure.

[mashehu]

@mahesh-panchal anyway around this? or should we just revert this change?

[mahesh-panchal]

If #2508 gets implemented, then this might be ok. It's just a question if anyone uses params in test profile to affect something that doesn't accept a closure. i.e. the solution is a closure around the values to ext.args, ext.prefix, etc in the modules.config.

I didn't reopen this basically because I wanted to see if it actually affects people, but I also don't know how many will notice quickly. Theoretically it should break test profiles really quickly if affects them.

[mashehu]

okay, let's keep an eye open. And we might need to prioritize #2508. thanks for flagging this!

[mirpedrol]

The PR #3356 will revert that change until we add the linting described in #2508, to avoid other failures.
@LouisLeNezet you can make this change in phaseimpute for now, to have the fix for your pipeline
And let's prioritize #2508 👍

[LouisLeNezet]

No worry I'll do so !

[mahesh-panchal]

Once the new publishing syntax is also enforced, then this should not be a problem either later.