GH-1095: Batch submit workflows to Cromwell #272

samanehsan · 2020-12-08T15:27:45Z

Purpose

https://broadinstitute.atlassian.net/browse/GH-1095

Changes

Start/exec all items in a XX or WGS workload to Cromwell using the batch endpoint, instead of sending a request for each item. The batch endpoint will apply the same labels to every workflow, so item-specific labels were removed.
Move submit-workload! to the batch module
Remove WGS module skip workflow functionality

Review Instructions

No instructions.

ehigham

Looking good! I wonder if you can share some of the logic with the xx module and move code into the batch module?

ehigham · 2020-12-08T15:56:27Z

api/src/wfl/service/cromwell.clj

+                         options
+                         labels)
+       (post-workflow (str (api environment) "/batch"))
+       (mapv #(:id %))))


Just want to check that that the order of the UUIDs returned by submit-workflows matches the order of workflows passed in?

That's what I noticed from testing, but I couldn't find anything about that specifically in the Cromwell docs. I'll double check with them in slack

So that is the behavior that the Cromwell team expects

ehigham · 2020-12-08T15:58:02Z

api/test/wfl/integration/modules/wgs_test.clj

-(defn ^:private mock-really-submit-one-workflow [& _]
-  (UUID/randomUUID))
+(defn ^:private mock-submit-workflows [& _]
+  [(UUID/randomUUID)])


Think this really needs to return as many UUIDs as sets of workflow inputs that are passed in.

Good point!

samanehsan · 2020-12-08T17:43:32Z

I wonder if you can share some of the logic with the xx module and move code into the batch module?

There's definitely a lot of similar code between the two modules! I was getting caught up on WGS "skipping" some of the workflows but I can probably move that outside of submit-workload! to make that more generalized. Is that what you were thinking @ehigham?

tbl3rd · 2020-12-08T17:06:58Z

api/src/wfl/service/cromwell.clj

@@ -189,9 +189,8 @@

 (defn make-workflow-labels
  "Return the workflow labels from ENVIRONMENT, WDL, and INPUTS."


"Return workflow labels for WDL."

tbl3rd · 2020-12-08T17:09:24Z

api/src/wfl/service/cromwell.clj

+                         options
+                         labels)
+       (post-workflow (str (api environment) "/batch"))
+       (mapv #(:id %))))


Isn't that just (mapv :id)?

tbl3rd · 2020-12-08T17:13:37Z

api/src/wfl/module/wgs.clj

  "The WDL label applied to Cromwell metadata."
-  {(keyword wfl/the-name)
-   pipeline})
+  {(keyword wfl/the-name) pipeline})


Delete those docstrings. They have been out of date for many moons.

tbl3rd · 2020-12-08T17:19:56Z

api/src/wfl/module/wgs.clj

@@ -96,30 +96,13 @@

 (defn ^:private make-cromwell-inputs
  "Return inputs for reprocessing IN-GS into OUT-GS in ENVIRONMENT."


Another dead docstring.

tbl3rd · 2020-12-08T17:21:35Z

api/src/wfl/module/wgs.clj

  (-> (util/deep-merge cram-ref hack-task-level-values)
      (util/deep-merge {:references default-references})
      (util/deep-merge (env-inputs environment))
-      (util/deep-merge workflow-inputs)
+      (util/deep-merge inputs)
      (util/prefix-keys (keyword pipeline))))


Call deep-merge once?

tbl3rd · 2020-12-08T17:24:36Z

api/src/wfl/module/wgs.clj

@@ -149,7 +132,7 @@
                                 util/do-or-nil
                                 seq))
          (processing? [in-gs]
-            (->> {:label cromwell-label :status cromwell/active-statuses}
+            (->> {:label (stringify-cromwell-label (first cromwell-labels)) :status cromwell/active-statuses}


Just inline this? Or just stringify it at top-level and be done?

tbl3rd · 2020-12-08T17:44:20Z

api/src/wfl/module/wgs.clj

@@ -161,36 +144,46 @@
          out-gs (str (all/slashify output) object)]
      (or (exists? out-gs) (processing? in-gs)))))


Make this (or (exists? out-gs) (processing? in-gs) false) and see below.

tbl3rd · 2020-12-08T17:46:19Z

api/src/wfl/module/wgs.clj

+                 #(assoc % :uuid util/uuid-nil
+                         :status "skipped"
+                         :updated (OffsetDateTime/now))
+                 skipped-workflows)


skipped-workflows -=> skipped

tbl3rd · 2020-12-08T17:46:21Z

api/src/wfl/module/wgs.clj

+                         :status "skipped"
+                         :updated (OffsetDateTime/now))
+                 skipped-workflows)
+                (mapcat submit-batch! (group-by :options  (get workflows nil))))))))


(get workflows nil) -=> workflows

tbl3rd · 2020-12-08T17:50:04Z

api/src/wfl/module/wgs.clj

+                    (map (partial make-cromwell-inputs env) workflows)
+                    (util/deep-merge default-options options)
+                    (merge cromwell-labels {:workload uuid}))))]
+      (let [workflows (group-by #(skip-workflow? env workload %) (:workflows workload))


(let [{skipped? true workflows false} (group-by #(skip-workflow? env workload %) (:workflows workload)) …
or maybe just handle the skipped workflows conditionally in line?

ehigham · 2020-12-08T19:18:40Z

I wonder if you can share some of the logic with the xx module and move code into the batch module?

There's definitely a lot of similar code between the two modules! I was getting caught up on WGS "skipping" some of the workflows but I can probably move that outside of submit-workload! to make that more generalized. Is that what you were thinking @ehigham?

Ah yes the skip-workload? gift that keeps on giving. I wonder if we remove this and consider another way of tracking inputs/outputs in the future if it's something that's really required. Maybe I'll can create a jira to come back to this at another point.

tbl3rd

Sorry. Thought I'd approved this.

jack-r-warren

Looks good to me, just one suggestion--since you're removing the workflow skipping, you can add ^:parallel to test-start-wgs-workload and test-exec-wgs-workload in wfl.system.v1-endpoint-test to have them run in parallel

ehigham reviewed Dec 8, 2020

View reviewed changes

tbl3rd changed the title ~~[1095] Batch submit workflows to Cromwell~~ GH-1095: Batch submit workflows to Cromwell Dec 8, 2020

tbl3rd self-assigned this Dec 8, 2020

tbl3rd reviewed Dec 8, 2020

View reviewed changes

samanehsan added 8 commits December 8, 2020 17:32

Use cromwell batch endpoint for xx

d503903

Fix formatting

893909d

Use cromwell batch endpoint for wgs

0efedd7

Update wgs integration tests

0d9ca9f

Fix linting errors

c33fd3e

Update wgs tests

f6fba7f

Code clean-up

db9a28c

Formatting fixes

945351f

samanehsan force-pushed the se-batch-submit-to-cromwell branch from 2ff4983 to 945351f Compare December 8, 2020 22:35

tbl3rd approved these changes Dec 8, 2020

View reviewed changes

samanehsan added 2 commits December 8, 2020 20:29

Fix typo

eb82373

Fix new wgs test

6b45714

samanehsan force-pushed the se-batch-submit-to-cromwell branch from 9fca563 to 6b45714 Compare December 9, 2020 14:43

samanehsan added 2 commits December 9, 2020 11:57

Remove WGS "skip" feature

d465dda

Move common code to batch module

afeb88c

samanehsan requested a review from ehigham December 9, 2020 22:03

jack-r-warren approved these changes Dec 10, 2020

View reviewed changes

Parallelize wgs system tests

015bee2

samanehsan merged commit 064ab00 into main Dec 10, 2020

rhiananthony deleted the se-batch-submit-to-cromwell branch March 17, 2021 15:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-1095: Batch submit workflows to Cromwell #272

GH-1095: Batch submit workflows to Cromwell #272

samanehsan commented Dec 8, 2020 •

edited

Loading

ehigham left a comment •

edited

Loading

ehigham Dec 8, 2020

samanehsan Dec 8, 2020

samanehsan Dec 8, 2020

ehigham Dec 8, 2020

samanehsan Dec 8, 2020

samanehsan commented Dec 8, 2020

tbl3rd Dec 8, 2020

tbl3rd Dec 8, 2020

tbl3rd Dec 8, 2020

tbl3rd Dec 8, 2020

tbl3rd Dec 8, 2020

tbl3rd Dec 8, 2020

tbl3rd Dec 8, 2020

tbl3rd Dec 8, 2020

tbl3rd Dec 8, 2020

tbl3rd Dec 8, 2020

ehigham commented Dec 8, 2020 •

edited

Loading

tbl3rd left a comment

jack-r-warren left a comment

		@@ -189,9 +189,8 @@

		(defn make-workflow-labels
		"Return the workflow labels from ENVIRONMENT, WDL, and INPUTS."

		@@ -96,30 +96,13 @@

		(defn ^:private make-cromwell-inputs
		"Return inputs for reprocessing IN-GS into OUT-GS in ENVIRONMENT."

		@@ -161,36 +144,46 @@
		out-gs (str (all/slashify output) object)]
		(or (exists? out-gs) (processing? in-gs)))))

GH-1095: Batch submit workflows to Cromwell #272

GH-1095: Batch submit workflows to Cromwell #272

Conversation

samanehsan commented Dec 8, 2020 • edited Loading

Purpose

Changes

Review Instructions

ehigham 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

Choose a reason for hiding this comment

samanehsan commented Dec 8, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehigham commented Dec 8, 2020 • edited Loading

tbl3rd left a comment

Choose a reason for hiding this comment

jack-r-warren left a comment

Choose a reason for hiding this comment

samanehsan commented Dec 8, 2020 •

edited

Loading

ehigham left a comment •

edited

Loading

ehigham commented Dec 8, 2020 •

edited

Loading