chanzuckerberg / idseq-workflows

Portable WDL workflows for IDseq production pipelines
https://idseq.net/
MIT License
31 stars 12 forks source link

Create update-run.wdl #74

Closed lynnlangit closed 3 years ago

lynnlangit commented 3 years ago

added update-run.wdl with most miniwdl check validation errors fixed. NOTE: remaining errors are noted in the comments at the EOF

was able to run successful test of this updated run WDL files using sample data in this repo on GCP Notebook instance.

lynnlangit commented 3 years ago

added links to 'learn miniwdl course and to example screencast on README.md page for consensus-genome

kislyuk commented 3 years ago

Hi Lynn, can you update the original run.wdl file instead? We don't want to have two copies of the workflow going forward.

lynnlangit commented 3 years ago

renamed update-run.wdl to run.wdl per your request

mlin commented 3 years ago

Summarizing @lynnlangit's changes to the WDL here,

  1. The s3_wd_uri input was unused, however, I see this comment next to it: # Dummy values - required by SFN interface; if that's true, then probably we want to suppress the lint warning (with #!UnusedDeclaration on the same or following line) instead of deleting the input
  2. The Quast prefix and threads, and ComputeStats ref_host inputs were unused. Perhaps if @tfrcarvalho @jshoe or @katrinakalantar could comment whether this is surprising.
  3. Several of the workflow outputs were marked optional File? but there's no control path that would lead to them being absent, so they can just be declared non-optional File. (No reason to leave the optional versions commented out)
  4. The remaining lint warnings are similar minor nits e.g. declaring a non-empty Array[X]+ that isn't statically guaranteed to be non-empty (but might always be in practice)
katrinakalantar commented 3 years ago

Regarding @mlin comment 2, it looks ok to me that the Quast prefix and threads, and ComputeStats ref_host inputs were unused. It is safe to remove the unused arguments from those functions.

mlin commented 3 years ago

Great, thanks @katrinakalantar

@lynnlangit do you want to finish mopping up to make a clean diff here?

  1. s3_wd_uri is needed for external reasons (@kislyuk true?) and we should leave it present, but suppress the warning with a #!UnusedDeclaration comment
  2. Delete stuff instead of commenting out; we'll always have the git history in the unlikely event we need to refer back
  3. The NonemptyCoercion warnings are kind of pedantic; it's just saying the declaration will cause a runtime error if given an empty array, which might well be the desired behavior. In the latest miniwdl v0.11.2, you can now miniwdl check --suppress NonemptyCoercion to turn them all off (a feature that had long been missing, so thanks for highlighting this! :)
  4. The StringCoercion of no_reads_quast: looking through the code, I think both declarations of no_reads_quast (one in the workflow, and one in the Quast task) can/should be Boolean instead of String; WDYT?
  5. SelectArray: is saying the outer array literal on that line doesn't have any optional items, so the outer select_all() will always return the same array it's given, and there's no point calling it. (Note that there's a nested inner invocation of select_all() in that declaration, which is needed.)
lynnlangit commented 3 years ago

@mlin

  1. done
  2. done
  3. done, added comment at top to explain flag
  4. added comment, change to Boolean
  5. no change needed, right?

fyi on line #191 there is an undone //TODO

jshoe commented 3 years ago

Migrated additions to #95