You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@taverna.apache.org by haouech <gi...@git.apache.org> on 2018/07/11 07:56:45 UTC
[GitHub] incubator-taverna-language pull request #39: Support nested workflows by par...
GitHub user haouech opened a pull request:
https://github.com/apache/incubator-taverna-language/pull/39
Support nested workflows by parsing processes recursively
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/haouech/incubator-taverna-language cwlparser
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-taverna-language/pull/39.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #39
----
commit 70c9d4adc1b2568821b20bd1a7a9a3aeb164cf62
Author: Majdi Haouech <m....@...>
Date: 2018-06-22T10:17:21Z
Parse workflows and command line tools recursively
----
---
[GitHub] incubator-taverna-language issue #39: Support nested workflows by parsing pr...
Posted by haouech <gi...@git.apache.org>.
Github user haouech commented on the issue:
https://github.com/apache/incubator-taverna-language/pull/39
Thank you for your comments.
I added the changes that you requested. I'll work now on adding more unit-tests for the next pull request.
---
[GitHub] incubator-taverna-language pull request #39: Support nested workflows by par...
Posted by stain <gi...@git.apache.org>.
Github user stain commented on a diff in the pull request:
https://github.com/apache/incubator-taverna-language/pull/39#discussion_r204712907
--- Diff: taverna-scufl2-cwl/src/test/java/org/apache/taverna/scufl2/cwl/TestWorkflowProcess.java ---
@@ -0,0 +1,135 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.taverna.scufl2.cwl;
+
+<<<<<<< HEAD
+
+=======
+import java.io.File;
+import java.io.IOException;
+>>>>>>> 87fa1c18d2b7aa210db3d238905234bf4f52b491
+import java.util.Set;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.HashMap;
+
+import org.junit.Before;
+import org.junit.Test;
+import static org.junit.Assert.assertEquals;
+
+import org.yaml.snakeyaml.Yaml;
+
+import com.fasterxml.jackson.core.type.TypeReference;
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.node.ArrayNode;
+
+import org.apache.taverna.scufl2.api.core.Workflow;
+import org.apache.taverna.scufl2.api.core.Processor;
+import org.apache.taverna.scufl2.api.core.DataLink;
+
+import org.apache.taverna.scufl2.api.common.NamedSet;
+
+import org.apache.taverna.scufl2.api.port.InputWorkflowPort;
+import org.apache.taverna.scufl2.api.port.OutputWorkflowPort;
+import org.apache.taverna.scufl2.api.port.InputProcessorPort;
+
+<<<<<<< HEAD
+=======
+import org.apache.taverna.scufl2.api.io.WorkflowBundleIO;
+import org.apache.taverna.scufl2.api.io.WriterException;
+
+import org.apache.taverna.scufl2.api.container.WorkflowBundle;
+
+>>>>>>> 87fa1c18d2b7aa210db3d238905234bf4f52b491
--- End diff --
This is trying to submit an unmerged merge conflict. Please revert and try again.
---
[GitHub] incubator-taverna-language pull request #39: Support nested workflows by par...
Posted by stain <gi...@git.apache.org>.
Github user stain commented on a diff in the pull request:
https://github.com/apache/incubator-taverna-language/pull/39#discussion_r201984063
--- Diff: taverna-scufl2-cwl/src/main/java/org/apache/taverna/scufl2/cwl/components/CommandLineTool.java ---
@@ -0,0 +1,80 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.taverna.scufl2.cwl;
+
+import java.util.Set;
+import java.util.Map;
+import java.util.HashMap;
+
+import com.fasterxml.jackson.databind.JsonNode;
+
+import org.apache.taverna.scufl2.api.port.InputProcessorPort;
+import org.apache.taverna.scufl2.api.port.OutputProcessorPort;
+
+public class CommandLineTool implements Process {
+
+ private final static String BASE_COMMAND = "baseCommand";
+ private final static String ID = "id";
+ private final static String INPUT_BINDINDGS = "inputBinding";
+
+ private CWLParser cwlParser;
+
+ private JsonNode node;
+
+ private String baseCommand = null;
+ private Map<String, InputProcessorPort> processorInputs;
+ private Map<String, OutputProcessorPort> processorOutputs;
+
+ public CommandLineTool(JsonNode node) {
+ this.node = node;
+ this.cwlParser = new CWLParser(node);
+ this.processorInputs = new HashMap<>();
+ this.processorOutputs = new HashMap<>();
+ this.parse();
+ this.receiverPorts = new HashSet(processorInputs.values());
+ this.senderPorts = new HashSet(processorOutputs.values());
+ }
+
+ public void parse() {
+ baseCommand = node.get(BASE_COMMAND).asText();
+ parseInputs();
+ parseOutputs();
+ }
+
+ public void parseInputs() {
+ Set<PortDetail> cwlInputs = cwlParser.parseInputs();
+ for(PortDetail detail: cwlInputs) {
+ String portId = detail.getId();
+ InputProcessorPort port = new InputProcessorPort();
--- End diff --
Add a TODO to set the processor port depth from the CWL type (e.g. checking for `[]`)
---
[GitHub] incubator-taverna-language pull request #39: Support nested workflows by par...
Posted by stain <gi...@git.apache.org>.
Github user stain commented on a diff in the pull request:
https://github.com/apache/incubator-taverna-language/pull/39#discussion_r204713406
--- Diff: taverna-scufl2-cwl/workflow.t2flow.txt ---
@@ -0,0 +1,10 @@
+WorkflowBundle 'bundle1'
--- End diff --
I don't know if the workflow bundle structure format supports `#` comments for Apache license headers, but if not we will need to add an apache-rat exclude of this file to pom.xml (there should be some already)
---
[GitHub] incubator-taverna-language pull request #39: Support nested workflows by par...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-taverna-language/pull/39
---
[GitHub] incubator-taverna-language pull request #39: Support nested workflows by par...
Posted by stain <gi...@git.apache.org>.
Github user stain commented on a diff in the pull request:
https://github.com/apache/incubator-taverna-language/pull/39#discussion_r201982376
--- Diff: taverna-scufl2-cwl/src/main/java/org/apache/taverna/scufl2/cwl/Main.java ---
@@ -0,0 +1,46 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.taverna.scufl2.cwl;
+
+import org.yaml.snakeyaml.Yaml;
+
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.ObjectMapper;
+
+public class Main {
+
+ private static final String HELLO_WORLD_CWL = "/hello_world.cwl";
+ private static final String WORKFLOW_WITH_COMMAND = "/workflow_with_command.cwl";
+ private static JsonNode cwlFile;
+
+ public static void main(String[] args) {
--- End diff --
This sounds like a test class, can we move it to src/test or ideally change it to a junit @Test?
---
[GitHub] incubator-taverna-language issue #39: Support nested workflows by parsing pr...
Posted by stain <gi...@git.apache.org>.
Github user stain commented on the issue:
https://github.com/apache/incubator-taverna-language/pull/39
Sorry, as I saw Jenkins fail this test (for no reason) I merged #40 first. Would you be able to merge against `upstream/master` and fix the conflict?
BTW, Can you delete the empty `Main.java` and move the other `Main.java` to `src/test/main/**` ?
---
[GitHub] incubator-taverna-language pull request #39: Support nested workflows by par...
Posted by stain <gi...@git.apache.org>.
Github user stain commented on a diff in the pull request:
https://github.com/apache/incubator-taverna-language/pull/39#discussion_r201983894
--- Diff: taverna-scufl2-cwl/src/main/java/org/apache/taverna/scufl2/cwl/components/CommandLineTool.java ---
@@ -0,0 +1,80 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.taverna.scufl2.cwl;
+
+import java.util.Set;
+import java.util.Map;
+import java.util.HashMap;
+
+import com.fasterxml.jackson.databind.JsonNode;
+
+import org.apache.taverna.scufl2.api.port.InputProcessorPort;
+import org.apache.taverna.scufl2.api.port.OutputProcessorPort;
+
+public class CommandLineTool implements Process {
+
+ private final static String BASE_COMMAND = "baseCommand";
+ private final static String ID = "id";
+ private final static String INPUT_BINDINDGS = "inputBinding";
+
+ private CWLParser cwlParser;
+
+ private JsonNode node;
+
+ private String baseCommand = null;
+ private Map<String, InputProcessorPort> processorInputs;
+ private Map<String, OutputProcessorPort> processorOutputs;
+
+ public CommandLineTool(JsonNode node) {
+ this.node = node;
+ this.cwlParser = new CWLParser(node);
+ this.processorInputs = new HashMap<>();
+ this.processorOutputs = new HashMap<>();
+ this.parse();
+ this.receiverPorts = new HashSet(processorInputs.values());
--- End diff --
`receiverPorts` here is a good example of when the field must be created by the constructor instead. But `processorInputs` and `processorOutputs` should be made outside.
---
[GitHub] incubator-taverna-language pull request #39: Support nested workflows by par...
Posted by stain <gi...@git.apache.org>.
Github user stain commented on a diff in the pull request:
https://github.com/apache/incubator-taverna-language/pull/39#discussion_r204713856
--- Diff: taverna-scufl2-cwl/workflow.wfbundle ---
@@ -0,0 +1,10 @@
+WorkflowBundle 'bundle1'
--- End diff --
This file has extension .wfbundle but is in text format. Also can we move these to src/test/resources or whatever the files are for?
The file /workflow.vnd.taverna.scufl2.structure.txt` has a weird filename and is not even a text file. Remove.
---
[GitHub] incubator-taverna-language pull request #39: Support nested workflows by par...
Posted by stain <gi...@git.apache.org>.
Github user stain commented on a diff in the pull request:
https://github.com/apache/incubator-taverna-language/pull/39#discussion_r201984605
--- Diff: taverna-scufl2-cwl/src/main/java/org/apache/taverna/scufl2/cwl/components/WorkflowProcess.java ---
@@ -0,0 +1,232 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.taverna.scufl2.cwl;
+
+import java.io.File;
+import java.io.IOException;
+import java.util.Set;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.HashMap;
+
+
+import org.apache.taverna.scufl2.api.core.Processor;
+import org.apache.taverna.scufl2.api.core.DataLink;
+import org.apache.taverna.scufl2.api.core.Workflow;
+
+import org.apache.taverna.scufl2.api.container.WorkflowBundle;
+
+import org.apache.taverna.scufl2.api.port.InputWorkflowPort;
+import org.apache.taverna.scufl2.api.port.OutputWorkflowPort;
+import org.apache.taverna.scufl2.api.port.InputProcessorPort;
+import org.apache.taverna.scufl2.api.port.OutputProcessorPort;
+import org.apache.taverna.scufl2.api.port.SenderPort;
+import org.apache.taverna.scufl2.api.port.ReceiverPort;
+
+import org.apache.taverna.scufl2.api.io.WorkflowBundleIO;
+import org.apache.taverna.scufl2.api.io.WriterException;
+
+import com.fasterxml.jackson.databind.JsonNode;
+
+public class WorkflowProcess implements Process {
+
+ private CWLParser cwlParser;
+
+ private Map<String, InputWorkflowPort> workflowInputs;
+ private Map<String, OutputWorkflowPort> workflowOutputs;
+ private Map<String, Processor> workflowProcessors;
+ private Map<String, InputProcessorPort> processorInputs;
+ private Map<String, OutputProcessorPort> processorOutputs;
+ private Set<DataLink> dataLinks;
+
+ private Converter converter;
+
+ public WorkflowProcess(JsonNode node) {
+ cwlParser = new CWLParser(node);
+ converter = new Converter();
+ workflowInputs = new HashMap<>();
+ workflowOutputs = new HashMap<>();
+ workflowProcessors = new HashMap<>();
+ processorInputs = new HashMap<>();
+ processorOutputs = new HashMap<>();
+ dataLinks = new HashSet<>();
+ this.parse();
+ this.receiverPorts = new HashSet(workflowInputs.values());
+ this.senderPorts = new HashSet(workflowOutputs.values());
+ }
+
+ public void parse() {
+ parseInputs();
+ parseOutputs();
+ Set<Step> cwlSteps = cwlParser.parseSteps();
+ parseProcessors(cwlSteps);
+ parseDataLinks(cwlSteps);
+
+ Workflow workflow = new Workflow();
+ Set<InputWorkflowPort> inputs = new HashSet<>(workflowInputs.values());
+ Set<OutputWorkflowPort> outputs = new HashSet<>(workflowOutputs.values());
+ Set<Processor> processors = new HashSet<>(workflowProcessors.values());
+
+ workflow.setInputPorts(inputs);
+ workflow.setOutputPorts(outputs);
+ workflow.setProcessors(processors);
+ workflow.setDataLinks(dataLinks);
+
+// System.out.println(workflow);
+// writeWorkflowToFile(workflow);
+//
+// System.out.println("DEBUG WORKFLOW");
+// System.out.println(workflow.getInputPorts());
+// System.out.println(workflow.getOutputPorts());
+// System.out.println(workflow.getProcessors());
+
+ }
+
+ public void writeWorkflowToFile(Workflow workflow) {
--- End diff --
This function looks like test code, can you move it to something under src/test?
---
[GitHub] incubator-taverna-language pull request #39: Support nested workflows by par...
Posted by stain <gi...@git.apache.org>.
Github user stain commented on a diff in the pull request:
https://github.com/apache/incubator-taverna-language/pull/39#discussion_r204713920
--- Diff: taverna-scufl2-cwl/taverna-scufl2-cwl.iml ---
@@ -0,0 +1,11 @@
+<?xml version="1.0" encoding="UTF-8"?>
--- End diff --
What is this file needed for? Probably should be in .gitignore
---
[GitHub] incubator-taverna-language pull request #39: Support nested workflows by par...
Posted by stain <gi...@git.apache.org>.
Github user stain commented on a diff in the pull request:
https://github.com/apache/incubator-taverna-language/pull/39#discussion_r201983228
--- Diff: taverna-scufl2-cwl/src/main/java/org/apache/taverna/scufl2/cwl/WorkflowParser.java ---
@@ -67,6 +68,12 @@ public WorkflowParser() {
JsonNode cwlFile = mapper.valueToTree(reader.load(WorkflowParser.class.getResourceAsStream(FILE_NAME)));
this.cwlParser = new CWLParser(cwlFile);
this.converter = new Converter();
+ workflowInputs = new HashMap<>();
--- End diff --
I don't know why these collections have to be created as part of this constructor, rather move that out to each of their field definitions.
It makes sense to do it in the constructor instead if you have multiple constructors that do it differently, say by passing in values; or if it's complex objects that are intra-connected, as the order of the field constructors is easy to change by accident. But for simple `HashMap` then they can be created in any order as I don't think anything else in the constructor expects them to pre-exist.
---