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

Adding a WDL demo #153

Merged
merged 3 commits into from
Jul 25, 2016
Merged

Adding a WDL demo #153

merged 3 commits into from
Jul 25, 2016

Conversation

briandoconnor
Copy link
Contributor

I added a WDL workflow and instructions on how to call this tool via WDL. I wanted to test that this tool is callable via Cromwell locally since I thought it would help with testing on the Broad's side.

I found that the ENTRYPOINT in Frank's Docker wasn't compatible with the way Cromwell calls the tool. It seems to assume the entrypoint is bash and pipes in the command.

To make this concrete, here's how Cromwell executes a command in a given Docker container:

"/bin/bash" "-c" "cat cromwell-executions/wf/ac99b2c7-2204-4827-a974-3873c3e6b5c5/call-adam/script | docker run --rm -v /Users/boconnor/Development/gitroot/BD2KGenomics/cgl-docker-lib/adam-pipeline/cromwell-executions/wf/ac99b2c7-2204-4827-a974-3873c3e6b5c5/call-adam:/root/wf/ac99b2c7-2204-4827-a974-3873c3e6b5c5/call-adam -i quay.io/ucsc_cgl/adam-pipeline /bin/bash <&0"

Feel free to tweak as needed or discard if I'm not understanding the issue properly.

I don't think I'm handling the output file correctly since it's not copied back to the output path specified in the JSON. I leave that for the Broad folks to comment on/fix.

@@ -0,0 +1,6 @@
{
"wf.adam.in": "/Users/boconnor/Development/gitroot/BD2KGenomics/cgl-docker-lib/adam-pipeline/test/small.sam",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are relative paths an option?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't work for me but I'm a total WDL newbie.

@fnothaft
Copy link
Contributor

@briandoconnor I just opened #154 that has some cleanup on top of this. Can you take a look and let me know what you think?

@fnothaft
Copy link
Contributor

Also, this /bin/bash -c business is ugly.

@briandoconnor
Copy link
Contributor Author

Not sure what the right thing to do with the entrypoint is. I think have a script/tool as the entrypoint is going to interfere with assumptions workflow systems like WDL and CWL have made about how they can execute a command inside a container. I don't think I have a clean solution. But this is just a demo. Something to think about for future containers since I don't want UCSC to make hundreds of tool Docker containers with customized entrypoints and then have those be unrunnable in any other system. It would be a lost opportunity.

@briandoconnor
Copy link
Contributor Author

Frank, by the way, please do what you want with this pull request. Since it's really a demo for the Broad it's your call how you want to tweak it/merge it with your changes.

@fnothaft
Copy link
Contributor

@hannes-ucsc this is good to merge. Let me know if you want the merge commit cleaned up.

@fnothaft
Copy link
Contributor

Tagging needs work again; @briandoconnor just ran into a test failure on his side.

@fnothaft
Copy link
Contributor

@briandoconnor I want to remove the merge commit and push some test cleanup to this branch. Is it OK with you if I force push to this branch?

@fnothaft fnothaft force-pushed the broad_demo_brian_edits branch from 83525a8 to a6edd9c Compare July 25, 2016 16:37
@fnothaft
Copy link
Contributor

@hannes-ucsc I've removed the needs work tag. Let me know if you want this squashed, otherwise let's merge.

@fnothaft
Copy link
Contributor

Merged, as per discussion with @hannes-ucsc! Thanks @briandoconnor.

cket pushed a commit to cket/cgl-docker-lib that referenced this pull request Sep 7, 2016
* Adding a WDL demo
* Made json into a template, added makefile target to elaborate template, added cromwell test.
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

Successfully merging this pull request may close these issues.

None yet

3 participants