Time parameters like 4.d get parsed as double and fail schema validation

[KevinMenden]

Description of the bug

Using e.g. --max_time 4.d throws a validation exception because the schema expects a string for --max_time
which should be granted by internal conversion of nextflow Duration objects to string, which seems to fail in this case

Steps to reproduce

Running a pipeline with the new parameter validation and a time parameter as described above should reproduce the bug.
I did it with a fresh template.

[KevinMenden]

Some testing confirmed that it is indeed not caught by the cleanParameters function.

Adding

if (p['value'].getClass() == Double) {
    new_params.replace(p.key, p['value'].toString())
}

To that function captures the parameter, but converts it to 4.0 which then doesn't match the schema validation anymore.

[KevinMenden]

Catching the double and then appending the .d back doesn't work by the way and is also way to hacky.

Given that you can just specify e.g. 72.h instead of 3.d we can live without the day version, but the error message might not be very clear to the user unfortunately.

[ewels]

Did we solve this by changing how we specify dates in the end?

[KevinMenden]

We might have ... wanted to spend some quality time with this issue during the hackathon.
I'm not too worried any more because I didn't run into this in the nextflow plugin tests.

[ewels]

Is this fixed by #974 ?

[ewels]

Nope:

$ nextflow run nf-core-testpipeline/ -profile test,docker --max_time 4.d
N E X T F L O W  ~  version 21.03.0-edge
Launching `nf-core-testpipelinee/main.nf` [nauseous_visvesvaraya] - revision: de1773e09a


------------------------------------------------------
                                        ,--./,-.
        ___     __   __   __   ___     /,-._.--~'
  |\ | |__  __ /  ` /  \ |__) |__         }  {
  | \| |       \__, \__/ |  \ |___     \`-._,-`-,
                                        `._,._,'
  nf-core/testpipelinee v1.0dev
------------------------------------------------------


ERROR: Validation of pipeline parameters failed!


* --max_time: expected type: String, found: Double (4.0)

[ewels]

I don't totally understand why 4.d gives a double and fails but 72.h works fine..? 🤔

[KevinMenden]

Ah thanks for reminding me, need to have a closer look at this ...

I think it's because of the d (as in double), but that's just a suspicion

[KevinMenden]

We could probably use the hack from above:

if (p['value'].getClass() == Double) {
    new_params.replace(p.key, p['value'].toString())
}

and fix the the resulting string to pass validation. This is as hacky as it gets though ... :/

[KevinMenden]

Okay we can catch the parameter internally, fix it so it looks like a proper duration ("4.d") but then it still fails because it's already parsed as double internally and sets to run time limit to a rather short 4ms ...

It get's converted to 4.0 before the parameter validation, somewhere through the Nextflow/Groovy parsing process ...

It works when setting the parameter in nextflow.config, although then we get a 2.d instead of 4.d
So somewhere during the command line parsing the whole thing happens :/

[ewels]

@pditommaso have you seen this before? Nextflow time objects with days being parsed as a numeric double instead?

Sounds like it may be something that should be fixed upstream @KevinMenden..

[pditommaso]

I think it conflicts with 'd' literal for double https://stackoverflow.com/a/27368444/395921. 1.day works tho.

Also try to avoid hacks like the following, looks very bad

if (p['value'].getClass() == Double) {
    new_params.replace(p.key, p['value'].toString())
}

[KevinMenden]

Okay thanks - so then there's really nothing we can do here, except adapting the regex and use *.day instead, I guess.

Yes we're not using that, it doesn't really work anyway :) Although we use similar hacks to deal with the nextflow duration objects etc 👀 need to remove them before passing the object to the Schema validator

[ewels]

I didn't know about .day syntax - ok then yes let's just force people to use that 👍🏻

[ewels]

@pditommaso we just implemented this but ran into a problem - if a Nextflow config file uses a native memory variable type then Nextflow will automatically simplify this and uses the .d syntax.

params.some_time = 48.h
println(params.some_time)

$ nextflow run mem_test.nf
N E X T F L O W  ~  version 21.03.0-edge
Launching `mem_test.nf` [condescending_panini] - revision: 18bf7ca334
2d

So then we're kind of screwed as this then hits the string double parsing issue we see above. But there's nothing that we can do about it..

Is it possible to get Nextflow to use .day instead of .d as the primary unit type for these conversions?

[ewels]

x-ref nextflow-io/nextflow#2038

[ewels]

@KevinMenden @drpatelh - can this issue be closed now?

[drpatelh]

Yes, I believe we fixed it in these PRs:
#1012
#1003