Workflow url in run mode #3988

salonishah11 · 2018-08-08T20:42:27Z

This PR adds the remaining functionality of submitting workflow url to run a workflow in Run mode.

Closes #3849
Closes #3986

mcovarr · 2018-08-09T15:08:40Z

docs/CommandLine.md

@@ -40,7 +41,7 @@ The Cromwell jar file can be built as described in [Building](Building).
 `run` mode executes a single workflow in Cromwell and then exits.

 * **`workflow-source`**  
-The single required argument for the workflow source file.
+The single required argument. It can either be a workflow source file or a url pointing to the workflow (The maximum length of url can be 2000 characters).


Not sure I would bother with the part between parentheses since that limit is so high it's unlikely anyone would ever hit it.

Makes sense. Will remove it 👍

mcovarr · 2018-08-09T15:12:42Z

engine/src/main/scala/cromwell/webservice/PartialWorkflowSources.scala

+    }
+
+    workflowUrl.length match {
+      case l if l > 2000 => s"Invalid workflow url. Length of the url:$l is more than 2000 characters".invalidNel


TOL something like val MaxWorkflowUrlLength = 2000 in the companion object would be nice.

TOL suggestion: Invalid workflow url: url has length $l, longer than the maximum allowed $MaxWorkflowUrlLength characters.

Good point!

mcovarr · 2018-08-09T15:59:45Z

server/src/main/scala/cromwell/CommandLineArguments.scala

      logger.info("Pre Processing Workflow...")
      lazy val preProcessedCwl = cwlPreProcessor.preProcessCwlFileToString(workflowPath, None)

      imports match {
-        case Some(explicitImports) => readOptionContent("Workflow source", workflowSource).map((_, Option(File(explicitImports.pathAsString)), workflowRoot))
-        case None => Try(preProcessedCwl.map((_, None, None)).value.unsafeRunSync())
+        case Some(_) => readContent("Workflow source", workflowSourcePath)


I'm not following what's going on here with imports. If they're present the logic reads the workflow source from the source path but ignores the imports. If the imports are missing this returns the preprocessed CWL.

I have tried to keep this CWL part of logic same as it was before workflow url was introduced. I thought it was better to keep it the same way as before.

Talked with @Horneth, and it does make sense to send CWL file path as workflow url if imports are present (in Run mode). Will change it. :)

mcovarr · 2018-08-09T16:06:55Z

server/src/main/scala/cromwell/CommandLineArguments.scala

-      case (None, Some(url)) =>  PartialWorkflowSources.convertStringToUrl(url)
-      case (Some(_), Some(_)) => "Both Workflow source and Workflow url can't be supplied".invalidNel
-      case (None, None) => "Workflow source and Workflow url needs to be supplied".invalidNel
+    def getWorkflowSourceFromPath(workflowPath: Path): ErrorOr[(Option[String], Option[String])] = {


TOL This 2-tuple of optional Strings would probably be better as a proper type, something like

case class SourceOrUrl(source: Option[String], url: Option[String])

but there are other options for representing this as well.

Makes sense! Thank you! I have implemented the case class :)

mcovarr · 2018-08-09T16:19:20Z

server/src/main/scala/cromwell/CromwellEntryPoint.scala

-          workflowSource = w,
-          workflowUrl = u,
+          workflowSource = finalWorkflowSourceAndUrl._1,
+          workflowUrl = finalWorkflowSourceAndUrl._2,


if you do the case class thing above you can unapply:

val SourceOrUrl(source, url) = { if (w.isDefined) (w, u) . . . } . . . WorkflowSingleSubmission( workflowSource = source, workflowUrl = url, . . . )

cjllanwarne

Just a few minor nit-picks

cjllanwarne · 2018-08-14T21:45:13Z

docs/CommandLine.md

@@ -40,7 +41,7 @@ The Cromwell jar file can be built as described in [Building](Building).
 `run` mode executes a single workflow in Cromwell and then exits.

 * **`workflow-source`**  
-The single required argument for the workflow source file.
+The single required argument. It can either be a workflow source file or a url pointing to the workflow.


I would say "it can be either a local path or a remote URL pointing to the workflow source file"

cjllanwarne · 2018-08-14T21:46:25Z

engine/src/main/scala/cromwell/webservice/PartialWorkflowSources.scala

+    }
+
+    workflowUrl.length match {
+      case l if l > MaxWorkflowUrlLength => s"Invalid workflow url: url has length $l, longer than the maximum allowed $MaxWorkflowUrlLength characters".invalidNel


Minor point but this whole match/case could be restructured as a simpler if (l > MaxWorkflowUrlLength) {} else {}

It sure can! 👍

cjllanwarne · 2018-08-14T21:48:53Z

server/src/main/scala/cromwell/CromwellEntryPoint.scala

@@ -175,17 +176,24 @@ object CromwellEntryPoint extends GracefulStopSupport {
    import spray.json._

    val validation = args.validateSubmission(EntryPointLogger) map {
-      case ValidSubmission(w, u, r, i, o, l, z) =>
+      case ValidSubmission(w, u, r, i, o, l, z) =>{


The { and } are redundant on a case (but maybe you want them to make things easier to read?)

Yeah I like to add braces if it make things easier to read. But, here it does not matter as there is just one case! So, will remove it.

cjllanwarne · 2018-08-14T21:50:58Z

server/src/test/scala/cromwell/CromwellCommandLineSpec.scala

+    validation.get.workflowUrl shouldBe Some(threeStep.wdl)
+  }
+
+  it should "run single when supplying workflow using url" in {


what does "run single" mean?

Run single workflow/submission. The test is meant for Run mode.

* Workflow url support in Run mode

mcovarr reviewed Aug 9, 2018

View reviewed changes

salonishah11 force-pushed the ss_workflow_url_run_mode branch from 7bdddb4 to 9c6560c Compare August 9, 2018 21:41

salonishah11 requested a review from cjllanwarne August 9, 2018 22:35

gemmalam requested a review from danbills August 14, 2018 15:37

salonishah11 added 8 commits August 14, 2018 11:56

Change in validateSubmission in run mode

32138fb

Take source or url as workdlow-source

099231d

Its all about tests-Fixed broken test and added new tests

aefa58b

Fixed broken test

5c3d5c4

Url length restriction

06f611f

Read content of WDL file if in Submit mode

59cc112

Review changes

ff2b315

Removed println

757617f

salonishah11 force-pushed the ss_workflow_url_run_mode branch from 9c6560c to 757617f Compare August 14, 2018 15:56

Fixed broken test

ff80636

cjllanwarne approved these changes Aug 14, 2018

View reviewed changes

Review changes

f7007e5

danbills approved these changes Aug 16, 2018

View reviewed changes

salonishah11 merged commit 6a0ac14 into develop Aug 16, 2018

mcovarr pushed a commit that referenced this pull request Aug 17, 2018

Workflow url in run mode (#3988)

4c5b10f

* Workflow url support in Run mode

salonishah11 deleted the ss_workflow_url_run_mode branch August 24, 2018 17:25

This was referenced Aug 5, 2019

Fix for the CWL issues with Cromwell 44 EpamLifeSciencesTeam/cromwell#7

Closed

fix for the problem with running single tasks described by CWL [BA-4969] #5104

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Workflow url in run mode #3988

Workflow url in run mode #3988

salonishah11 commented Aug 8, 2018

mcovarr Aug 9, 2018 •

edited

Loading

salonishah11 Aug 9, 2018

mcovarr Aug 9, 2018

mcovarr Aug 9, 2018

salonishah11 Aug 9, 2018

mcovarr Aug 9, 2018

salonishah11 Aug 9, 2018

salonishah11 Aug 9, 2018

mcovarr Aug 9, 2018

salonishah11 Aug 9, 2018

mcovarr Aug 9, 2018

cjllanwarne left a comment

cjllanwarne Aug 14, 2018

cjllanwarne Aug 14, 2018

salonishah11 Aug 14, 2018

cjllanwarne Aug 14, 2018

salonishah11 Aug 14, 2018

cjllanwarne Aug 14, 2018

salonishah11 Aug 14, 2018

Workflow url in run mode #3988

Workflow url in run mode #3988

Conversation

salonishah11 commented Aug 8, 2018

mcovarr Aug 9, 2018 • 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

Choose a reason for hiding this comment

cjllanwarne 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

mcovarr Aug 9, 2018 •

edited

Loading