Add manifest summary file to GvsExtractCallset #7457

ericsong · 2021-09-07T19:13:15Z

The manifest file describes the files produced by the GvsExtractCallset task and is uploaded to GCS once all the shards have finished running.

The existence of the manifest.txt file can be used to determine if the extraction is complete or not by just using gsutil command and not going through Cromwell/Terra.

Here's a snippet of what that looks like.

interval_number, vcf_file_location, vcf_file_bytes, vcf_index_location, vcf_index_bytes
0,gs://fc-secure-765db3ba-3e2d-432e-89eb-9efda7430f93/genomic-extractions/66f225d3-35cf-4ff7-a4ec-3d887a86ea8b/vcfs/interval_0.vcf.gz,879403,gs://fc-secure-765db3ba-3e2d-432e-89eb-9efda7430f93/genomic-extractions/66f225d3-35cf-4ff7-a4ec-3d887a86ea8b/vcfs/interval_0.vcf.gz.tbi,1682
1,gs://fc-secure-765db3ba-3e2d-432e-89eb-9efda7430f93/genomic-extractions/66f225d3-35cf-4ff7-a4ec-3d887a86ea8b/vcfs/interval_1.vcf.gz,871855,gs://fc-secure-765db3ba-3e2d-432e-89eb-9efda7430f93/genomic-extractions/66f225d3-35cf-4ff7-a4ec-3d887a86ea8b/vcfs/interval_1.vcf.gz.tbi,1670
2,gs://fc-secure-765db3ba-3e2d-432e-89eb-9efda7430f93/genomic-extractions/66f225d3-35cf-4ff7-a4ec-3d887a86ea8b/vcfs/interval_2.vcf.gz,710617,gs://fc-secure-765db3ba-3e2d-432e-89eb-9efda7430f93/genomic-extractions/66f225d3-35cf-4ff7-a4ec-3d887a86ea8b/vcfs/interval_2.vcf.gz.tbi,1629
3,gs://fc-secure-765db3ba-3e2d-432e-89eb-9efda7430f93/genomic-extractions/66f225d3-35cf-4ff7-a4ec-3d887a86ea8b/vcfs/interval_3.vcf.gz,715565,gs://fc-secure-765db3ba-3e2d-432e-89eb-9efda7430f93/genomic-extractions/66f225d3-35cf-4ff7-a4ec-3d887a86ea8b/vcfs/interval_3.vcf.gz.tbi,1645
...

calbach

Nice, thanks. We should definitely check with spec ops for their thoughts; I think this should be generally useful but there may be other things they want to see or not see here.

calbach · 2021-09-07T21:37:40Z

scripts/variantstore/wdl/GvsExtractCallset.wdl

@@ -188,16 +195,30 @@ task ExtractTask {
                ~{true='--emit-pls' false='' emit_pls} \
                ${FILTERING_ARGS}

-        du -b ~{output_file} | cut -f1 > vcf_bytes.txt
-        du -b ~{output_file}.tbi | cut -f1 > vcf_index_bytes.txt
+        INTERVAL_NUMBER=$(echo ~{output_file} | grep -oEi '[0-9]+\.vcf\.gz' | cut -d'.' -f1)


This is complicated and also breaks layers of abstraction by assuming a specific filename format. If you need this, I would add a new input parameter like interval_index

calbach · 2021-09-07T21:42:03Z

scripts/variantstore/wdl/GvsExtractCallset.wdl


        # Drop trailing slash if one exists
        OUTPUT_GCS_DIR=$(echo ~{output_gcs_dir} | sed 's/\/$//')

        if [ -n "${OUTPUT_GCS_DIR}" ]; then
          gsutil cp ~{output_file} ${OUTPUT_GCS_DIR}/
          gsutil cp ~{output_file}.tbi ${OUTPUT_GCS_DIR}/
+          OUTPUT_FILE_DEST=${OUTPUT_GCS_DIR}/~{output_file}


Not for this PR, existing issue: a lot of these variable replacements should probably be quoted. This prevents potential word splitting bugs, an possibly security vulnerabilities.

calbach · 2021-09-07T21:45:13Z

scripts/variantstore/wdl/GvsExtractCallset.wdl

+
+        # Parent Task will collect manifest lines and create a joined file
+        # Currently, the schema is `[interval_number], [output_file_location], [output_file_size_bytes], [output_file_index_location], [output_file_size_bytes]`
+        echo ${INTERVAL_NUMBER},${OUTPUT_FILE_DEST},${OUTPUT_FILE_BYTES},${OUTPUT_FILE_INDEX_DEST},${OUTPUT_FILE_INDEX_BYTES} >> manifest.txt


I'm not totally sold on this column's necessity. What are your thoughts? Is it needed in order to produce an ordered/merged manifest, i.e. for the sort -n later?

If we do need it, then I would use more generic terminology here, e.g. shard_index or output_index. I think the fact that this corresponds to a set of intervals currently is not relevant to or usable by an end consumer

I'm OK with dropping it. I mostly added it for the sort functionality but also didn't think it would hurt to add.

calbach · 2021-09-07T21:46:57Z

scripts/variantstore/wdl/GvsExtractCallset.wdl

+task CreateManifest {
+
+    input {
+        Array[String] manifest_lines


are these ordered? If so, I suppose this task could also be responsible for numbering the rows.

yeah. I wanted them to be ordered which is why I run it through sort -n. It's also why I have the index as the first column.. I can see if I can try something smarter by using a Map in WDL.

I couldn't find a way to make this work with the built in WDL functions.. I can package the interval_index and manifest line with their Pair object but I don't see a reasonable way to actually print the values out into the command block. Given that, I'm going to pass the index through as part of the manifest line like I was doing before but I am at least passing IN the index as part of the scatter call instead of parsing it from the filename.

gatk-bot · 2021-09-09T01:47:15Z

Travis reported job failures from build 35897
Failures in the following jobs:

Test Type	JDK	Job ID	Logs
integration	openjdk11	35897.12	logs

kcibul

looks reasonable to me -- question about the non-output gcs bucket path, and agree with CHs comments code/style wise

kcibul · 2021-09-09T13:57:43Z

scripts/variantstore/wdl/GvsExtractCallset.wdl

+
+        # Parent Task will collect manifest lines and create a joined file
+        # Currently, the schema is `[interval_number], [output_file_location], [output_file_size_bytes], [output_file_index_location], [output_file_size_bytes]`
+        echo ~{interval_index},${OUTPUT_FILE_DEST},${OUTPUT_FILE_BYTES},${OUTPUT_FILE_INDEX_DEST},${OUTPUT_FILE_INDEX_BYTES} >> manifest.txt


I believe in the case where there is not an OUTPUT_GCS_DIR theOUTPUT_FILE_DEST is just going to be the filename on local filesystem not in the cromwell execution gcs bucket… is that what you intend?

yeah. I could try to add the cromwell execution gcs bucket though, didn't know if that was possible. Do you know how I can get the current execution directory from a cromwell task?

@kcibul going to merge this for now so I can get this in but I can follow up

* add manifest file * proper sorting * fix regex * better comment * remove debug lines * upload manifest to GCS * pass through interval number * write as map * wdl 1.1 * use old method for sort * add quotes

ericsong added 6 commits September 3, 2021 15:34

add manifest file

9e9f06b

proper sorting

3df6992

fix regex

8b288e6

better comment

1c0f411

remove debug lines

565e85f

upload manifest to GCS

a46cac6

ericsong requested a review from calbach September 7, 2021 19:14

calbach reviewed Sep 7, 2021

View reviewed changes

ericsong added 3 commits September 8, 2021 17:31

pass through interval number

bbc2508

write as map

4187062

wdl 1.1

b611e87

use old method for sort

8231cb1

ericsong requested a review from calbach September 9, 2021 03:19

kcibul approved these changes Sep 9, 2021

View reviewed changes

add quotes

6ef3b04

ericsong merged commit d14eb3c into ah_var_store Sep 9, 2021

ericsong deleted the songe/add-manifest branch September 9, 2021 16:22

ericsong mentioned this pull request Sep 9, 2021

[risk=no][RW-7052][RW-6806] Update extraction WDL to bring in BQ label and manifest file changes all-of-us/workbench#5566

Merged

8 tasks

This was referenced Mar 17, 2023

lb merge gvs branch #8248

Closed

testing something, please ignore #8251

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add manifest summary file to GvsExtractCallset #7457

Add manifest summary file to GvsExtractCallset #7457

ericsong commented Sep 7, 2021 •

edited

Loading

calbach left a comment

calbach Sep 7, 2021

calbach Sep 7, 2021

calbach Sep 7, 2021

ericsong Sep 8, 2021

calbach Sep 7, 2021

ericsong Sep 8, 2021

ericsong Sep 9, 2021

gatk-bot commented Sep 9, 2021

kcibul left a comment

kcibul Sep 9, 2021

ericsong Sep 9, 2021

ericsong Sep 9, 2021

Add manifest summary file to GvsExtractCallset #7457

Add manifest summary file to GvsExtractCallset #7457

Conversation

ericsong commented Sep 7, 2021 • edited Loading

calbach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gatk-bot commented Sep 9, 2021

kcibul left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericsong commented Sep 7, 2021 •

edited

Loading