[GH-1045] Arbitrary WGS Inputs #218

ehigham · 2020-11-09T21:18:44Z

In this change:

WGS

specify arbitrary inputs
update skip-workflow? to handle bams as well!

Database changes:

CromwellWorkflow is unreleased so we can just shove the options in (don't need a change log)
copyfile can just use the CromwellWorkflow table

API changes

combine workflowOptions and common_inputs into a "common" map :common { :inputs {} :options {}}, where options are workflowOptions renamed.
implement (extending Jack's work) for xx, wgs and copyfile

Serialisation change

we can't store and just load workflow options owing to compatibility concerns (previous workflows didn't store these). I've opted not to store the default environment workflow-options and simply merge these when we talk to cromwell. I'm happy to move this to the loader if this is a point of contention. Thoughts/ideas/opinions welcome.

Puzzles:

unsure what to do with artificial inputs like reference_fasta_prefix and how that should tie in with being able to specify any input.

Not included

doc changes will not be included in this PR. They'll be updated in a subsequent change

…loading"

jack-r-warren · 2020-11-09T21:43:01Z

Maybe when we do that followup documentation PR we should write down what the default options are for the different modules since this API no longer surfaces that information. Could also address the loading-old-workloads thing by merging the defaults in twice--once when we write to the DB and once when we submit to Cromwell. Would generally be a no-op the second time but it would provide the backwards compatibility from merging at submit-time.

ehigham · 2020-11-09T22:05:16Z

Maybe when we do that followup documentation PR we should write down what the default options are for the different modules since this API no longer surfaces that information. Could also address the loading-old-workloads thing by merging the defaults in twice--once when we write to the DB and once when we submit to Cromwell. Would generally be a no-op the second time but it would provide the backwards compatibility from merging at submit-time.

Do we want all these options to be made public? This includes service account stuff that's sort of a hack/implementation detail?
I can make all options visible, if you prefer! I'll move the behaviour into the serialisation impl if you prefer - not a big issue for me.

ehigham · 2020-11-10T14:47:28Z

api/src/wfl/module/copyfile.clj

+(defn create-copyfile-workload!
+  "Use transaction TX to add the workload described by REQUEST."
+  [tx {:keys [items common] :as request}]
+  (letfn [(merge-to-json [shared specific]
+            (json/write-str (util/deep-merge shared specific)))
+          (serialize [workflow id]
+            (-> workflow
+              (assoc :id id)
+              (update :inputs #(merge-to-json (:inputs common) %))
+              (update :options #(merge-to-json (:options common) %))))]
+    (let [[id table] (batch/add-workload-table! tx workflow-wdl request)]
+      (jdbc/insert-multi! tx table (map serialize items (range)))
+      (workloads/load-workload-for-id tx id))))


The create-workload! implementations for copyfile, wgs and xx are almost identical. In a subsequent change, we could explore reducing this technical debt. This means (in theory) we need to test only once for the "batch" workflows (or instantiate a test suite).

ehigham · 2020-11-10T14:50:09Z

api/src/wfl/module/copyfile.clj

 (defn start-copyfile-workload!
  "Use transaction TX to start _WORKLOAD."
-  [tx {:keys [cromwell items uuid] :as workload}]
-  (let [env (first (all/cromwell-environments cromwell))]
-    (letfn [(submit! [{:keys [id inputs workflow_options]}]
-              [id (submit-workflow env inputs workflow_options {:workload uuid}) "Submitted"])
-            (update! [tx [id uuid status]]
+  [tx {:keys [items uuid] :as workload}]
+  (let [env             (get-cromwell-environment workload)
+        default-options (util/make-options env)]
+    (letfn [(submit! [{:keys [id inputs options]}]
+              [id (submit-workflow env inputs
+                    (util/deep-merge default-options options)
+                    {:workload uuid})])
+            (update! [tx [id uuid]]
              (jdbc/update! tx items
-                {:updated (OffsetDateTime/now) :uuid uuid :status status}
+                {:updated (OffsetDateTime/now) :uuid uuid :status "Submitted"}
                ["id = ?" id]))]
      (run! (comp (partial update! tx) submit!) (:workflows workload))
      (jdbc/update! tx :workload
        {:started (OffsetDateTime/now)} ["uuid = ?" uuid]))))


The implementations for start-workload! still differ somewhat. This is due to the xx module using (a faked) batch submit to cromwell. In a subsequent change, we could explore moving the other modules onto that endpoint and actually implement it. Given that, maybe we can extract common functionality for these modules (c.f. create-workload!)

jack-r-warren · 2020-11-10T14:54:38Z

Do we want all these options to be made public? This includes service account stuff that's sort of a hack/implementation detail?
I can make all options visible, if you prefer! I'll move the behaviour into the serialisation impl if you prefer - not a big issue for me.

I think it would be helpful for ops to know in order for them to override stuff. Honestly I think if we are doing hacky stuff I think that's all the more reason to surface what on earth we are doing. And again we don't gotta actually return it from the endpoint I guess -- if there's stuff we really don't want to show them then maybe just documenting what we do want to show them is fine. But since they'd be able to override our hacky stuff I don't see much of a harm.
A bit of my reasoning here is that Cromwell doesn't document any way to get the existing workflow options -- you can get them if you pass /metadata through jq '.submittedFiles.options | fromjson', but that submittedFiles field isn't documented and that makes me worry that we don't have guarantees around it.

ehigham · 2020-11-10T14:55:49Z

api/src/wfl/api/workloads.clj

+(defn saved-before?
+  "Test if the `_workload` was saved before the `reference` version string."
+  [reference {:keys [version] :as _workload}]
+  (letfn [(decode [v] (map util/parse-int (str/split v #"\.")))
+          (lt? [[x & xs] [y & ys]]
+            (or (< x y) (and (== x y) (every? some? [xs ys]) (lt? xs ys))))]
+    (lt? (decode version) (decode reference))))


I think this implementation is over-complicated but I'm not sure how to simplify. Suggestions welcome.

ehigham · 2020-11-10T15:00:25Z

Do we want all these options to be made public? This includes service account stuff that's sort of a hack/implementation detail?
I can make all options visible, if you prefer! I'll move the behaviour into the serialisation impl if you prefer - not a big issue for me.

I think it would be helpful for ops to know in order for them to override stuff. Honestly I think if we are doing hacky stuff I think that's all the more reason to surface what on earth we are doing. And again we don't gotta actually return it from the endpoint I guess -- if there's stuff we really don't want to show them then maybe just documenting what we do want to show them is fine. But since they'd be able to override our hacky stuff I don't see much of a harm.
A bit of my reasoning here is that Cromwell doesn't document any way to get the existing workflow options -- you can get them if you pass /metadata through jq '.submittedFiles.options | fromjson', but that submittedFiles field isn't documented and that makes me worry that we don't have guarantees around it.

As an aside - if they don't document if, I don't think we should. (i.e. we document that you can pass workflow options, just not what they are.)

tbl3rd · 2020-11-10T16:55:55Z

api/src/wfl/api/workloads.clj

-            (update m :workflow_options #(when % (util/parse-json %))))]
+            (let [keep [:id :finished :status :updated :uuid :options]]
+              (assoc (select-keys m keep) :inputs (apply dissoc m keep))))
+          (load-options [m] (update m :options (fnil util/parse-json "null")))]


Maybe we should build that into parse-json?

There may be uses where passing nil into this is an error - building the fnil'ness into parse-json would make that harder to detect.

api/src/wfl/module/batch.clj

api/src/wfl/module/wgs.clj

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

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

api/test/wfl/tools/workloads.clj

api/test/wfl/unit/modules/xx_test.clj

database/changesets/20201029_CromwellWorkflow.xml

tbl3rd · 2020-11-10T17:19:31Z

api/src/wfl/api/workloads.clj

+(defn saved-before?
+  "Test if the `_workload` was saved before the `reference` version string."
+  [reference {:keys [version] :as _workload}]
+  (letfn [(decode [v] (map util/parse-int (str/split v #"\.")))
+          (lt? [[x & xs] [y & ys]]
+            (or (< x y) (and (== x y) (every? some? [xs ys]) (lt? xs ys))))]
+    (lt? (decode version) (decode reference))))


ehigham added 3 commits November 9, 2020 20:27

use "common" for workload-level inputs and options. fix copyfile+wgs …

be711c4

…loading"

handle reference merging properly

e67dc41

merge the reference_fasta prefixes properly and test

3e82e67

ehigham added 6 commits November 9, 2020 22:06

remove nil "options"

7bba5ae

fix copyfile after system tests

e101bca

i hate this middleware crap

41ac192

Merge branch 'master' into ehigham/GH-1045-wgs-inner-wdl-inputs

385432c

give up with spec

d8169e1

add more tests!

392c318

ehigham commented Nov 10, 2020

View reviewed changes

mark workflows as skipped if they are to be

96e35fd

ehigham mentioned this pull request Nov 10, 2020

[GH-1045] update docs for wgs + common block changes #219

Merged

tbl3rd approved these changes Nov 10, 2020

View reviewed changes

ehigham added 4 commits November 10, 2020 21:09

macroexpand

65d7cad

un-weirdify

9f5221f

don't disable sanity checking by default

7ccecca

enforce semver length

5e1f892

ehigham merged commit ff13c3a into master Nov 12, 2020

ehigham deleted the ehigham/GH-1045-wgs-inner-wdl-inputs branch November 18, 2020 19:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GH-1045] Arbitrary WGS Inputs #218

[GH-1045] Arbitrary WGS Inputs #218

ehigham commented Nov 9, 2020 •

edited

Loading

jack-r-warren commented Nov 9, 2020

ehigham commented Nov 9, 2020

ehigham Nov 10, 2020

ehigham Nov 10, 2020

jack-r-warren commented Nov 10, 2020

ehigham Nov 10, 2020 •

edited

Loading

tbl3rd Nov 10, 2020

ehigham commented Nov 10, 2020 •

edited

Loading

tbl3rd Nov 10, 2020

ehigham Nov 10, 2020

tbl3rd Nov 10, 2020

[GH-1045] Arbitrary WGS Inputs #218

[GH-1045] Arbitrary WGS Inputs #218

Conversation

ehigham commented Nov 9, 2020 • edited Loading

WGS

Database changes:

API changes

Serialisation change

Puzzles:

Not included

jack-r-warren commented Nov 9, 2020

ehigham commented Nov 9, 2020

ehigham Nov 10, 2020

Choose a reason for hiding this comment

ehigham Nov 10, 2020

Choose a reason for hiding this comment

jack-r-warren commented Nov 10, 2020

ehigham Nov 10, 2020 • edited Loading

Choose a reason for hiding this comment

tbl3rd Nov 10, 2020

Choose a reason for hiding this comment

ehigham commented Nov 10, 2020 • edited Loading

tbl3rd Nov 10, 2020

Choose a reason for hiding this comment

ehigham Nov 10, 2020

Choose a reason for hiding this comment

tbl3rd Nov 10, 2020

Choose a reason for hiding this comment

ehigham commented Nov 9, 2020 •

edited

Loading

ehigham Nov 10, 2020 •

edited

Loading

ehigham commented Nov 10, 2020 •

edited

Loading