Adding the uber_monitor.py script #8268

gbggrant · 2023-03-28T20:27:24Z

Adding the uber_monitor.py script to GvsUtils.wdl
Threaded it into GvsCreateFilterSet
Included a unit test.

Included a unit test.

scripts/variantstore/wdl/GvsCreateFilterSet.wdl

scripts/variantstore/wdl/GvsUtils.wdl

scripts/vcf_site_level_filtering_wdl/JointVcfFiltering.wdl

codecov · 2023-03-28T20:48:17Z

Codecov Report

❗ No coverage uploaded for pull request base (ah_var_store@0f24625). Click here to learn what that means.
The diff coverage is n/a.

Additional details and impacted files

@@               Coverage Diff                @@
##             ah_var_store     #8268   +/-   ##
================================================
  Coverage                ?   86.097%           
  Complexity              ?     35609           
================================================
  Files                   ?      2197           
  Lines                   ?    167119           
  Branches                ?     18006           
================================================
  Hits                    ?    143884           
  Misses                  ?     16800           
  Partials                ?      6435

…rMonitor # Conflicts: # .dockstore.yml # scripts/variantstore/wdl/GvsCreateFilterSet.wdl

Run uber_monitor in GvsCreateFilterSet.wdl on all tasks.

gbggrant · 2023-04-04T19:24:56Z

scripts/variantstore/wdl/GvsUtils.wdl

-  Int disk_size = if (defined(split_intervals_disk_size_override)) then select_first([split_intervals_disk_size_override]) else 10
-  Int disk_memory = if (defined(split_intervals_mem_override)) then select_first([split_intervals_mem_override]) else 16
+  Int disk_size = select_first([split_intervals_disk_size_override, 10])
+  Int disk_memory = select_first([split_intervals_mem_override, 16])


Minor fix - noticed by miniwdl

gbggrant · 2023-04-04T19:26:03Z

scripts/vcf_site_level_filtering_wdl/JointVcfFiltering.wdl

@@ -137,6 +145,15 @@ workflow JointVcfFiltering {
 		Array[File] indels_variant_scored_vcf_index = ScoreVariantAnnotationsINDELs.output_vcf_index
 		Array[File] snps_variant_scored_vcf = ScoreVariantAnnotationsSNPs.output_vcf
 		Array[File] snps_variant_scored_vcf_index = ScoreVariantAnnotationsSNPs.output_vcf_index
+		Array[File?] monitoring_logs = flatten(


Pondering if the pattern should be that any given workflow also call and output uber_monitor's summary file?

I don't think I understand what this is asking

Should I have JointVcfFiltering itself call summarize_task_monitor_logs and generate its own report in addition to providing the outputs for the parent workflow to summarize the logs.

gbggrant · 2023-04-04T19:26:44Z

scripts/variantstore/wdl/GvsCreateFilterSet.wdl

@@ -357,10 +358,44 @@ workflow GvsCreateFilterSet {
    }
  }

+  call Utils.UberMonitor as UberMonitorItAll {


This will output a summary file for all tasks - used or not.

gbggrant · 2023-04-04T19:30:11Z

Successful VQSR Lite Run (with monitoring summary output) here
Successful VQSR Classic Run (with monitoring summary output) here

mcovarr

Minor changes to globals requested. Also both the uber script and its test could use some PEP 8 love as I expect PEP 8 to be among the validations we'll run automatically in the new repo. 🙂 IntelliJ / PyCharm should provide PEP8 warnings by default.

mcovarr · 2023-04-04T21:20:46Z

scripts/variantstore/wdl/extract/uber_monitor.py

+global MaxCpu
+global MaxMem
+global MaxMemPct
+global MaxDisk
+global MaxDiskPct


I don't think these need to be declared global here

Got rid of those

mcovarr · 2023-04-04T21:21:14Z

scripts/variantstore/wdl/extract/uber_monitor.py

+        global MaxCpu
+        MaxCpu = -100.0
+        global MaxMem
+        MaxMem = -100.0
+        global MaxMemPct
+        MaxMemPct = -100.0
+        global MaxDisk
+        MaxDisk = -100.0
+        global MaxDiskPct
+        MaxDiskPct = -100.0


couldn't the initializations happen where the variables are defined and this whole block deleted?

They need to be initialized here since they are per used per monitoring log fle

scripts/variantstore/wdl/extract/uber_monitor.py

rsasch

A nit, but I would suggest renaming "uber_monitor.py" and "test_uber_monitor.py" to better reflect what the script does, e.g. "collate_task_monitor_logs.py".

scripts/variantstore/wdl/GvsCreateFilterSet.wdl

…rMonitor

gbggrant · 2023-04-10T18:48:13Z

Added pep8 fixes, renamed script.

gbggrant · 2023-04-10T20:32:34Z

Updated:
Successful VQSR Lite Run (with monitoring summary output) here
Successful VQSR Classic Run (with monitoring summary output) here

mcovarr

a few minor issues perhaps best reviewed in mobbing

mcovarr · 2023-04-11T11:09:01Z

scripts/variantstore/wdl/GvsCreateFilterSet.wdl

@@ -398,12 +432,14 @@ task ExtractFilterTask {
  }

  String intervals_name = basename(intervals)
-
+  File monitoring_script = "gs://gvs_quickstart_storage/cromwell_monitoring_script.sh"


A bucket with quickstart in its name might not be the best place for a script that's going to be used for non-quickstart runs. Maybe gs://gvs_internal?

mcovarr · 2023-04-11T11:22:27Z

scripts/variantstore/wdl/extract/summarize_task_monitor_logs.py

+def parse_monitoring_log_file(mlog_file, output):
+    eprint(f"Parsing: {mlog_file}")
+
+    if (os.stat(mlog_file).st_size == 0):


Yay for the PEP 8 fixups, but I'm still seeing a lot of non-PEP 8 warnings in IntelliJ. e.g. on this line "Remove redundant parantheses". If/when we go to our own repo there will likely be Python linting that will error on issues like this. Happy to review in mobbing to make sure we're seeing the same thing!

scripts/variantstore/wdl/extract/summarize_task_monitor_logs.py

scripts/variantstore/wdl/extract/test_summarize_task_monitor_logs.py

mcovarr · 2023-04-11T11:56:51Z

scripts/vcf_site_level_filtering_wdl/JointVcfFiltering.wdl

@@ -137,6 +145,15 @@ workflow JointVcfFiltering {
 		Array[File] indels_variant_scored_vcf_index = ScoreVariantAnnotationsINDELs.output_vcf_index
 		Array[File] snps_variant_scored_vcf = ScoreVariantAnnotationsSNPs.output_vcf
 		Array[File] snps_variant_scored_vcf_index = ScoreVariantAnnotationsSNPs.output_vcf_index
+		Array[File?] monitoring_logs = flatten(


I don't think I understand what this is asking

…cript.

gbggrant · 2023-04-12T20:52:36Z

Okay, I think I've got most of it. Still want to move the monitoring script somewhere better.

gbggrant · 2023-04-14T11:27:54Z

After moving the monitoring script:
Successful VQSR Lite Run in AoU-land (with monitoring summary output) here
Successful VQSR Classic Run in non-AoU terra (with monitoring summary output) here

mcovarr · 2023-04-14T12:23:55Z

There are still 17 (!) references to the script in its previous location.

Is it possible to bring that number down?

…rMonitor

gbggrant · 2023-04-15T11:24:39Z

Passing integration test here

Adding the uber_monitor.py script to GvsUtils.wdl

f02c27c

Included a unit test.

gbggrant requested review from mcovarr and rsasch March 28, 2023 20:27

mcovarr requested changes Mar 28, 2023

View reviewed changes

gbggrant added 12 commits March 28, 2023 16:56

Code review updates.

15781a4

Merge remote-tracking branch 'origin/ah_var_store' into gg_VS-871_Ube…

8d74ea9

…rMonitor # Conflicts: # .dockstore.yml # scripts/variantstore/wdl/GvsCreateFilterSet.wdl

flatten, select_all, and select_first - oh my.

e76867a

Maybe this?

2f2f462

Have UberMonitor empty no inputs

efc5eda

Have UberMonitor empty no inputs

1c36ce4

Why?

dabb9c0

Wack-a-mole

457e401

Enclose the INPUTS in quotes

ddccc45

Update uber_monitor.py to handle naming of call cached logs

374deb1

Run uber_monitor in GvsCreateFilterSet.wdl on all tasks.

Make summary file a workflow output.

04ef4d0

Clean up.

2aaff12

gbggrant commented Apr 4, 2023

View reviewed changes

gbggrant requested a review from mcovarr April 4, 2023 19:30

mcovarr requested changes Apr 4, 2023

View reviewed changes

rsasch approved these changes Apr 6, 2023

View reviewed changes

scripts/variantstore/wdl/GvsCreateFilterSet.wdl Outdated Show resolved Hide resolved

gbggrant added 5 commits April 7, 2023 13:29

Some Pep-8 changes!

337ed14

Code review updates

290ed18

Resolved PEP8 issues for code review

64f434b

Merge remote-tracking branch 'origin/ah_var_store' into gg_VS-871_Ube…

ddaac91

…rMonitor

Removed unused lines.

d1eb59a

gbggrant requested a review from mcovarr April 11, 2023 02:06

mcovarr requested changes Apr 11, 2023

View reviewed changes

gbggrant added 2 commits April 11, 2023 13:03

Updates for python issues.

a3dc42d

Cleaned up python errors in the test_summarize_task_monitor_logs.py s…

6e6983e

…cript.

Move the monitoring script

9839b2a

gbggrant requested a review from mcovarr April 14, 2023 11:28

gbggrant added 3 commits April 14, 2023 08:43

More than 17.

4250ae2

Update the .dockstore.yml for integration test to run.

9084595

Merge remote-tracking branch 'origin/ah_var_store' into gg_VS-871_Ube…

fdc7d97

…rMonitor

mcovarr approved these changes Apr 15, 2023

View reviewed changes

gbggrant merged commit ce3a5c7 into ah_var_store Apr 16, 2023

gbggrant deleted the gg_VS-871_UberMonitor branch April 16, 2023 11:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding the uber_monitor.py script #8268

Adding the uber_monitor.py script #8268

gbggrant commented Mar 28, 2023

codecov bot commented Mar 28, 2023 •

edited

Loading

gbggrant Apr 4, 2023

gbggrant Apr 4, 2023

mcovarr Apr 11, 2023

gbggrant Apr 11, 2023

gbggrant Apr 4, 2023

gbggrant commented Apr 4, 2023 •

edited

Loading

mcovarr left a comment •

edited

Loading

mcovarr Apr 4, 2023

gbggrant Apr 10, 2023

mcovarr Apr 4, 2023

gbggrant Apr 10, 2023

rsasch left a comment

gbggrant commented Apr 10, 2023

gbggrant commented Apr 10, 2023 •

edited

Loading

mcovarr left a comment

mcovarr Apr 11, 2023

mcovarr Apr 11, 2023

mcovarr Apr 11, 2023

gbggrant commented Apr 12, 2023

gbggrant commented Apr 14, 2023

mcovarr commented Apr 14, 2023

gbggrant commented Apr 15, 2023

Adding the uber_monitor.py script #8268

Adding the uber_monitor.py script #8268

Conversation

gbggrant commented Mar 28, 2023

codecov bot commented Mar 28, 2023 • edited Loading

Codecov Report

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

gbggrant commented Apr 4, 2023 • edited Loading

mcovarr left a comment • edited Loading

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

rsasch left a comment

Choose a reason for hiding this comment

gbggrant commented Apr 10, 2023

gbggrant commented Apr 10, 2023 • edited Loading

mcovarr 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

gbggrant commented Apr 12, 2023

gbggrant commented Apr 14, 2023

mcovarr commented Apr 14, 2023

gbggrant commented Apr 15, 2023

codecov bot commented Mar 28, 2023 •

edited

Loading

gbggrant commented Apr 4, 2023 •

edited

Loading

mcovarr left a comment •

edited

Loading

gbggrant commented Apr 10, 2023 •

edited

Loading