dockerize #439

allieychen · 2019-02-08T15:59:12Z

With the following change, the user is able to use docker run to run VT:

Add a script to use pipelines to run variant transforms.
Add a new docker file to have the above script as the entry point.

Tested: integration tests & manually tested on the new image.
Todo: add tests and update the doc

coveralls · 2019-02-08T16:11:04Z

Pull Request Test Coverage Report for Build 1637

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 88.937%

Totals
Change from base Build 1635:	0.0%
Covered Lines:	7042
Relevant Lines:	7918

💛 - Coveralls

kemp-google · 2019-02-12T00:43:05Z

docker/pipelines_runner/pipelines_runner.sh

+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+readonly google_cloud_project="${PROJECT}"


The GOOGLE_CLOUD_PROJECT variable should be used for this: it's already automatically set in some environments like Cloud Shell

However, we shouldn't force the user to set it anyway if we can avoid it. Given that you are mapping in $HOME/.config, the current project should be recoverable from gcloud

Thank you Aaron! Updated. PTAL :)

kemp-google · 2019-02-15T15:30:09Z

docker/pipelines_runner/Dockerfile

+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+FROM google/cloud-sdk


Do you really need cloud-sdk? Usually cloud-sdk:slim is sufficient...

Thanks, done.

kemp-google · 2019-02-15T15:32:29Z

docker/pipelines_runner/pipelines_runner.sh

+
+readonly google_cloud_project="${GOOGLE_CLOUD_PROJECT:-$(gcloud config get-value project)}"
+readonly temp_location="${TEMP_LOCATION}"
+readonly service_account_scopes="${SERVICE_ACCOUNT_SCOPE:-https://www.googleapis.com/auth/cloud-platform}"


Why do we need this? Isn't cloud-platform always sufficient?

Yes, cloud-platform is sufficient. I don't expect many users would change this value, but I don't know whether there are cases that they want to limit their scope.

What I am trying to do is to provide flexibility, and the user can still have the options to change all pipelines' flags (see pipelines_flag below). I am debating with myself whether I should do this, since on one hand I don't want to expose the details of pipelines tool, but on the other hand we should allow the user to customize it.

kemp-google · 2019-02-15T15:33:57Z

docker/pipelines_runner/pipelines_runner.sh

+# limitations under the License.
+
+readonly google_cloud_project="${GOOGLE_CLOUD_PROJECT:-$(gcloud config get-value project)}"
+readonly temp_location="${TEMP_LOCATION}"


Do users typically want to inspect the temporary files? Ideally we would just build a bucket, set a reasonable TTL and delete it when we are done.

This isn't something we should do in this change but would be good to think about.

Thank you, this is a good point and removing it can simplify the running of our tool. I have created an issue (issue 442) for it.

BTW, the temp_location provided here is for pipelines logging. And the user doesn't need to provide one.

allieychen

Thank you Aaron! I have updated the base image. PTAL and let me know your opinions on my other comments. :)

allieychen · 2019-02-15T19:12:08Z

docker/pipelines_runner/pipelines_runner.sh

+
+readonly google_cloud_project="${GOOGLE_CLOUD_PROJECT:-$(gcloud config get-value project)}"
+readonly temp_location="${TEMP_LOCATION}"
+readonly service_account_scopes="${SERVICE_ACCOUNT_SCOPE:-https://www.googleapis.com/auth/cloud-platform}"


Yes, cloud-platform is sufficient. I don't expect many users would change this value, but I don't know whether there are cases that they want to limit their scope.

What I am trying to do is to provide flexibility, and the user can still have the options to change all pipelines' flags (see pipelines_flag below). I am debating with myself whether I should do this, since on one hand I don't want to expose the details of pipelines tool, but on the other hand we should allow the user to customize it.

allieychen · 2019-02-20T19:01:21Z

docker/pipelines_runner/pipelines_runner.sh

+# limitations under the License.
+
+readonly google_cloud_project="${GOOGLE_CLOUD_PROJECT:-$(gcloud config get-value project)}"
+readonly temp_location="${TEMP_LOCATION}"


Thank you, this is a good point and removing it can simplify the running of our tool. I have created an issue (issue 442) for it.

allieychen · 2019-02-20T19:16:21Z

docker/pipelines_runner/Dockerfile

+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+FROM google/cloud-sdk


Thanks, done.

allieychen · 2019-02-20T19:27:22Z

docker/pipelines_runner/pipelines_runner.sh

+# limitations under the License.
+
+readonly google_cloud_project="${GOOGLE_CLOUD_PROJECT:-$(gcloud config get-value project)}"
+readonly temp_location="${TEMP_LOCATION}"


BTW, the temp_location provided here is for pipelines logging. And the user doesn't need to provide one.

allieychen · 2019-03-05T20:10:58Z

Thanks Aaron! Based on our off-line discussion, I have modified the runner to take flags rather than environment variables. One sample run command:
docker run -v ~/.config:/root/.config NEW_DOCKER_IMAGE --command "vcf_to_bq --input_pattern INPUT_PATTERN --output_table OUTPUT_TABLE --temp_location TEMP_LOCATION".
PTAL :)

kemp-google · 2019-03-06T17:54:15Z

docker/pipelines_runner/pipelines_runner.sh

+function parse_args {
+  while [[ "$#" -gt 0 ]]; do
+    case "$1" in
+      ${PROJECT_OPT})


Just inline these flags:
"--project" etc

kemp-google · 2019-03-06T18:01:49Z

docker/pipelines_runner/pipelines_runner.sh

+        zones="$2"
+        ;;
+
+      ${COMMAND_OPT})


It might be nice to make this a positional argument instead

docker run -v ~/.config:/root/.config ... vcf_to_bq --input_pattern INPUT_PATTERN --output_table OUTPUT_TABLE --temp_location TEMP_LOCATION

Optional, though

docker/pipelines_runner/pipelines_runner.sh

allieychen

Thanks Aaron! Now we can run it as
docker run -v ~/.config:/root/.config NEW_DOCKER_IMAGE "vcf_to_bq --input_pattern INPUT_PATTERN --output_table OUTPUT_TABLE --temp_location TEMP_LOCATION".
PTAL :)

docker/pipelines_runner/pipelines_runner.sh

kemp-google · 2019-03-07T19:32:07Z

docker/pipelines_runner/pipelines_runner.sh

+
+function main {
+  options=`getopt -o '' -l project:,temp_location:,docker_image:,zones: -- "$@"`
+  parse_args "$@"


shouldn't you pass $options not $@ here?

We can. getopt inserts -- between the long arguments and the command (i.e.,
vcf_to_bq --input_pattern ...), which I find hard to understand, so I used "$@" instead (I should remove options= though). Why do u prefer $options?

Got it - as discussed offline, let's just add a comment that we are using getopt only for checking (and drop options=)

It would be best to put this inside parse_args since that function makes assumptions about the state of the arguments (ie, if the getopt line is removed, parse_args will fail).

Done. Thanks!

change the base image to slim replace environment variables with flags

allieychen requested a review from kemp-google February 8, 2019 16:14

kemp-google reviewed Feb 12, 2019

View reviewed changes

allieychen force-pushed the docker branch 5 times, most recently from f5bb3e9 to 406fbdb Compare February 13, 2019 19:05

kemp-google reviewed Feb 15, 2019

View reviewed changes

allieychen commented Feb 20, 2019

View reviewed changes

allieychen force-pushed the docker branch 6 times, most recently from 0273d44 to f151b68 Compare March 5, 2019 20:06

kemp-google reviewed Mar 6, 2019

View reviewed changes

allieychen force-pushed the docker branch from f151b68 to 7cc6ef0 Compare March 7, 2019 18:07

allieychen commented Mar 7, 2019

View reviewed changes

docker/pipelines_runner/pipelines_runner.sh Show resolved Hide resolved

kemp-google reviewed Mar 7, 2019

View reviewed changes

allieychen force-pushed the docker branch from b6df84d to 3ec2327 Compare March 8, 2019 21:53

allieychen added 6 commits March 13, 2019 09:42

dockerize

79b583b

set the default google project

2d6f6e0

address comments:

dc9a655

change the base image to slim replace environment variables with flags

Use getopt to simplify the arguments checking.

c14247b

add comment for getopt

38b086b

Update parse_args to include the validation of the args

336587c

allieychen force-pushed the docker branch from 3ec2327 to 336587c Compare March 13, 2019 13:42

kemp-google approved these changes Mar 14, 2019

View reviewed changes

allieychen added 2 commits March 14, 2019 13:13

Merge branch 'master' into docker

febc99d

Merge branch 'master' into docker

acd41db

allieychen merged commit 7a2309a into googlegenomics:master Mar 14, 2019

allieychen deleted the docker branch March 14, 2019 19:08

dockerize #439

dockerize #439

Uh oh!

Conversation

allieychen commented Feb 8, 2019

Uh oh!

coveralls commented Feb 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 1637

💛 - Coveralls

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

allieychen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

allieychen commented Mar 5, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

allieychen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

allieychen Mar 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

coveralls commented Feb 8, 2019 •

edited

Loading

allieychen Mar 7, 2019 •

edited

Loading