Conditional include are not expected to work

[pditommaso]

Don't think this pattern is a good idea, if it works it's a bug!

viralrecon/main.nf

Lines 55 to 72 in a85d596

	if (params.public_data_ids) {
	include { SRA_DOWNLOAD } from './workflows/sra_download'
	SRA_DOWNLOAD ()
	//
	// WORKFLOW: Variant and de novo assembly analysis for Illumina data
	//
	} else if (params.platform == 'illumina') {
	include { ILLUMINA } from './workflows/illumina'
	ILLUMINA ()
	//
	// WORKFLOW: Variant analysis for Nanopore data
	//
	} else if (params.platform == 'nanopore') {
	include { NANOPORE } from './workflows/nanopore'
	NANOPORE ()
	}

[drpatelh]

It does work indeed because the CI tests each of these independently. Will need to remember why I done things this way but I suspect it was because include out of the conditional statement didn't give me the expected behaviour.

[drpatelh]

Yep, so the problem is that if I move the include out of the conditional block then the code in all of those workflows is still executed even if it isn't invoked with e.g. ILLUMINA (). This messes with the parameter validation etc I have written there because these workflows have different params and for example I get this error when running the Illumina workflow that is actually coming from this line as a result of include the Nanopore workflow out of the conditional block:

ARTIC scheme not specified with e.g. --artic_scheme 'nCoV-2019' or via a detectable config file.

Not sure I am making sense 😅 Let me try a couple of things.

[pditommaso]

I warn you this can stop at any time, this was never meant to work in this way

[drpatelh]

I agree. If you don't think it's a good idea then we need to find a fix 🛠️

So I had to move that conditional logic to within the workflow's themselves and these are the minimal changes I can make to get everything working f889766

How does that look?

[JoseEspinosa]

@pditommaso will it work if the include is inside a condition outside the workflow statement, this will then solve the problem @drpatelh mentioned above.

e.g.

if (params.public_data_ids) {
    include { SRA_DOWNLOAD } from "${moduleDir}/workflows/sra_download"
}
else if (params.platform == 'illumina') {
    include { ILLUMINA } from "${moduleDir}/workflows/illumina.nf"
}
else {
    include { NANOPORE } from "${moduleDir}/workflows/nanopore"
}

workflow NFCORE_VIRALRECON {
    if (params.public_data_ids) {
        SRA_DOWNLOAD ()
    } else if (params.platform == 'illumina') {
        ILLUMINA ()
    } else if (params.platform == 'nanopore') {
        NANOPORE ()
    }
}

[pditommaso]

That should be ok, but what's the real need of condition inclusion?

[JoseEspinosa]

This 👇

Yep, so the problem is that if I move the include out of the conditional block then the code in all of those workflows is still executed even if it isn't invoked with e.g. ILLUMINA (). This messes with the parameter validation etc I have written there because these workflows have different params and for example I get this error when running the Illumina workflow that is actually coming from this line as a result of include the Nanopore workflow out of the conditional block:

ARTIC scheme not specified with e.g. --artic_scheme 'nCoV-2019' or via a detectable config file.

Not sure I am making sense 😅 Let me try a couple of things.

[drpatelh]

Much better solution @JoseEspinosa 👍🏽 Reverted to that here 40d520e

[drpatelh]

Tests are happy again. Thanks for pointing this out @pditommaso :) Will remember to change this elsewhere too.

[pditommaso]

so the problem is that if I move the include out of the conditional block then the code in all of those workflows is still executed even if it isn't invoked with e.g. ILLUMINA ().

Can you point out an example?

[drpatelh]

So both the ILLUMINA and NANOPORE workflows have their own parameter validation functions here and here. If we include both of these workflows at the same time without any conditional logic then both of those functions get executed at run-time. I would like to avoid that because the parameter validation will obviously depend on which workflow the user chooses to run.

[JoseEspinosa]

@pditommaso I modified the example I added on the nextflow issue I opened yesterday. If you run:

nextflow run main.nf --skip_foo

Due to the validation trigger by the inclusion of the FOO process you get an error.

N E X T F L O W  ~  version 21.04.0
Launching `main.nf` [mighty_wescoff] - revision: 71cf30c291
WARN: Access to undefined parameter `foo` -- Initialise it to a default value eg. `params.foo = some_value`
Missing foo parameter

This is solved by putting the include inside a conditional statement.

[pditommaso]

I think we should introduce a validation concept to separate this logic from the pipeline execution

[drpatelh]

Sounds interesting. How would something like that look? Would it be something implemented in native NF or in the pipeline?

[JoseEspinosa]

Yeah, also curious 🤓