created a new build_docker_remote.sh script for building the docker image remotely with google cloud build by jamesemery · Pull Request #7951 · broadinstitute/gatk

jamesemery · 2022-07-19T20:55:57Z

…mage remotely with google cloud build

codecov · 2022-07-19T22:28:03Z

Codecov Report

Merging #7951 (3bcf33a) into master (ace7821) will decrease coverage by 0.025%.
The diff coverage is n/a.

@@               Coverage Diff               @@
##              master     #7951       +/-   ##
===============================================
- Coverage     87.089%   87.064%   -0.025%     
- Complexity     36993     37014       +21     
===============================================
  Files           2217      2218        +1     
  Lines         173847    173760       -87     
  Branches       18791     18769       -22     
===============================================
- Hits          151402    151282      -120     
- Misses         15816     15892       +76     
+ Partials        6629      6586       -43

Impacted Files	Coverage Δ
...er/tools/spark/sv/evidence/BreakpointEvidence.java	`66.822% <0.000%> (-12.383%)`	⬇️
...ute/hellbender/utils/reference/ReferenceUtils.java	`73.529% <0.000%> (-5.418%)`	⬇️
...on/FindBreakpointEvidenceSparkIntegrationTest.java	`96.296% <0.000%> (-3.704%)`	⬇️
...rg/broadinstitute/hellbender/utils/SVInterval.java	`90.000% <0.000%> (-2.857%)`	⬇️
...ute/hellbender/tools/spark/utils/IntHistogram.java	`86.335% <0.000%> (-2.484%)`	⬇️
.../sv/integration/SVIntegrationTestDataProvider.java	`93.333% <0.000%> (-0.784%)`	⬇️
...nder/utils/runtime/StreamingProcessController.java	`66.818% <0.000%> (-0.455%)`	⬇️
...tructuralVariationDiscoveryArgumentCollection.java	`94.595% <0.000%> (-0.142%)`	⬇️
...nstitute/hellbender/utils/haplotype/Haplotype.java	`90.244% <0.000%> (ø)`
...itute/hellbender/engine/spark/GATKRegistrator.java	`100.000% <0.000%> (ø)`
... and 18 more

build_docker_remote.sh

meganshand · 2022-07-21T14:09:44Z

build_docker_remote.sh

+
+## generate the tag if it wasn't explicitly specified
+if [ -z "$DOCKER_IMAGE_TAG" ]; then
+  DOCKER_IMAGE_TAG=${GCR_REPO}:$(whoami)-${GITHUB_TAG}-${GIT_HASH_FOR_TAG}


If a Github tag is used this ends up with a name ending in tag-tag. For example: us.gcr.io/broad-dsde-methods/broad-gatk-snapshots/gatk-remote-builds:mshand-4.2.6.1-4.2.6.1. I don't think that's a problem, but just fyi

Yeah. I decided against going down the rabbit hole of trying too hard to make a reasonable image name.... If you pull a specific commit then there isn't necessarily a branch associated and I don't think this is super high priority to fix given that most ofthe time people should give their own -T tags

meganshand · 2022-07-21T14:15:00Z

scripts/docker/README.md

-``scripts/docker/gatkbase/build_docker_base.sh`` is a script to create the gatkbase docker image.
+``scripts/docker/gatkbase/build_docker_base.sh`` is a script to create the gatkbase docker image. 
 ``build_docker.sh`` is a script to create the full gatk4 docker image.
+``build_docker_remote.sh`` is a script to create the full gatk4 docker image using google cloud build remotely. NOTE: this requires the user first specify their project with the command `gcloud config set project <PROJECT>` to a project that has access to google cloud build. 


Is it worth mentioning somewhere that one potential motivation for using the remote version is if you have M1 hardware?

droazen

@jamesemery Back to you with my comments

droazen · 2022-07-21T16:39:05Z

build_docker_remote.sh

+
+REPO=broadinstitute
+PROJECT=gatk
+REPO_PRJ=${REPO}/${PROJECT}


Delete REPO_PRJ -- it is a reference to the dockerhub repo, which we can't push to in this script.

False, its used farther down git clone https://github.com/${REPO}/${PROJECT}.git when we do the git clone.

No it's not -- that's $REPO and $PROJECT. $REPO_PRJ is unused, and in the original script it's used for the dockerhub repo.

build_docker_remote.sh

droazen · 2022-07-21T17:06:27Z

build_docker_remote.sh

+
+SUBMIT_COMMAND="gcloud builds submit --tag ${DOCKER_IMAGE_TAG} --timeout=24h --machine-type n1_highcpu_8"
+echo "running the following gcloud command: ${SUBMIT_COMMAND}"
+echo -n "" >> .gcloudignore


What does this line do? Can you add a comment?

Its from the Carrot version of this code. Without an explicit gcloudignore set it blacklists the .git directory which causes our script to crash since we need to download the lfs files from the intermediate docker image. I've added a comment

droazen · 2022-07-21T17:07:59Z

build_docker_remote.sh

+
+cd ${ORIGINAL_WORKING_DIRECTORY}
+if [ -n "$STAGING_DIR" ] ; then
+    rm -Rf ${STAGING_DIR}/${STAGING_CLONE_DIR}


I think the clone actually may get put into ${STAGING_DIR}/${STAGING_DIR}/${STAGING_CLONE_DIR} rather than ${STAGING_DIR}/${STAGING_CLONE_DIR}. Can you double-check this and make sure the script is cleaning up properly?

build_docker_remote.sh

scripts/docker/README.md

droazen · 2022-07-21T17:11:58Z

scripts/docker/README.md

+export GITHUB_TAG=4.2.6.1
+
+# REPLACE with the directory where you'd like to clone the repo
+export STAGING_DIR=~/tmp/tmp_build_docker_image/


These don't actually have to be exported, since you're passing them into the script directly below.

I'm simply copying the syntax of the other dozen of these things in this folder

jamesemery · 2022-07-22T14:13:26Z

@droazen i responded to your comments can this be merged?

droazen

@jamesemery Still a few issues here...

droazen · 2022-07-22T15:55:16Z

build_docker_remote.sh

+    mkdir -p ${STAGING_DIR}
+    cd ${STAGING_DIR}
+    set +e
+    rm -Rf ${STAGING_DIR}/${STAGING_CLONE_DIR}


We cd into ${STAGING_DIR}, and then rm -Rf ${STAGING_DIR}/${STAGING_CLONE_DIR}. This still seems wrong...

droazen · 2022-07-22T15:55:48Z

build_docker.sh

@@ -93,7 +93,7 @@ if [ -n "$STAGING_DIR" ]; then
    set +e
    rm -Rf ${STAGING_DIR}/${STAGING_CLONE_DIR}


Same issue here -- we've just cd'd into ${STAGING_DIR}, so why are we deleting ${STAGING_DIR}/${STAGING_CLONE_DIR}?

droazen · 2022-07-22T15:56:50Z

build_docker_remote.sh

+    rm -Rf ${STAGING_DIR}/${STAGING_CLONE_DIR}
+    set -e
+    GIT_LFS_SKIP_SMUDGE=1 git clone https://github.com/${REPO}/${PROJECT}.git ${STAGING_CLONE_DIR}
+    cd ${STAGING_DIR}/${STAGING_CLONE_DIR}


How does this cd command even succeed? We already cd'd into ${STAGING_DIR}, and now we are cd'ing into ${STAGING_DIR}/${STAGING_CLONE_DIR}?

build_docker_remote.sh

droazen · 2022-07-22T16:03:03Z

build_docker_remote.sh

+
+echo "Building image with the tag ${DOCKER_IMAGE_TAG}..."
+
+SUBMIT_COMMAND="gcloud builds submit --tag ${DOCKER_IMAGE_TAG} --timeout=24h --machine-type n1_highcpu_8"


Will an existing image with the same tag get overwritten by this command?

yes. but thats effectively impossible unless the user specifies the image themselves (in which case its their fault and problem) since we include the github tag/hash in the image name

scripts/docker/README.md

jamesemery · 2022-07-22T18:27:41Z

@droazen one more round

droazen

👍 Merge after doing a test run of the latest version with both relative and absolute paths for the staging dir.

jamesemery · 2022-07-25T16:31:59Z

@droazen just checked and it works with both relative and absolute paths. I'm going to hit merge.

created a new build_docker_remote.sh script for building the docker i…

8f1bc3b

…mage remotely with google cloud build

jamesemery assigned droazen Jul 19, 2022

jamesemery requested review from droazen and meganshand July 20, 2022 18:22

jamesemery mentioned this pull request Jul 20, 2022

Support for flow based sequencing #7876

Merged

meganshand approved these changes Jul 21, 2022

View reviewed changes

droazen suggested changes Jul 21, 2022

View reviewed changes

responded to github comments

2a67c6b

droazen suggested changes Jul 22, 2022

View reviewed changes

responded to yet more comments

3bcf33a

droazen approved these changes Jul 22, 2022

View reviewed changes

jamesemery merged commit 8c348aa into master Jul 25, 2022

jamesemery deleted the je_addRemoteDockerBuildScript branch July 25, 2022 16:32

		@@ -93,7 +93,7 @@ if [ -n "$STAGING_DIR" ]; then
		set +e
		rm -Rf ${STAGING_DIR}/${STAGING_CLONE_DIR}


		echo "Building image with the tag ${DOCKER_IMAGE_TAG}..."

		SUBMIT_COMMAND="gcloud builds submit --tag ${DOCKER_IMAGE_TAG} --timeout=24h --machine-type n1_highcpu_8"

Conversation

jamesemery commented Jul 19, 2022

Uh oh!

codecov bot commented Jul 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

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!

droazen 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!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamesemery commented Jul 22, 2022

Uh oh!

droazen 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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jamesemery commented Jul 22, 2022

Uh oh!

droazen left a comment

Choose a reason for hiding this comment

Uh oh!

jamesemery commented Jul 25, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Jul 19, 2022 •

edited

Loading