search

theiagen / public_health_bioinformatics

Bioinformatics workflows for genomic characterization, submission preparation, and genomic epidemiology of pathogens of public health concern.
GNU General Public License v3.0
37 stars 17 forks source link

[Augur wf] Potential bug with augur_clades call block conditional `run_traits` #633

Open kapsakcj opened 2 months ago

kapsakcj commented 2 months ago

:bug:

:pencil: Describe the Issue

Can someone please double check the logic here?

https://github.com/theiagen/public_health_bioinformatics/blob/9a10de708d97bbe5a5cbcce80c8e1ef5509f3f24/workflows/phylogenetics/wf_augur.wdl#L152

I think this is set incorrectly and should be if (run_traits); <do stuff below> instead of how it's set currently.

@jrotieno can you comment?

We have a user that is running HAV through Augur and despite run_traits being set to false, the augur_clades task is queued & run (due to organism_parameters sub workflow running and defining a minimal clades TSV file (that is empty except for column headers)) and the task fails due to no clades being defined in the minimal clades TSV file:

Augur_clades task output failure:

ERROR: No clades were defined in /cromwell_root/theiagen-public-files-rp/terra/augur-defaults/minimal-clades.tsv

workflow here: https://app.terra.bio/#workspaces/vrdl-billing/dataAnalysis_HAV_VRDL/job_history/fbb5974a-04fd-472e-b3e4-41f8a3b64aa7

:repeat: How to Reproduce

v1.3.0

:computer: Version Information

kapsakcj commented 2 months ago

https://app.terra.bio/#workspaces/theiagen-training-workspaces/Theiagen_Young_Sandbox/job_history/55abb7a6-f17a-4992-9d11-4d9ea2d8511c

With this test workflow, we set run_traits to true and if the workflow succeeds (and skips the augur_clades task) then this helps provide evidence that the conditional logic is incorrect. Let's see!

jrotieno commented 2 months ago

Yup, something needs to be fixed here.

kapsakcj commented 2 months ago

The workflow linked above did indeed skip the augur_clades task, so I think the code change is simply removing the ! from the line I permalinked above. So if(run_traits) { ....

kapsakcj commented 2 months ago

Noting that we need to review the documentation for this particular Augur optional input genome_length_input and could be done as part of this PR. It currently states that it is required for organisms without standards (like SC2, MPXV, etc.) but it should NOT be provided by the user for the Augur workflow.