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

Stricter rules for initialising params with no default value #992

Closed
drpatelh opened this issue Apr 7, 2021 · 7 comments
Closed

Stricter rules for initialising params with no default value #992

drpatelh opened this issue Apr 7, 2021 · 7 comments
Assignees

Comments

@drpatelh
Copy link
Member

drpatelh commented Apr 7, 2021

It would be good to standardise how we are defining default / empty parameter values in the pipeline nextflow.config. Seeing as we are now validating these parameters against the JSON schema it will be nice to use a uniform approach.

As shouted on Slack:

  • Default values should be specified as usual using native Groovy variable types
  • BOOLEAN PARAMETERS SHOULD BE SET TO false in nextflow.config
  • FILE PATHS / STRINGS / INTEGERS / FLOATS / LISTS / MAPS SHOULD BE SET TO null in nextflow.config
@ewels
Copy link
Member

ewels commented Apr 25, 2021

  • nf-core schema lint should check for this and fail if false found for a non-boolean
  • Can look into removing workarounds for empty strings / false values with no default..

@drpatelh
Copy link
Member Author

drpatelh commented Apr 26, 2021

nf-core schema lint should check for this and fail if false found for a non-boolean

This is a little tricky because NF doesn't accept empty strings on the command-line e.g. --foo '' and will ignore the quotes and instead set params.foo = true in the workflow. I came across this in the rnaseq pipeline the other day and the only workaround to evaluate if that parameter has been set is to use --foo false.

@ewels
Copy link
Member

ewels commented Apr 28, 2021

Ah and doing --foo false would then fail the parameter validation you mean? Yeah bit of an edge case but I can see that this could cause problems. What does --foo null do? Or --foo "''"?

@ewels ewels changed the title Add docs and lint for default parameter values specified in nextflow.config Stricter rules for initialising params with no default value May 1, 2021
@ggabernet
Copy link
Member

ggabernet commented May 3, 2021

I do have a case where I want to set an integer parameter to 0 by default (not null). It specifies the start position for e.g. a UMI barcode or a sequencing primer. So I actually need to perform some arithmetics on it later in the pipeline.

So I think having an integer default to 0 shouldn't be interpreted as false. How should then this default be specified?

@ewels
Copy link
Member

ewels commented May 6, 2021

Yes that should be totally fine - set it to 0. If it is set to an int and has the default set to 0 in the schema then it will be fine - basically the same setup as if it was any other number.

This is about being more strict when you don't want to have a default value, which in turn allows to be stricter about defaults that evaluate to false like this.

@ewels
Copy link
Member

ewels commented May 6, 2021

In practice this will basically mean deleting / making these workarounds more strict:

tools/nf_core/schema.py

Lines 504 to 506 in d0a6649

# Assume that false and empty strings shouldn't be a default
if p_val == "false" or p_val == "":
del p_schema["default"]

if (schema_value == null) {
if (param_type == 'boolean') {
schema_value = false
}
if (param_type == 'string') {
schema_value = ''
}
if (param_type == 'integer') {
schema_value = 0
}
} else {

// remove anything evaluating to false
if (!p['value']) {
new_params.remove(p.key)
}

def defaultValue = group_params.get(param).default ? " [default: " + group_params.get(param).default.toString() + "]" : ''

..and probably more (eg. in website code too etc)

@KevinMenden
Copy link
Contributor

I can have a go at this, unless anyone has started something already?

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

4 participants