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

Boolean evaluation result incorrect for enabled attribute of publishDir #2677

Closed
junjun-zhang opened this issue Feb 25, 2022 · 4 comments
Closed

Comments

@junjun-zhang
Copy link

Bug report

With the following testing script, when an outdir params is provided, there is no output folder created.

nextflow.enable.dsl=2

params.name = "world"
params.outdir = ""

process test_publishDir {
  publishDir "${params.outdir}/${task.process.replaceAll(':', '_')}", mode: "copy", enabled: params.outdir
  input:
    val name
  output:
    path "*.txt"
  script:
    """
    echo "Hello ${name}!" > hello.txt
    """
}

workflow {
  test_publishDir(params.name)
}

Expected behavior and actual behavior

Expected behavior: run the above process by nextflow run test_publishDir.nf --outdir out, I should be able to see the output inside of a folder named as out. Previously this worked fine as discussed here: #1933

Actual behavior: no out folder created.

Environment

  • Nextflow version: 21.10.6
  • Java version: openjdk version "17.0.2"
  • Operating system: macOS
  • shell version: zsh 5.8

Additional context

  • Run the same test using nextflow 21.04.3 works perfectly fine
  • With nextflow 21.10.6, if updated as enabled: params.outdir ? true : false then it works as expected.
@pditommaso
Copy link
Member

Think it's a side effect of this change

651835d

However, I think it's still consistent because enabled is expected to be a bool.

Changing the param to the following should work

params.outdir = false

@mjhipp
Copy link

mjhipp commented Mar 3, 2022

@junjun-zhang Simply changing the enabled part to: params.outDir as Boolean fixes the issue.

  publishDir "${params.outdir}/${task.process.replaceAll(':', '_')}", mode: "copy", enabled: params.outdir as Boolean

I did this in my project as well, and mentioned in our original discussion: #1933 (reply in thread)

It threw me at first also, but it is not really a bug, but a bugfix. Consider:

Before, enabled used as boolean, which is like this:

groovy> "/path/to/output" as boolean // output is defined 
 
Result: true

groovy> "" as boolean // output not defined 
 
Result: false 

Now, it uses parseBoolean():

groovy> Boolean.parseBoolean("/path/to/output") // output is defined 
 
Result: false

groovy> Boolean.parseBoolean("") // output not defined 
 
Result: false

The reason they changed it is because of this:

groovy> "false" as boolean 
 
Result: true

By changing your enabled to include the as Boolean yourself, you get the desired effect:

groovy> Boolean.parseBoolean("${'/path/to/output' as boolean}") 
 
Result: true

It meant having to change this line in all process files, which was a bit tedious. In fact, it is what inspired me to open this PR: #2432 (@pditommaso), which would make it much easier to set this default publishing behavior.

@pditommaso
Copy link
Member

pditommaso commented Mar 4, 2022

The rationale of this change is to allow interpreting "true" (string) as true (bool)

#2220 (comment)

@pditommaso
Copy link
Member

Inclined to keep in this form. Closing this issue, feel to comment below if needed.

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

No branches or pull requests

3 participants