-
Notifications
You must be signed in to change notification settings - Fork 11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WDL imports #43
WDL imports #43
Conversation
…ols for imports. Exceptions added: 'file://' protocol for imports; generation from graph with imports
…into import_wdl
…into import_wdl
… imports and no import's wdl presented
…into import_wdl
…into import_wdl
…into import_wdl
@ekoltsova , @TimSPb89 , Thanks for your work!
|
That looks great for me as a starting point. Thank you! @daniilsavchuk , could you please review the code once you'll have time? |
Dear @sidoruka and colleagues, Looks like the PR is too big to be reviewed just by me. Let me add @paulsmirnov to the reviewers? Thank you in advance! |
@daniilsavchuk , indeed I've counted 6 files that are somehow changed (if not counting tests and configuration changes). But if you are busy now, that's fine, I'll ask someone else to perform the review |
I reviewed part of changes in files I responsible for. The code looks quite good, but there are some slight issues (see file changes comments) |
…led SubWorkflow; name conflicts resolved in visualisation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your valuable efforts in making PB to enhance the support of WDL syntax! Import statements are quite important. However, I have a few inline comments for changed files. Please consider revising the changes.
src/app.js
Outdated
flow1 = res.model[0]; | ||
diagram.attachTo(flow1); | ||
} else { | ||
throw new Error(res.message, 'wdl parsing error'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I'm not quite sure why do you throw here and where do you catch this exception. When you decide to migrate to a promise-based solution, you should take care of errors differently.
- The second argument to Error constructor is a file name. Shouldn't you remove the erroneous argument or use a plus (
+
) instead of a comma? I think it's an old bug but I've just noticed it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
|
||
return `${res}${EOL}`; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The method genWorkflow()
completely duplicates genCall()
. Please refactor this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generation feature is not part of this PR.
src/dataServices/data-services.js
Outdated
@@ -0,0 +1,22 @@ | |||
export default function $http(method, url, data) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name of a file and its default export should match and describe their purpose. Please decide if it's a single method (then rename both) or a module (then don't use a default export and rename the method properly).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
src/parser/WDL/entities/Context.js
Outdated
@@ -27,6 +28,7 @@ export default class Context { | |||
* @param {ast} ast - Root ast tree node of parsing result | |||
*/ | |||
buildWorkflowList(ast) { | |||
if (ast.attributes.imports && ast.attributes.imports.list.length) this.hasImports = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we use a single statement per line for easier debugging and comprehension?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
src/app.js
Outdated
} else { | ||
throw new Error(res.message, 'wdl parsing error'); | ||
} | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that this block duplicates the one above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After changing error handling it's not actual anymore
src/parser/WDL/parse.js
Outdated
|
||
// check if calls're already in existing tasks | ||
calls = calls.filter(call => !tasks.includes(call)); | ||
if (!calls.length) return result; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's stick to using one statement per line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
ast = ret.ast; | ||
const ret = hermesStage(data); | ||
result.status = ret.status; | ||
result.message = ret.message; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should consider revising the approach to returning results, as we change parse()
method to an async promise-based implementation. Errors should be thrown to be caught in a .catch()
clause.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
src/parser/parse.js
Outdated
parseWDL(text, opts).then((data) => { | ||
resolve(data); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Am I right saying that with this implementation we are unable to catch exceptions raised in the
parseWDL()
function? - I'm not sure that we need a
new Promise()
here since we already have one fromparseWDL()
function.
Please test thoroughly if a user can successfully intercept all exceptions by artificially generating them in different parts of code, and then adjust the code correspondingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
src/visual/VisualWorkflow.js
Outdated
@@ -13,11 +13,15 @@ export default class VisualWorkflow extends VisualGroup { | |||
super(_.defaultsDeep(opts, { | |||
attrs: { | |||
'.label': { | |||
text: opts.step.type, | |||
text: `${opts.step.type} ${opts.step.name}`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it really necessary to see a step type on a diagram?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There can be multiple subWorkflows in real WDL examples. So it's better to show name to be able to recognise one subWorkflow from other and show it's type to recognise subWorkflow visualisation from task visualisation
test/parser/parseTest.js
Outdated
})).to.throw(Error); | ||
return parse(src, { format: 'cwl' }).catch((e) => { | ||
expect(e).to.be.an('Error'); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately the logic of the assertion has changed and it tests almost nothing now. If parse()
completes successfully the test will succeed (but it should fail). Consider using chai-as-promised
plugin for this and other tests to test promise-based interface in more intuitive manner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
return pipeline.parse(wdl, { format: 'wdl' }).model[0]; | ||
async function createFlow() { | ||
const res = await pipeline.parse(wdl); | ||
return res.model[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@paulsmirnov what do you think about compatibility with all supported browsers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I've forgotten to check if polyfills are included in the demo app build. I guess they are not, neither for Promise class, nor for Array/Object methods. We should fix this too then. Of course, library should not have polyfills included, as it is a concern of the final application. We could carefully use babel-polyfill
and adjust the docs to make user know about the steps to take.
@@ -238,6 +241,46 @@ export default class WorkflowGenerator { | |||
return res; | |||
} | |||
|
|||
genWorkflow(child) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please explain what genWorkflow does? It looks like genCall function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generation feature is not part of this PR.
@@ -25,7 +27,11 @@ export default function generate(objectModel) { | |||
actionSelector(objectModel.children); | |||
|
|||
_.forEach(actionsToBeRendered, (action) => { | |||
tasks += new TaskGenerator(action).renderTask(); | |||
if (!!action.type && action.type.toLowerCase() === 'workflow') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What a generation result?
Users are waiting that WDL script will no differ from original(only spacing) if nothing changed on the diagram after multiple generations performed.
If so, I kindly expect function that generates strings like: " "smthng"" will be implemented as another type of WDL script item class like WorkflowGenerator or TaskGenerator, but not injected to existed one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generation feature is not part of this PR.
@@ -10,6 +10,9 @@ import generateWDL from './WDL/generate'; | |||
* @returns {string} Textual representation of the workflow. | |||
*/ | |||
function generate(flow, opts = {}) { | |||
if (flow.hasImports) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it mean that declared "importing" feature not supported in fact?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generation feature is not part of this PR.
src/parser/WDL/parse.js
Outdated
} | ||
|
||
/** Parse function with WDL imports support */ | ||
async function importParsingStage(firstAst, opts) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Up to me, this function is a code copy according to existed workflow parser. It is suitable to reuse existed parser. And patch it to support multilevel name accessors like "."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need you to explain your idea more detailed
src/visual/VisualWorkflow.js
Outdated
@@ -13,11 +13,15 @@ export default class VisualWorkflow extends VisualGroup { | |||
super(_.defaultsDeep(opts, { | |||
attrs: { | |||
'.label': { | |||
text: opts.step.type, | |||
text: `${opts.step.type} ${opts.step.name}`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this solution approved by the final user? It breaks out start ideology to keep just names for steps and just types for groups
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Answered above
…ation_detailing' into import_wdl_sub-workflow_visualization_detailing # Conflicts: # src/parser/WDL/parse.js # src/parser/parse.js
Added sub workflow expanding with new examples added to PR body. And updated code according to reviews. |
General idea
Currently pipeline-builder has no support for import statements (https://software.broadinstitute.org/wdl/documentation/spec#import-statements)
The PR resolve this problem
Changes
Added support for sub workflows and import statements:
import "foo.wdl" as Foo
andimport "bar.wdl"
like imports.zip
archive file orbaseURI
(baseURI
+fileName
)http://
andhttps://
protocols support (likeimport "http://foo/bar.wdl" as Bar
)Added tests
Acceptance criteria
1st scenario:
gulp serve
to launch the demo.wdl
script in textareaLoad zip
button and choose .zip file containing .wdl import filesExample:
WDL script with import statements:
.zip archive containing files for imports:
importsTestArchive.zip
2nd scenario:
gulp serve
to launch the demo.wdl
script in textareaBuild
buttonExample:
WDL script with import statements:
baseURI:
https://raw.githubusercontent.com/broadinstitute/gatk/master/scripts/cnv_wdl
3rd scenario:
gulp serve
to launch the demo.wdl
script withhttp://
URI right in imput statement in textareaBuild
buttonExample:
WDL script with import statements:
Visualization result for first example should looks like:
![image](https://user-images.githubusercontent.com/31283705/33180070-98bb5f3e-d07c-11e7-8f43-44f6337b98af.png)
Visualization result for last two examples should looks like:
![image](https://user-images.githubusercontent.com/31283705/33180023-5e94ec76-d07c-11e7-8543-fd103c67843c.png)
4th scenario:
gulp serve
to launch the demo.wdl
script in textareaDepth of recursion
; default is 0) and sepcified sub workflows to expand according to expanding level (SubWorkflow to be detailed
; type sub workflows names as you can see it on graph comma separated or*
to expand all sub workflows)Load zip
button and choose .zip file containing .wdl import filesExample:
WDL script with import statements:
.zip archive containing files for imports:
importsTestArchive2.zip
Visualization result:
Specified parameters:
![image](https://user-images.githubusercontent.com/31283705/33728643-1d0937a6-db8c-11e7-957d-a1bf1f0470b8.png)
SubWorkflow to be detailed
-*
Depth of recursion
-1
Specified parameters:
![image](https://user-images.githubusercontent.com/31283705/33728684-3c7e6cbe-db8c-11e7-950c-ca02527a562a.png)
SubWorkflow to be detailed
-SubWorkflow2_SubWorkflow
Depth of recursion
-1
Example from Broadinstitute running on 1st scenario:
WDL script:
.zip archive containing files for imports:
broad_cnvSomaticLegacy.zip
Visualization result:
Specified parameters:
![image](https://user-images.githubusercontent.com/31693137/33763592-28d03c06-dc22-11e7-9256-785268614729.png)
SubWorkflow to be detailed
-*
Depth of recursion
-1
(Expanding all sub workflows)
Specified parameters:
![image](https://user-images.githubusercontent.com/31693137/33763727-9d8825cc-dc22-11e7-9874-eb7b403984b8.png)
SubWorkflow to be detailed
-AlleleFraction_CNVSomaticAlleleFractionPairWorkflow
Depth of recursion
-1
(Expanding only 1 sub workflow)