Handle cases where the WDL workflow output contains exprs #201
Conversation
kinow
force-pushed
the
fix-200
branch
2 times, most recently
from
6ce5a7e to
317d796
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #201 +/- ##
==========================================
- Coverage 94.97% 94.65% -0.32%
==========================================
Files 7 3 -4
Lines 816 730 -86
Branches 223 217 -6
==========================================
- Hits 775 691 -84
+ Misses 16 14 -2
Partials 25 25 ☔ View full report in Codecov by Sentry. |
|
@kinow Thank you very much. I am sorry for late reply. |
| - id: out | ||
| type: File | ||
| outputBinding: | ||
| glob: $(inputs.Outfile) |
There was a problem hiding this comment.
@mr-c , I tried to manually partially translate the blast.wdl file. Could you take a look at the file above and see if it's going in the right direction here, please?
There are a couple TODO's where I am not sure how to translate something from WDL, 🙏
I used two tasks only, translating them both into CommandLineTool's. Also used that $graph trick to pack multiple processes in the same file. Finally, added Workflow named main for the blast.wdl workflow definition. The Workflow uses conditional to pick the first non null (i.e. select_first and if's from WDL).
There was a problem hiding this comment.
Great approach! Do we have test data so we can compare the WDL and CWL versions?
| - id: blast_docker | ||
| type: string | ||
| # TODO: how to translate the original? | ||
| # String blast_docker = select_first([blast_docker_override,"swr.cn-south-1.myhuaweicloud.com/cngbdb/blast:1.2"]) |
There was a problem hiding this comment.
For the general case of programmatic conversion, we would need to synthesize a new step to compute the usable value of blast_docker and use the output of that step everywhere that originally referred to the blast_docker input.
However, if I was converting this by hand I would have combined blast_docker_override and blast_docker into a single CWL input with a default value. However, this is being used to override the docker image name with a dynamic value, which is not yet supported in CWL; so it would be useless :-P
Anyhow, we still need a general way to convert WDL expressions in the definition of inputs. I recommend creating a separate issue for that; modifying this WDL to not need workflow input declaration expressions, and focusing on synthesizing new steps for workflow output expressions.
| # TODO: how to translate the original? | ||
| # String Outfile = basename(queryfa)+'.blast_result.txt' | ||
| - id: Outfile | ||
| type: string |
There was a problem hiding this comment.
This is another WDL expression in the definition of an input; I've made an issue for that #240
|
@kinow, when you've got a working and tested manually written CWL conversion, please see (or take over) my work at https://github.com/common-workflow-lab/wdl-cwl-translator/pull/191/files for synthesizing new steps to deal with workflow output expressions |
| - id: blast_docker | ||
| type: string | ||
| # TODO: how to translate the original? | ||
| # String blast_docker = select_first([blast_docker_override,"swr.cn-south-1.myhuaweicloud.com/cngbdb/blast:1.2"]) |
There was a problem hiding this comment.
For the general case of programmatic conversion, we would need to synthesize a new step to compute the usable value of blast_docker and use the output of that step everywhere that originally referred to the blast_docker input.
However, if I was converting this by hand I would have combined blast_docker_override and blast_docker into a single CWL input with a default value. However, this is being used to override the docker image name with a dynamic value, which is not yet supported in CWL; so it would be useless :-P
Anyhow, we still need a general way to convert WDL expressions in the definition of inputs. I recommend creating a separate issue for that; modifying this WDL to not need workflow input declaration expressions, and focusing on synthesizing new steps for workflow output expressions.
| outfmt: outfmt | ||
| Outfile: Outfile | ||
| threads: threads | ||
| run: runblastp |
There was a problem hiding this comment.
| run: runblastp | |
| run: #runblastp |
| - id: out | ||
| type: File | ||
| outputBinding: | ||
| glob: $(inputs.Outfile) |
There was a problem hiding this comment.
Great approach! Do we have test data so we can compare the WDL and CWL versions?
Closes #200
@admi2u, can you check if this branch's code works for you? It produced the
blast.cwlfile in this branch/pull request.I am taking a look at the tests that broke after this change, and will push a new commit to fix it later.
Thanks
Bruno