Release/0.18.4 rc (#611)

* Pick up GDCWholeGenomeSomaticSingleSample_v1.3.0 in sg module. (#610) * [GH-1654] Admit memory retry multiplier. (#608) * These settings all have their own columns in the executor table. * Round-trip the memoryRetryMultiplier through database. * Pass memoryRetryMultiplier through to Firecloud. * Document memoryRetryMultiplier. * Update version and CHANGLOG.md
broadinstitute · Jun 8, 2022 · 33e05c4 · 33e05c4
1 parent da4c501
commit 33e05c4
Show file tree

Hide file tree

Showing 10 changed files with 96 additions and 49 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,3 +1,7 @@
+# Release 0.18.4
+- [GH-1666] Pick up GDCWholeGenomeSomaticSingleSample_v1.3.0 in sg module. ([#610](https://github.com/broadinstitute/wfl/pull/610))
+- [GH-1654] Admit memory retry multiplier. ([#608](https://github.com/broadinstitute/wfl/pull/608))
+
 # Release 0.18.3
 - [GH-1658] Shouldn't wfl.service.rawls/batch-upsert handle Boolean attributes? ([#607](https://github.com/broadinstitute/wfl/pull/607))
 - [GH-1653] TDR snapshot direct links have changed ([#606](https://github.com/broadinstitute/wfl/pull/606))

diff --git a/api/src/wfl/executor.clj b/api/src/wfl/executor.clj
@@ -80,13 +80,15 @@
 (def ^:private ^:const terra-executor-table "TerraExecutor")
 (def ^:private ^:const terra-executor-serialized-fields
   {:workspace                  :workspace
+   :memoryRetryMultiplier      :memory_retry_multiplier
    :methodConfiguration        :method_configuration
    :methodConfigurationVersion :method_configuration_version
    :fromSource                 :from_source})
 
 ;; Specs
 (s/def ::entity ::all/uuid)
 (s/def ::fromSource string?)
+(s/def ::memoryRetryMultiplier number?)
 (s/def ::methodConfiguration (s/and string? util/terra-namespaced-name?))
 (s/def ::methodConfigurationVersion integer?)
 (s/def ::reference ::all/uuid)
@@ -97,7 +99,8 @@
                                          ::fromSource
                                          ::methodConfiguration
                                          ::all/workspace]
-                                :opt-un [::methodConfigurationVersion]))
+                                :opt-un [::memoryRetryMultiplier
+                                         ::methodConfigurationVersion]))
 
 (s/def ::terra-executor-workflow (s/keys :req-un [::entity
                                                   ::methodConfiguration
@@ -248,16 +251,16 @@
     user-comment-note
     reference
     [_type snapshot   :as _source-object]]
-   (let [{:keys [workspace methodConfiguration]} executor]
+   (let [{:keys [memoryRetryMultiplier methodConfiguration workspace]} executor]
      (update-method-configuration! workload reference snapshot)
      (log/debug "Initiating Terra submission"
-                :workload            (workloads/to-log workload)
-                :methodConfiguration methodConfiguration
-                :workspace           workspace)
+                :workload (workloads/to-log workload)
+                :executor executor)
      (let [userComment
            (create-user-comment user-comment-note workload snapshot)
            submission
-           (firecloud/submit-method workspace methodConfiguration userComment)
+           (firecloud/submit-method memoryRetryMultiplier methodConfiguration
+                                    userComment workspace)
            message
            (submission-created-slack-msg executor submission snapshot)]
        (slack/notify-watchers workload message)

diff --git a/api/src/wfl/module/sg.clj b/api/src/wfl/module/sg.clj
@@ -31,7 +31,7 @@
                                           ::all/input_cram]))
 (def workflow-wdl
   "The top-level WDL file and its version."
-  {:release "GDCWholeGenomeSomaticSingleSample_v1.1.0"
+  {:release "GDCWholeGenomeSomaticSingleSample_v1.3.0"
    :path    "pipelines/broad/dna_seq/somatic/single_sample/wgs/gdc_genome/GDCWholeGenomeSomaticSingleSample.wdl"})
 
 (defn ^:private cromwell->strings

diff --git a/api/src/wfl/service/firecloud.clj b/api/src/wfl/service/firecloud.clj
@@ -86,21 +86,23 @@
    (create-submission workspace methodconfig entity {})))
 
 (defn submit-method
-  "Submit the`methodconfig` for processing in the Terra `workspace`."
-  ([workspace methodconfig]
-   (submit-method workspace methodconfig ""))
-  ([workspace methodconfig usercomment]
-   {:pre [(every? string? [workspace methodconfig usercomment])]}
-   (let [[mcns mcn] (str/split methodconfig #"/")]
+  "Submit the`methodConfiguration` for processing in the Terra `workspace`
+  with `memoryRetryMultiplier` when not nil."
+  ([memoryRetryMultiplier methodConfiguration workspace]
+   (submit-method memoryRetryMultiplier methodConfiguration workspace ""))
+  ([memoryRetryMultiplier methodConfiguration workspace userComment]
+   {:pre [(every? string? [methodConfiguration userComment workspace])]}
+   (let [[mcns mcn] (str/split methodConfiguration #"/")
+         body       (util/unnilify
+                     {:memoryRetryMultiplier memoryRetryMultiplier
+                      :methodConfigurationNamespace mcns
+                      :methodConfigurationName mcn
+                      :useCallCache true
+                      :userComment userComment})]
      (-> (workspace-api-url workspace "submissions")
          (http/post {:headers      (auth/get-auth-header)
                      :content-type :application/json
-                     :body         (json/write-str
-                                    {:methodConfigurationNamespace mcns
-                                     :methodConfigurationName mcn
-                                     :useCallCache true
-                                     :userComment usercomment}
-                                    :escape-slash false)})
+                     :body         (json/write-str body :escape-slash false)})
          util/response-body-json))))
 
 (defn create-workspace

diff --git a/api/test/wfl/integration/executor_test.clj b/api/test/wfl/integration/executor_test.clj
@@ -15,23 +15,28 @@
   (:import [java.util UUID]
            [wfl.util UserException]))
 
-(def ^:private testing-namespace "wfl-dev")
-(def ^:private testing-workspace (str testing-namespace "/" "CDC_Viral_Sequencing"))
-(def ^:private testing-method-name "sarscov2_illumina_full")
-(def ^:private testing-method-configuration (str testing-namespace "/" testing-method-name))
+(def ^:private testing-namespace
+  "wfl-dev")
+(def ^:private testing-workspace
+  (str testing-namespace "/" "CDC_Viral_Sequencing"))
+(def ^:private testing-method-name
+  "sarscov2_illumina_full")
+(def ^:private testing-method-configuration
+  (str testing-namespace "/" testing-method-name))
 (def ^:private testing-method-configuration-version 2)
 
-(let [new-env {"WFL_FIRECLOUD_URL" "https://api.firecloud.org"
-               "WFL_RAWLS_URL"     "https://rawls.dsde-prod.broadinstitute.org"}]
-  (use-fixtures :once
-    (fixtures/temporary-environment new-env)
-    fixtures/temporary-postgresql-database))
+(use-fixtures :once
+  (fixtures/temporary-environment
+   {"WFL_FIRECLOUD_URL" "https://api.firecloud.org"
+    "WFL_RAWLS_URL"     "https://rawls.dsde-prod.broadinstitute.org"})
+  fixtures/temporary-postgresql-database)
 
 (deftest test-validate-terra-executor-with-valid-executor-request
-  (let [request {:name                "Terra"
-                 :workspace           testing-workspace
-                 :methodConfiguration testing-method-configuration
-                 :fromSource          "importSnapshot"}]
+  (let [memoryRetryMultiplier 5.23
+        request               {:name                "Terra"
+                               :workspace           testing-workspace
+                               :methodConfiguration testing-method-configuration
+                               :fromSource          "importSnapshot"}]
     (letfn [(check-mc-version [executor]
               (is executor)
               (is (== testing-method-configuration-version
@@ -43,7 +48,12 @@
       (testing "request ignores specified method configuration version"
         (-> (assoc request :methodConfigurationVersion -1)
             (#'executor/terra-executor-validate-request-or-throw)
-            check-mc-version)))))
+            check-mc-version))
+      (testing "request propagates memoryRetryMultiplier"
+        (-> request
+            (assoc :memoryRetryMultiplier memoryRetryMultiplier)
+            (#'executor/terra-executor-validate-request-or-throw)
+            :memoryRetryMultiplier (== memoryRetryMultiplier) is)))))
 
 (deftest test-validate-terra-executor-with-invalid-executor-request
   (is (thrown-with-msg?
@@ -200,11 +210,12 @@
 
 (defn ^:private create-terra-executor [id]
   (jdbc/with-db-transaction [tx (postgres/wfl-db-config)]
-    (->> {:name                "Terra"
-          :workspace           "workspace-ns/workspace-name"
-          :methodConfiguration fake-method-config
-          :fromSource          "importSnapshot"
-          :skipValidation      true}
+    (->> {:fromSource            "importSnapshot"
+          :memoryRetryMultiplier 1.23
+          :methodConfiguration   fake-method-config
+          :name                  "Terra"
+          :skipValidation        true
+          :workspace             "workspace-ns/workspace-name"}
          (executor/create-executor! tx id)
          (zipmap [:executor_type :executor_items])
          (executor/load-executor! tx))))
@@ -246,7 +257,7 @@
       (jdbc/with-db-transaction [tx (postgres/wfl-db-config)]
         (let [[running-record succeeded-record & _ :as records]
               (->> executor :details (postgres/get-table tx) (sort-by :id))
-              executor-record
+              {:keys [memory_retry_multiplier method_configuration_version]}
               (#'postgres/load-record-by-id! tx "TerraExecutor" (:id executor))
               running-workflow (running-workflow-from-submission init-submission-id)
               succeeded-workflow (succeeded-workflow-from-submission init-submission-id)]
@@ -263,8 +274,9 @@
           (is (not (stage/done? executor)) "executor should not have finished processing")
           (verify-record-against-workflow running-record running-workflow 1)
           (verify-record-against-workflow succeeded-record succeeded-workflow 2)
-          (is (== method-config-version-post-update (:method_configuration_version executor-record))
-              "Method configuration version was not incremented."))))))
+          (is (== method-config-version-post-update method_configuration_version)
+              "Method configuration version was not incremented.")
+          (is (== (:memoryRetryMultiplier executor) memory_retry_multiplier)))))))
 
 (deftest test-peek-terra-executor-queue
   (let [succeeded?    #{"Succeeded"}

diff --git a/api/test/wfl/unit/slack_test.clj b/api/test/wfl/unit/slack_test.clj
@@ -20,7 +20,7 @@
   (let [queue (conj (PersistentQueue/EMPTY)
                     (make-notification 'test-dispatch-does-not-throw))]
     (with-redefs
-     [slack/post-message #({:ok false :error "something_bad"})]
+     [slack/post-message (constantly {:ok false :error "something_bad"})]
       (is (= (seq queue) (seq (slack/dispatch-notification queue)))
           "Queue should remain when posting Slack message returns error"))
     (with-redefs

diff --git a/database/changelog.xml b/database/changelog.xml
@@ -49,4 +49,5 @@
     <include file="changesets/20220119_TerraDataRepoSourceAddLoadTagColumn.xml"       relativeToChangelogFile="true"/>
     <include file="changesets/20220125_TerraDataRepoSourceRemoveTableColumnName.xml"  relativeToChangelogFile="true"/>
     <include file="changesets/20220228_TerraExecutorNullableMethodConfigVersion.xml"  relativeToChangelogFile="true"/>
+    <include file="changesets/20220523_AddMemoryRetryMultiplierColumn.xml"            relativeToChangelogFile="true"/>
 </databaseChangeLog>
diff --git a/database/changesets/20220523_AddMemoryRetryMultiplierColumn.xml b/database/changesets/20220523_AddMemoryRetryMultiplierColumn.xml
@@ -0,0 +1,23 @@
+<?xml version="1.1" encoding="UTF-8" standalone="no"?>
+<databaseChangeLog
+        xmlns="http://www.liquibase.org/xml/ns/dbchangelog"
+        xmlns:ext="http://www.liquibase.org/xml/ns/dbchangelog-ext"
+        xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+        xsi:schemaLocation="http://www.liquibase.org/xml/ns/dbchangelog-ext
+                        http://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-ext.xsd
+                        http://www.liquibase.org/xml/ns/dbchangelog
+                        http://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-3.8.xsd">
+    <changeSet author="[email protected]"
+               dbms="postgresql"
+               id="20220523"
+               logicalFilePath="20220523_AddMemoryRetryMultiplierColumn.xml"
+               runInTransaction="false">
+        <comment>
+            Add support for Cromwell's memoryRetryMultiplier option.
+        </comment>
+        <sql>
+            ALTER TABLE TerraExecutor
+            ADD COLUMN memory_retry_multiplier REAL;
+        </sql>
+    </changeSet>
+</databaseChangeLog>
diff --git a/docs/md/staged-executor.md b/docs/md/staged-executor.md
@@ -35,6 +35,7 @@ And a real-life example for a known method configuration:
 {
   "name": "Terra",
   "workspace": "wfl-dev/CDC_Viral_Sequencing",
+  "memoryRetryMultiplier": 1.5
   "methodConfiguration": "wfl-dev/sarscov2_illumina_full",
   "fromSource": "importSnapshot"
 }
@@ -44,12 +45,13 @@ And a real-life example for a known method configuration:
 
 The table below summarises the purpose of each attribute in the above request.
 
-| Attribute             | Description                                                                                       |
-|-----------------------|---------------------------------------------------------------------------------------------------|
-| `name`                | Selects the `Terra` executor implementation.                                                      |
-| `workspace`           | Terra Workspace in which to execute workflows.                                                    |
-| `methodConfiguration` | Method configuration from which to generate submissions.                                          |
-| `fromSource`          | Instruction to coerce an output from an upstream `Source` to a type understood by this `executor`.|
+| Attribute               | Description                                                                                        |
+|-------------------------|----------------------------------------------------------------------------------------------------|
+| `name`                  | Selects the `Terra` executor implementation.                                                       |
+| `workspace`             | Terra Workspace in which to execute workflows.                                                     |
+| `memoryRetryMultiplier` | Retry OutOfMemory Cromwell tasks with memory increased by this factor.                             |
+| `methodConfiguration`   | Method configuration from which to generate submissions.                                           |
+| `fromSource`            | Instruction to coerce an output from an upstream `Source` to a type understood by this `executor`. |
 
 #### `workspace`
 A `{workspace-namespace}/{workspace-name}` string as it appears in the URL path

diff --git a/version b/version
@@ -1 +1 @@
-0.18.3
+0.18.4