Implemented funtionality to support scatter, loops, conditionals and call sorting #3

daniilsavchuk · 2017-04-10T22:11:58Z

General idea

This Pull Request introduces support for Scatter, Loop and Conditionals syntax. These features are widely used in WDL scripts hosted on the Broad Institute FireCloud service, so it seems that visualization and generation of such constructions is a must

Examples of WDL visualizations are listed below

Conditionals

WDL Specification: Conditionals

Code example:

workflow foo {
		Boolean useXTask
		if (useXTask) {
		call x
	}

		call y { input: optional_int = x.out }
}

Visual representation:

Scatter

WDL Specification: Scatter

Code example:

workflow foo {
	File inputSamples
	scatter (sample in inputSamples) {
		call x
	}
	call y {
		input: 
			GVCFs=x.GVCF
	}
}

Visual:

Loop

WDL Specification: Loops

Code example:

workflow status {
	while(true) {
		call print_status
	}
}

Visual:

Calls sorting

According to WDL specification it is necessary to sort the calls during the generation in case of order calls should be executed

Changes

Added

Model/Group.js - describing the group inherited from Step.js class
Visualizer/VisualGroup - represent the visual group

Changed

Parser/WDLWorkflow - added support for parsing substeps
Generator/WorkflowGenerator - added support for substeps generation
Visualizer/Paper - changed the preferences to enable grouping and embedding
Visualizer/Visualizer - refactored the function of links update to make possible linking the grouped element with usual
WorkflowGenerator - added sorting function that iterates througs children and select the first suitable ordered

…DL code

coveralls · 2017-04-10T22:13:47Z

Coverage increased (+2.09%) to 94.822% when pulling 1b537c9 on feature/scatter-gather-support into be6cd03 on dev.

sidoruka · 2017-04-13T11:03:18Z

@daniilsavchuk thanks a lot, this is great!
@paulsmirnov , @voidest could you please review the changes?

One remark from my side: @daniilsavchuk could you please update http://pb.opensource.epam.com/ demo application once this PR is merged?

paulsmirnov

Great job. However, I think a few changes should be made before accepting the PR.

paulsmirnov · 2017-04-13T12:16:10Z

test/parser/WDL/entities/WDLWorkflowTest.js

@@ -123,7 +196,7 @@ describe('parser/WDL/entities/WDLWorkflow', () => {
      })).to.throws(WDLParserError);
    });

-    it('throws error when workflow uses scatter', () => {
+    it('requires scatter', () => {


Did you really mean requires scatter/while/if or supports / knows / understands / whatever?

paulsmirnov · 2017-04-13T12:26:45Z

README.md

-* [Loop](https://github.com/broadinstitute/wdl/blob/develop/SPEC.md#loops) - IN DEVELOPMENT
-* [Workflow level resolution](https://github.com/broadinstitute/wdl/blob/develop/SPEC.md#workflow-level-resolution)
+* [Order when generating declarations of any step(workflow, call, if, scatter, while and task)](https://github.com/broadinstitute/wdl/blob/develop/SPEC.md#workflow-level-resolution)
+* [Declarations into the [call, if, scatter and while] blocks are not supported](https://github.com/broadinstitute/wdl/blob/develop/SPEC.md#conditionals)


Could you reformulate these long titles, please? I am unable to understand the first one completely.

I'm not a meanie, you know, but: into the -> in a / inside a

Thank you very much for this note!

Would it be suitable to write:
"Order of steps (workflow, call, if, scatter, while) in generated WDL code" for the first case?

(It is placed in the limitation paragraph)

Thank you!

Done in afde1ce commit

paulsmirnov · 2017-04-13T13:05:08Z

src/model/Group.js

+  /* Override the parent function to prohibit the port addition*/
+  // eslint-disable-next-line class-methods-use-this
+  _onActionChanged() {
+  }


Does this code really prevents port addition? I think, an Action will still hold new ports but they will be out of sync with the Step. That's not good.

Thank you!

Done in 14ae69c commit

paulsmirnov · 2017-04-13T13:07:33Z

src/generator/WDL/entities/WorkflowGenerator.js

+      scatter: this.genScatter,
+      whileloop: this.genWhile,
+      if: this.genIf,
+    };


What is the justification for the whileloop name? You've used if and scatter but not while. Consider changing the name.

Thank you so much for deep analysis of my changes, it is really helpfull for me to get so detailed review!

This name threated by this way because hermes\wdl_parser.js named this so.

Please look at the endings of the grammar rules listed below: (there are names the same to my processor names mapping array)
grammar.hgr

$while_loop = :while :lparen $e :rparen :lbrace list($wf_body_element) :rbrace -> WhileLoop(expression=$2, body=$5) $if_stmt = :if :lparen $e :rparen :lbrace list($wf_body_element) :rbrace -> If(expression=$2, body=$5) $scatter = :scatter :lparen :identifier :in $e :rparen :lbrace list($wf_body_element) :rbrace -> Scatter(item=$2, collection=$4, body=$7)

If this conduct could not be a reason of some issues related to this naming, please let me to leave it.

You converted if_stmt -> if, but while_loop -> whileloop... I still don't see the logic. I wish you would keep names clear and concise, no matter how ugly and complicated they are in hermes grammar =) Ok, go on, if you are sure.

paulsmirnov · 2017-04-13T13:09:22Z

test/model/GroupTest.js

+
+    it('throws error when try to create with ports', () => {
+      expect(() => new Group(name, type, config)).to.throw(Error);
+    });


Is a port creation disabled in such Actions? A user can create without ports and add them later. You removed a test which (incorrectly) asserts on _onActionChanged() but didn't add a correct test for the case I mention.

Thank you!

Done in 14ae69c commit

Probably the reference is broken.

voidest

Well done. Everything is almost fine. Nevertheless, I found several issues that I believe have to be resolved prior to the merge.

voidest · 2017-04-13T13:35:15Z

src/generator/WDL/entities/WorkflowGenerator.js

+    }
+
+    return `${res}${EOL}`;
+  }


Is it worth trying to add all strings into array and then just join them with EOL separator? It applies to this particular case as well as to the overall routine.
To be honest, I don't think this is a major issue, but I would like you to consider this oppotunity too.

Thank you so it is a one more great thing done by your review!

It is special case for call generating function because it could be leaded by different way (it could have the "input mapping block" or could not) and so it is impossible to place ${EOL} by way you recommened.

Let me leave it if you do not mindly?

voidest · 2017-04-13T15:20:41Z

src/visual/VisualGroup.js

+ * @private
+ */
+export default class VisualGroup extends joint.shapes.devs.Model {
+  constructor(step, opts = {}) {


Why does this class inherits joint Model instead of the VisualStep

Thank you very much for your valuable review it is making development cleaner!

In this case I did not use the inheritance because in my opinion it is not necessary, so the both classes are doing different (not at all, them looks like to be inherited but it could be a reason of additional problems related to styles, updates, and bbox functions).

If you do not mind I'll leave them both inherited from joint.shapes.devs.Model

voidest · 2017-04-13T15:23:03Z

src/visual/VisualGroup.js

+        },
+      },
+      type: 'VisualStep',
+    });


Please, check if it is OK to have two different Model classes with the same type name.

Thank you!

Done in 1b537c9 commit

voidest · 2017-04-13T15:43:58Z

test/parser/WDL/parseTest.js

+
+    expect(parse(src).status).to.equal(false);
+  });
+


Test name says that this code throws something, but there is no check that it actually does it.

Thank you!

Done in afde1ce commit

voidest · 2017-04-13T15:49:18Z

src/visual/Visualizer.js

+              bottom: 50,
+            },
+          });
+        }


Is it wrong to move this code to VisualGroup's update method?

Thank you, very strong and trully note, I followed it!

Done in d47a5df commit

coveralls · 2017-04-14T13:47:45Z

Coverage increased (+2.0%) to 94.703% when pulling dda3b6b on feature/scatter-gather-support into be6cd03 on dev.

voidest

Great job! Now everything is fine by me.

paulsmirnov

Ok, thanks.

sidoruka and others added 8 commits April 11, 2017 01:06

Scatter/Gather support initial commit

f7ccae2

Added prohibition of creating port in group

3348793

Added support of call ordering before putting it into the resulting W…

65668b8

…DL code

Added tests for new functions and classes

9738924

Fixed some logic and linter errors

8d95a78

Added more test for new features

e531d1d

Added test for meta and output blocks

d1627ab

Updated the documentation according to new API abilities

1b537c9

sidoruka requested review from voidest, paulsmirnov and sidoruka April 13, 2017 11:00

paulsmirnov requested changes Apr 13, 2017

View reviewed changes

voidest suggested changes Apr 13, 2017

View reviewed changes

daniilsavchuk added 4 commits April 14, 2017 15:11

Improved visual and layout for groups(scatter, if, while)

14ae69c

Moved port prohibition for Group to Action management

afde1ce

Expressed the wrong descriptions

d47a5df

Moved group fitting block to group update function

dda3b6b

voidest approved these changes Apr 17, 2017

View reviewed changes

paulsmirnov approved these changes Apr 17, 2017

View reviewed changes

paulsmirnov merged commit fa591e4 into dev Apr 17, 2017

paulsmirnov deleted the feature/scatter-gather-support branch April 17, 2017 11:22

sidoruka mentioned this pull request Apr 17, 2017

IF Statements parsing #5

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented funtionality to support scatter, loops, conditionals and call sorting #3

Implemented funtionality to support scatter, loops, conditionals and call sorting #3

daniilsavchuk commented Apr 10, 2017

coveralls commented Apr 10, 2017

sidoruka commented Apr 13, 2017

paulsmirnov left a comment

paulsmirnov Apr 13, 2017

paulsmirnov Apr 13, 2017

daniilsavchuk Apr 14, 2017 •

edited

Loading

daniilsavchuk Apr 14, 2017

paulsmirnov Apr 13, 2017

daniilsavchuk Apr 14, 2017

paulsmirnov Apr 13, 2017

daniilsavchuk Apr 14, 2017

paulsmirnov Apr 17, 2017

paulsmirnov Apr 13, 2017

daniilsavchuk Apr 14, 2017

paulsmirnov Apr 17, 2017

voidest left a comment

voidest Apr 13, 2017

daniilsavchuk Apr 14, 2017

voidest Apr 13, 2017

daniilsavchuk Apr 14, 2017

voidest Apr 13, 2017

daniilsavchuk Apr 14, 2017 •

edited

Loading

voidest Apr 13, 2017

daniilsavchuk Apr 14, 2017 •

edited

Loading

voidest Apr 13, 2017

daniilsavchuk Apr 14, 2017 •

edited

Loading

coveralls commented Apr 14, 2017

voidest left a comment

paulsmirnov left a comment

Implemented funtionality to support scatter, loops, conditionals and call sorting #3

Implemented funtionality to support scatter, loops, conditionals and call sorting #3

Conversation

daniilsavchuk commented Apr 10, 2017

General idea

Conditionals

Scatter

Loop

Calls sorting

Changes

coveralls commented Apr 10, 2017

sidoruka commented Apr 13, 2017

paulsmirnov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daniilsavchuk Apr 14, 2017 • 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

voidest 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

daniilsavchuk Apr 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daniilsavchuk Apr 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daniilsavchuk Apr 14, 2017 • edited Loading

Choose a reason for hiding this comment

coveralls commented Apr 14, 2017

voidest left a comment

Choose a reason for hiding this comment

paulsmirnov left a comment

Choose a reason for hiding this comment

daniilsavchuk Apr 14, 2017 •

edited

Loading

daniilsavchuk Apr 14, 2017 •

edited

Loading

daniilsavchuk Apr 14, 2017 •

edited

Loading

daniilsavchuk Apr 14, 2017 •

edited

Loading