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.

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

Allow custom configs params to be parsed before nextflow.config #2662

Open
mahesh-panchal opened this issue Feb 21, 2022 · 13 comments
Open

Allow custom configs params to be parsed before nextflow.config #2662

mahesh-panchal opened this issue Feb 21, 2022 · 13 comments

Comments

@mahesh-panchal
Copy link
Contributor

mahesh-panchal commented Feb 21, 2022

New feature

I'm not sure whether to classify this as a feature request or bug.
The issue is that params configuration from custom configs (-c) appears to be read in after nextflow.config.
The docs describe that params provided in a custom config have a higher priority than those in the nextflow.config. This is true at the time of running the workflow, but not when parsing and evaluating the conditions in the Nextflow config.

For example:
main.nf:

#! /usr/bin/env nextflow

nextflow.enable.dsl = 2

workflow {
    FOO()
}

process FOO {

    output:
    path 'myfile.txt'

    script:
    """
    touch myfile.txt
    """
}

nextflow.config:

params.outdir = 'results'
process {
    withName: 'FOO' {
        publishDir = "$params.outdir/foo"
    }
}

If one runs:

$ nextflow run main.nf --outdir '/my/new/path'

or

$ cat params.yml
outdir : '/my/new/path'
$ nextflow run main.nf -params-file params.yml

Then the publishDir is correctly set.

However, if one uses

$ cat custom.config
params.outdir = '/my/new/path'
$ nextflow run main.nf -c custom.config

Then the publishDir is not correct, instead using the default value.

Usage scenario

Lot's of Nextflow users currently provide custom params via the -c option, not realizing it is not applying to configuration in the nextflow.config.

Suggest implementation

Perhaps read in any custom configs in the order described by the docs before and implement the configuration evaluation in such a way that if the keys are already present, don't override them.

@abhi18av
Copy link
Member

Good point, this situation seems to also affect launches from the tw CLI

tw launch $PIPELINE_URL `
    --profile conda_local `
    --params-file params.yml `              <-- Local params file
    --config slurm.config `                 <-- Local config file
    --compute-env $COMPUTE_ENV_ID `

@stale

This comment was marked as outdated.

@stale stale bot added the stale label Jul 31, 2022
@mahesh-panchal
Copy link
Contributor Author

Is this intended to be addressed in the new config implementation?

@abhi18av
Copy link
Member

@mahesh-panchal , I think that this would also be addressed as per the overall vision. Though I don't have any more visibility at this point

@stale stale bot removed the stale label Aug 10, 2022
@stale

This comment was marked as outdated.

@stale stale bot added the stale label Jan 16, 2023
@maxulysse
Copy link
Contributor

I managed to overcome this by supplying params in a yml file

@stale stale bot removed the stale label Jan 16, 2023
@mahesh-panchal
Copy link
Contributor Author

Not everyone knows to do this. Also configs support things the yml and command line do not, e.g. environment variables (not possible in yml), or closures (config specific).

@maxulysse
Copy link
Contributor

Not everyone knows to do this. Also configs support things the yml and command line do not, e.g. environment variables (not possible in yml), or closures (config specific).

I know, I was commenting more to make the issue not stale anymore

@jfy133
Copy link
Contributor

jfy133 commented Jan 25, 2023

Could someone please document this mysterious params yml file 😅?

I can find a single reference to it here: https://www.nextflow.io/docs/latest/config.html?highlight=profiles and that's just the flag used to supply it... but that's after the -params-file not even being searchable on the documentation (https://www.nextflow.io/docs/latest/search.html?q=-params-file&check_keywords=yes&area=default, no hits).

I find this is a really bad regression/bug, I know a lot of users who have been using configs/profiles to set parameters for a long time and after some testing I find that e.g. tower.nf configuration and also nf-core run headers correctly resolve the user supplied parameter in the config, but that the parameter is not actually passed to the command itself.

So essentially this a completely silent but major error, and a user would only discover it if they looked through every single .command.sh or table entry in tower (which is not to be expected of a user really).

So that a) the bug exists, and b) the solution is not actually documented (or publically reported) is really worrying and I would argue this should be a high priority for fixing :\

EDIT: by not documented, I mean a) not searchable, b) not in the 'logical' place (i.e. the configuration section) and c) the structure of the file is not reported either...

@jfy133
Copy link
Contributor

jfy133 commented Jan 25, 2023

Oh btw it should be noted this issue only applies for DSL2 workflows

@stale

This comment was marked as outdated.

@stale stale bot added the stale label Aug 12, 2023
@jfy133
Copy link
Contributor

jfy133 commented Aug 15, 2023

The documentation has been partly addressed (, see the closed PR above which was replaced by a different PR by the main Devs), but I think this is still an issue

@bentsherman
Copy link
Member

I realized I've been seeing this issue for a while with nf-core pipelines. As described in the OP, if you set params.outdir in a config file instead of on the command line or params file, Nextflow will save some reports to a null directory while everything else will be published correctly.

In order to fix it, I think we would need to parse the config files twice -- first to collect the params, second to parse the other scopes. I'm not sure how difficult that would be.

@pditommaso I think this issue is another good reason to allow the report file settings to be closures (#3651). This way, the settings would be evaluated only after all the params and config files have been resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants