WDL compatibility #33
Conversation
…rather than just flooding a VM
|
Hi! Thanks so much for contributing to Parliament2! Sorry it's taken so long for me to get around to looking at this. Could you please merge in the |
|
No worries! I've merged master in, but I haven't yet tested if WDL still works. Can we hold off on merging until that gets done sometime this week? |
|
Sure, take your time. |
|
@edawson Please merge in master again; I have added a patch so that hopefully the tests now work. Thanks |
|
@edawson Sorry about that -- had to do one more bug fix. Hopefully it should work now. |
There was a problem hiding this comment.
Thanks for the PR for WDL compatibility, @edawson ! Please check the change requests (mostly questions) within the code lines.
Also, if possible, please help add documentation draft for how to utilize the WDL file to run (on firecloud and/or other environments).
| @@ -114,4 +114,3 @@ WORKDIR /home/dnanexus | |||
| RUN ["chmod", "+x", "parliament2.py"] | |||
| RUN ["chmod", "+x", "parliament2.sh"] | |||
|
|
|||
| ENTRYPOINT ["python","/home/dnanexus/parliament2.py"] | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
@edawson Please merge master back into this branch (hopefully the testing framework will work this time) so that we can confirm how this line affects the Docker call (if at all). Thanks!
| if [ ! -z ${user_defined_threads} ] | ||
| then | ||
| threads="${user_defined_threads}" | ||
| fi |
There was a problem hiding this comment.
user_defined_threads will always be defined as the default value with 0 as in line 30 of parliament2.py, right?
Should we change line 91-94 to
if [[ "${user_defined_threads}" -ne 0 ]]; then
threads="${user_defined_threads}"
fi
?
There was a problem hiding this comment.
I think so - thanks for catching this!
| @@ -0,0 +1,8 @@ | |||
| { | |||
| "ParliamentWF.threads": "4", | |||
There was a problem hiding this comment.
Is there a reason for threads her set to 4?
parliament2.wdl is pointing to a runtime instance with 32 cores. Can we set the default here to 32?
At line 96 of parliament2.sh the script is reserving 3 threads. If we set default to 4, would this affect to be only one thread being used for running parliament2?
There was a problem hiding this comment.
This is just an example script that is used for testing locally on my laptop; the runtime:cpu argument is ignored by WDL locally. I agree this is a bad value though - we should probably set it dynamically in the WDL.
I'm not sure how the thread reservation on line 96 (now 90) works exactly - does the core parliament2 script need three threads, or can we reclaim some of those?
|
Hi, The standard dnanexus/parliament2 docker does not work with firecloud. I'm getting the error: "parliament2.py: error: argument --bam is required". It seems to give this error immediately upon running the docker image. What is the status on modifying parliament2's main dnanexus docker to work with WDL? |
I wanted to run Parliament2 on the Broad's FireCloud system, so I've wrapped it in a WDL description and made some changes to the Dockerfile and run scripts. I hope these don't break DNAnexus compatibility; unfortunately I don't really have a way to test if everything on this branch still works there.
There are a lot of optimizations that could still be made (e.g. running on preemptible VMs to cut run costs), but going for that seemed a little premature given how fast the code already is running on a single VM. Please let me know if there are any breaking changes and I'll try to fix them!