[AN-226] Enable submitting workflows to Cromwell's GCP Batch backend #3141

sam-schu · 2024-11-22T19:34:44Z

Updated the WorkflowSubmissionActor to submit workflows to Cromwell's GCP Batch backend, rather than to the PAPI backend, if the UseCromwellGcpBatchBackendSetting workspace setting is enabled.
- This workspace setting was added in AN-225 (PR here). It is currently exposed only by the workspace settings API, but will later be added to the Terra UI workspace settings modal under a feature flag.
- Uses a new helm config value added here.
Added the gcp_batch_gcs_root key, which takes the same value as the existing jes_gcs_root, to the execution service workflow options. This key/value pair is needed by the GCP Batch backend, and ignored by the PAPI backend.
Updated the README and the PR template to both include all 3 places where the rawls-model dependency version should be updated when a new model is published.

PR checklist

Include the JIRA issue number in the PR description and title
Make sure Swagger is updated if API changes
- ...and Orchestration's Swagger too!
If you changed anything in model/, then you should publish a new official rawls-model and update rawls-model in Orchestration's dependencies.
Get two thumbsworth of PR review
Verify all tests go green, including CI tests
Squash commits and merge to develop (branches are automatically deleted after merging)
Inform other teams of any substantial changes via Slack and/or email

model/src/main/scala/org/broadinstitute/dsde/rawls/model/ExecutionModel.scala

aednichols · 2024-12-04T19:12:18Z

core/src/test/scala/org/broadinstitute/dsde/rawls/model/ExecutionModelSpec.scala

@@ -126,6 +126,7 @@ class ExecutionModelSpec extends AnyFlatSpec with Matchers {
  "ExecutionServiceWorkflowOptions" should "serialize/deserialize to/from JSON" in {
    val test = ExecutionServiceWorkflowOptions(
      jes_gcs_root = "jes_gcs_root",
+      gcp_batch_gcs_root = "gcp_batch_gcs_root",


I am not a fan of key/value ambiguity, even for tests.

Suggested change

gcp_batch_gcs_root = "gcp_batch_gcs_root",

gcp_batch_gcs_root = "example_gcp_batch_gcs_root",

aednichols · 2024-12-04T19:13:09Z

core/src/main/scala/org/broadinstitute/dsde/rawls/jobexec/WorkflowSubmissionActor.scala

@@ -467,6 +480,7 @@ trait WorkflowSubmission extends FutureSupport with LazyLogging with MethodWiths
        monitoringImageScript = submissionRec.monitoringImageScript
      )
    } yield {
+      logger.error("Test workflow options: " + wfOpts.toString);


Remove or demote below error.

Please see my standalone comment below.

aednichols · 2024-12-04T19:19:17Z

README.md

+* **Note:** Local Rawls instances are currently unable to submit workflows to CromIAM due to an issue with the
+`caas-collection-name` label. If you are able to make a workflow submission to your local back Rawls instance and have
+it run on Cromwell, check that you did not use the dev database and accidentally allow dev Rawls to pick up your
+submission.


For now I recommend only checking in the content in the Publish rawls-model section.

I think a larger rewrite of the instructions is in order, which I intend to do in AN-297.

In particular, I don't think it makes sense to write instructions on what doesn't work, people want to know what does work.

I removed the note from lines 177-180 — I am happy to defer updates related to local workflow submission to your ticket. Did you also think I should revert my other updates other than those related to the Rawls model? I think my additional VPN reminder and additional context about using the dev database based on what we learned will help future developers not make the same mistakes that I did.

Please stick to updating the content in the Publish rawls-model section.

We will have horrible merge conflicts otherwise.

I will definitely pull you in to review that PR so you can confirm I covered everything you ran into.

That makes sense, thanks! I have reverted the other changes.

Co-authored-by: Adam Nichols <[email protected]>

jgainerdewar

This looks awesome!

jgainerdewar · 2024-12-04T20:21:05Z

core/src/main/scala/org/broadinstitute/dsde/rawls/jobexec/WorkflowSubmissionActor.scala

+      useCromwellGcpBatchBackend: Boolean = currentSettings
+        .collectFirst { case setting: UseCromwellGcpBatchBackendSetting =>
+          setting
+        }
+        .exists(_.config.enabled)


I think this would be clearer if we did it in a single invocation (further tweaking welcome, this is just an idea):

useCromwellGcpBatchBackend = currentSettings.exists { setting => setting match { case backendSetting: UseCromwellGcpBatchBackendSetting => backendSetting.config.enabled case _ => false } }

What do you mean by a single invocation?

I mean going from a collectFirst followed by a exists, to just an exists. I find it easier to read because I don't have to think about the type coming out of the collectFirst. I also like making it obvious that we are handling two different false cases - the settings exists and is set to false, or the setting doesn't exist.

we should probably create a reusable utility method to retrieve a strongly-typed workspace setting from the settings list, a la:

def findSetting[T](allSettings: List[WorkspaceSetting]): Option[T] = { allSettings.collectFirst { case found:T => found } }

if we had that, then this code could be a bit clearer, like:

useBatchSetting: Option[UseCromwellGcpBatchBackendSetting] = somwhereTheUtilityLives .findSetting[UseCromwellGcpBatchBackendSetting](currentSettings)

or

useCromwellGcpBatchBackend: Boolean = somwhereTheUtilityLives .findSetting[UseCromwellGcpBatchBackendSetting](currentSettings).exists(_.config.enabled)

you could use the same utility to calculate separateSubmissionSetting up at line 293, too.

I'd consider this a stretch goal/out of scope/does not block this PR. I don't want to scope-creep!

Thanks! @davidangb I agree that refactoring would be great for clarity and reusability in the future, but I have implemented Janet's suggestion for now.

jgainerdewar · 2024-12-04T20:23:17Z

core/src/test/scala/org/broadinstitute/dsde/rawls/jobexec/SubmissionMonitorSpec.scala

@@ -11,7 +11,10 @@ import org.broadinstitute.dsde.rawls.coordination.{DataSourceAccess, Uncoordinat
 import org.broadinstitute.dsde.rawls.dataaccess._


Are there any non-scalafmt or whitespace changes to this file? If not, can you back out these changes? It's better to not have files modified at all if we don't need to change them functionally.

I'm pretty certain that I made the commit to scalafmt this file because it was giving me an error on the scalafmt PR check. I see that that action claims to check only modified files, but maybe that file was being checked because the merge commit from dev modified it. Is there a way to revert the scalafmt changes without failing the check?

Hm, maybe reverse all of the changes so that the diff between this branch and develop includes no change to this file, and see how it feels? It sounds like that might be the state you started in, but worth going back to that so we can try again.

I was about to revert the commit that scalafmt-ed this file, but I decided to check the diff first and I couldn't see my changes anymore. It turns out that Bria must have been having the same problem, and this PR that was merged yesterday also fixed the formatting. Does it matter now whether or not I revert the commit that made the same changes on my branch?

Nope, if the diff looks good you're all set!

jgainerdewar · 2024-12-04T20:24:48Z

core/src/test/scala/org/broadinstitute/dsde/rawls/jobexec/WorkflowSubmissionSpec.scala

@@ -1202,6 +1208,90 @@ class WorkflowSubmissionSpec(_system: ActorSystem)
    }
  }

+  it should "submit workflows to Cromwell's GCP Batch backend when UseCromwellGcpBatchBackendSetting is true" in withDefaultTestDatabase {


Nice tests!

jgainerdewar · 2024-12-04T20:26:02Z

local-dev/templates/rawls.conf

-    regular = "cwb_ws_dev_v7"
-    exfiltrationControlled = "vpc_sc_v10"
+    regular = "cwb_ws_dev_v8"
+    exfiltrationControlled = "vpc_sc_v13"


Whoops, should have updated these when I switched to the new pools but I didn't know they existed!

model/src/main/scala/org/broadinstitute/dsde/rawls/model/ExecutionModel.scala

sam-schu · 2024-12-04T21:01:02Z

local-dev/templates/rawls.conf

@@ -93,8 +93,8 @@ dataRepo {

 resourceBuffer {
  projectPool {
-    regular = "cwb_ws_dev_v7"
-    exfiltrationControlled = "vpc_sc_v10"
+    regular = "cwb_ws_dev_v8"


@jgainerdewar I updated these pools in the local dev config to reflect the new pools you recently created because the old values were giving me trouble when trying to run Rawls locally. Do these new values seem correct? (The exfiltrationControlled value I was particularly unsure of.)

Yup, those look correct!

sam-schu · 2024-12-04T21:01:45Z

core/src/main/scala/org/broadinstitute/dsde/rawls/jobexec/WorkflowSubmissionActor.scala

@@ -467,6 +480,7 @@ trait WorkflowSubmission extends FutureSupport with LazyLogging with MethodWiths
        monitoringImageScript = submissionRec.monitoringImageScript
      )
    } yield {
+      logger.error("Test workflow options: " + wfOpts.toString);


I will remove this log before merging.

Looks like this is still here!

aednichols

LGTM pending resolution of README merge conflicts/duplication with #3146

davidangb

I did not test it myself, but code looks good! Minor comments inline.

davidangb · 2024-12-05T14:21:54Z

core/src/main/scala/org/broadinstitute/dsde/rawls/jobexec/WorkflowSubmissionActor.scala

+        submission.submissionRoot,
        submission.submissionRoot,


there's a good comment on ExecutionServiceWorkflowOptions that explains why we're passing the same value as the first two arguments … perhaps add a code comment here too? I was awfully confused when I first read this code, until I drilled down into ExecutionServiceWorkflowOptions

I added a clarifying comment, thanks!

davidangb · 2024-12-05T14:28:28Z

core/src/main/scala/org/broadinstitute/dsde/rawls/jobexec/WorkflowSubmissionActor.scala

+      useCromwellGcpBatchBackend: Boolean = currentSettings
+        .collectFirst { case setting: UseCromwellGcpBatchBackendSetting =>
+          setting
+        }
+        .exists(_.config.enabled)


we should probably create a reusable utility method to retrieve a strongly-typed workspace setting from the settings list, a la:

def findSetting[T](allSettings: List[WorkspaceSetting]): Option[T] = { allSettings.collectFirst { case found:T => found } }

if we had that, then this code could be a bit clearer, like:

useBatchSetting: Option[UseCromwellGcpBatchBackendSetting] = somwhereTheUtilityLives .findSetting[UseCromwellGcpBatchBackendSetting](currentSettings)

or

useCromwellGcpBatchBackend: Boolean = somwhereTheUtilityLives .findSetting[UseCromwellGcpBatchBackendSetting](currentSettings).exists(_.config.enabled)

you could use the same utility to calculate separateSubmissionSetting up at line 293, too.

I'd consider this a stretch goal/out of scope/does not block this PR. I don't want to scope-creep!

calypsomatic · 2024-12-05T14:48:43Z

core/src/main/scala/org/broadinstitute/dsde/rawls/jobexec/WorkflowSubmissionActor.scala

      executionServiceWorkflowOptions = ExecutionServiceWorkflowOptions(
+        submission.submissionRoot,


is this meant to be duplicated?

Yes! David was confused by the same thing. I added a clarifying comment as he suggested!

This reverts commit 0697b44.

This reverts commit b8bc763.

This reverts commit 25e2c4e.

jgainerdewar

Looks great, happy to approve once that last log line is removed!

sam-schu added 21 commits November 15, 2024 13:15

Add new workspace setting

1c37f55

Remove unnecessary cases

6adc2cc

Move tests for consistency

9f0a1bc

Fix new setting capitalization

e5c70f3

Update Swagger page

6493663

Merge branch 'develop' into sts_AN-225_batch_api_workspace_setting

4c21a9d

Add unit tests

0924efb

Refactor test

fb0bbc7

Minimum viable changes for new backend value

d065d7f

Merge branch 'develop' into sts_AN-226_submit_to_batch_backend

fef634e

Update local config template pool versions

bb29484

Scalafmt

60b70c7

Add additional GCS root workflow option key

1af1359

Get GCP Batch backend name from config

f33844a

Update tests

5f142a8

Merge branch 'develop' into sts_AN-226_submit_to_batch_backend

01b9f48

Add unit tests for switching backend based on setting value

6f7de22

Update out-of-date comment

02dddf7

Clarify rawls-model dependency update process

3187133

Add troubleshooting notes to README

25e2c4e

Add dev DB warning

b8bc763

sam-schu changed the title ~~[AN-226] Submit to batch backend~~ [AN-226] Submit workflows to Cromwell's GCP Batch backend when workspace setting is enabled Dec 4, 2024

sam-schu changed the title ~~[AN-226] Submit workflows to Cromwell's GCP Batch backend when workspace setting is enabled~~ [AN-226] Enable submitting workflows to Cromwell's GCP Batch backend Dec 4, 2024

lucymcnatt requested review from aednichols, LizBaldo and jgainerdewar December 4, 2024 16:07

Add comment for new workflow options key

024e36d

aednichols reviewed Dec 4, 2024

View reviewed changes

Update comment

1a4de46

Co-authored-by: Adam Nichols <[email protected]>

jgainerdewar reviewed Dec 4, 2024

View reviewed changes

sam-schu added 4 commits December 4, 2024 15:29

Update test from PR feedback

d05642c

Update comment pt. 2

4ae9be2

Remove local Rawls workflow submission note

0697b44

Merge branch 'develop' into sts_AN-226_submit_to_batch_backend

8ee7126

sam-schu commented Dec 4, 2024

View reviewed changes

sam-schu marked this pull request as ready for review December 4, 2024 21:17

sam-schu requested a review from a team as a code owner December 4, 2024 21:17

sam-schu requested review from calypsomatic and davidangb and removed request for a team December 4, 2024 21:17

aednichols approved these changes Dec 5, 2024

View reviewed changes

davidangb approved these changes Dec 5, 2024

View reviewed changes

calypsomatic reviewed Dec 5, 2024

View reviewed changes

sam-schu added 5 commits December 5, 2024 10:32

Revert "Remove local Rawls workflow submission note"

0d90360

This reverts commit 0697b44.

Revert "Add dev DB warning"

2d8bf9d

This reverts commit b8bc763.

Revert "Add troubleshooting notes to README"

9c3b890

This reverts commit 25e2c4e.

Add clarifying comment

d39caae

Rewrite backend determination code for clarity

1129375

sam-schu requested a review from jgainerdewar December 6, 2024 19:51

jgainerdewar reviewed Dec 9, 2024

View reviewed changes

Remove test log message

c854091

jgainerdewar approved these changes Dec 9, 2024

View reviewed changes

sam-schu merged commit 83bca16 into develop Dec 9, 2024
29 checks passed

sam-schu deleted the sts_AN-226_submit_to_batch_backend branch December 9, 2024 18:52

This was referenced Dec 9, 2024

[AN-226] Update Rawls model version broadinstitute/workbench-libs#1712

Merged

[AN-226] Update Rawls model version in automation #3144

Merged

[AN-226] Cromwell Google Batch API workspace setting broadinstitute/firecloud-orchestration#1503

Merged

	gcp_batch_gcs_root = "gcp_batch_gcs_root",
	gcp_batch_gcs_root = "example_gcp_batch_gcs_root",

		@@ -11,7 +11,10 @@ import org.broadinstitute.dsde.rawls.coordination.{DataSourceAccess, Uncoordinat
		import org.broadinstitute.dsde.rawls.dataaccess._

		executionServiceWorkflowOptions = ExecutionServiceWorkflowOptions(
		submission.submissionRoot,

[AN-226] Enable submitting workflows to Cromwell's GCP Batch backend #3141

[AN-226] Enable submitting workflows to Cromwell's GCP Batch backend #3141

Conversation

sam-schu commented Nov 22, 2024 • 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

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

jgainerdewar 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

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

aednichols left a comment • edited Loading

Choose a reason for hiding this comment

davidangb 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

jgainerdewar left a comment

Choose a reason for hiding this comment

sam-schu commented Nov 22, 2024 •

edited

Loading

aednichols left a comment •

edited

Loading