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

Validate nextflow_schema.json that parameter type matches actual value #823

Closed
jfy133 opened this issue Jan 11, 2021 · 3 comments · Fixed by #830
Closed

Validate nextflow_schema.json that parameter type matches actual value #823

jfy133 opened this issue Jan 11, 2021 · 3 comments · Fixed by #830

Comments

@jfy133
Copy link
Member

jfy133 commented Jan 11, 2021

Is your feature request related to a problem? Please describe

I got a bug report in eager where although a params.json specifies a parameter to be an integer, it is formatted in the schema as a string. However in a validation check in the eager code, that parameter is checked against an integer.
i.e.
Schema specifies integer

                "bt2n": {
                    "type": "integer",
                    "description": "Specify the -N parameter for bowtie2 (mismatches in seed). This will override defaults from alignmode/sensitivity.",
                    "default": "0",
                    "fa_icon": "fas fa-sort-numeric-down",
                    "help_text": "The number of mismatches allowed in the seed during seed-and-extend procedure of Bowtie2. This will override any values set with `--bt2_sensitivity`. Can either be 0 or 1. Default: 0 (i.e. use`--bt2_sensitivity` defaults).\n\n> Modifies Bowtie2 parameters: `-N`"
                },

But building params.json, it is is in quotes

{
    "bt2n": "1",
}

But the check is outside quotes:

if (params.bt2n != 0 && params.bt2n != 1) {
    exit 1, "[nf-core/eager] error: invalid bowtie2 --bt2n (-N) parameter. Options: 0, 1. Found parameter: --bt2n ${params.bt2n}."
}

After discussion with @KevinMenden we identified that actually the issue likely stems from the fact the eager schema is very old. Probably at the time, the actual format of the value did not match the specified type - and this has since not been checked.

Describe the solution you'd like

During linting (at some point), the nextflow_schema.json should check that the format of the value matches the specified type.

For example, if the parameter type is specified to be a integer, check there are no quotes in the default.

This is important given some people may occasionally modify their JSONs out the build schema and make this mistake.

Describe alternatives you've considered

Manually fix the eager json (which I will do anyway).

Additional context

https://nfcore.slack.com/archives/CUC8PPDL2/p1610343655159600

@jfy133 jfy133 changed the title Validate nextflow_schema.json that type matches actual value Validate nextflow_schema.json that parameter type matches actual value Jan 11, 2021
@KevinMenden
Copy link
Contributor

I agree things like this should be captured by nf-core lint . Manual changing of the json schema will probably happen every now and then :)

It basically should already be captured be the schema check in the linting function .... maybe the schema validation doesn't look at the default values.

@KevinMenden
Copy link
Contributor

Yep adding a validation check for the default parameter should solve that problem. Currently only the schema format is checked, which doesn't validate the default parameters it seems.

Also pointed to the hostremoval_model by the way, which has strip and replace as enum but remove is default, so that doesn't pass the test.

@KevinMenden KevinMenden linked a pull request Jan 15, 2021 that will close this issue
4 tasks
@KevinMenden
Copy link
Contributor

Made a draft PR that should fix this problem #830

Just draft for now because I want to go over this a bit more and test it but I think it should be fine. Not too complicated.

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

Successfully merging a pull request may close this issue.

2 participants