Feature/export cwl #19

james-strauss-uwa · 2020-08-05T15:41:07Z

Sorry, I've put this off for too long thinking I would make improvements, but decided to just create the pull request and get your feedback.

Changes:

Added a 'Export to CWL' button to the pg_viewer HTML page that is served by lg web on translation.
Added dlg.dropmake.cwl with cwl-related functionality
Create zip file containing multiple "cwl tool description" files and a single "cwl workflow" file.
Unit tests from Dave

Possible issues:

CWL only supports BashShellApp nodes. If a user exports a graph with nodes of other types, the feedback isn't great.
There are some CI tests that don't pass, but these remind me of the previous "timeout" issues you put down to messaging changes. Let me know if this is not correct.

…n makes a GET request to server for CWL file. Note: at the moment, the reply is the original JSON of the graph, not its CWL export.

…WL file for the requested graph. Note: At the moment, the CWL response is a trivial placeholder workflow with no steps.

…t. The pg_spec contains more information and I hope it is sufficient to build a CWL.

…. The N command line tool descriptions plus the workflow description file are all added to a zip file for download.

… and command line description files. Generated CWL is almost executable.

… than use the path inherited from the server filesystem.

…ecutable.

… command for cwl translator (dlg cwl).

…into feature/export-cwl

This removes these test-only dependencies from the daliuge-translator package, which otherwise doesn't needs these. There was also a conflict with the ruamel.yaml dependency as needed by cwlgen (used by daliuge-translator) and cwltool (used by the tests), where the version constraints on ruamel.yaml stated by each package are not fully exclusive, but are different enough that pip will install a version of ruamel.yaml that satisfies only one of the packages. By specifying a specific version of this package we ensure both our packages get what they need. There was a second similar issue with Travis' pip not automatically upgrading the "typing" package: version 3.6.6 comes pre-installed in the virtual environment created by Travis, and the first package to require it (cwltool) requires >= 3.5.3, and therefore the version installed is deemed as safe. A later package (typing_extensions, required by cwltool too) requires >= 3.7.4, however pip doesn't automatically upgrade the installed version. This issues an "ERROR" message by pip, which still exits with a 0 return code, and therefore the unit tests still run (and fail). Signed-off-by: Rodrigo Tobar <rtobar@icrar.org>

Signed-off-by: Rodrigo Tobar <rtobar@icrar.org>

This is different to common.b2s (always turns bytes into a str instance) and six.b (always turns a str instance into bytes): this one turns a text object (str in py3, unicode in 2.7) into a str object. One does not usually need this, but we have now a use case. Signed-off-by: Rodrigo Tobar <rtobar@icrar.org>

cwlgen doesn't know how to serialize unicode objects in python 2.7, making our tests fail. This commit fixes that by turning unicode objects into str instances before giving them to cwlgen. Signed-off-by: Rodrigo Tobar <rtobar@icrar.org>

Signed-off-by: Rodrigo Tobar <rtobar@icrar.org>

coveralls · 2020-08-05T15:49:13Z

Coverage increased (+0.2%) to 75.511% when pulling 5d29079 on feature/export-cwl into ce0be8a on master.

rtobar

Thanks James, In general I think it look really good :).

Yes, the failures are all due to the timeout issue. That's on me to fix properly on the master branch, sorry for the noise.

I left a couple of observations throughout the code, mostly around simplifying a bit the translation process, plus other minor things. Let me know your thoughts on these and whether you think it's feasible to tackle them. Thanks again!

daliuge-common/dlg/clients.py

daliuge-translator/dlg/dropmake/cwl.py

daliuge-translator/dlg/dropmake/web/lg_web.py

rtobar · 2020-08-06T08:48:38Z

daliuge-translator/dlg/translator/tool_commands.py

+    # get the pgt
+    pgt = unroll(opts.lg_path, opts.oid_prefix, zerorun=opts.zerorun, app=apps[opts.app])
+    partition(pgt, opts)


Since the CWL translation takes a physical graph as an input, I'd incline myself to reflect that better in the "dlg cwl" command. What I have in mind is to remove the unroll and partition steps, and let it accept a physical graph as input instead. This in turn makes it composable, so you can run "dlg unroll -L lg.json | dlg cwl" (one could even add a unit test for it, like the one already in test_tool.py).

Also: ideally it would be nice to follow the same idea of "the output goes directly to stdout by default" that the other commands have, but that would mean more changes to the code below so let's not do that yet. Could we however have a command-line option to specify the output file name? That should be simpler, and takes us a bit further ahead.

I like this change too. I'll change dlg cwl to accept a physical graph as input.

I guess we'll json.load() the physical graph file from the opts.pgt_path and then pass it to create_workflow(). But do we have to do anything to the JSON to make it a valid PGT to be passed to create_workflow(), perhaps parse the JSON into a PGT data structure?

Mmmmm I don't think so, but I might be wrong. Maybe give it a try and see how that goes. If it fails we can have a look at what exactly the requirement is.

Yes, it does work. The PGT loaded from JSON is a list of drops/nodes and can be passed directly to create_workflow().

But, the PGT that is used in 'lg web' is slightly different. It is retrieved from the pg_mgr using pg_mgr.get_pgt() and the list of drops/nodes is within a 'drops' attribute of the PGT. So I pass pgt.drops to create_workflow() in this case. Maybe the difference is that in this case the structure is actually a PGTP not a PGT.

I'll add the ability to specify the output filename.

Can now specify the location of the CWL output from "dlg cwl", for example:

dlg unroll -L test.graph | dlg cwl -o outputs/test.zip

If the output location is not specified, then the output will be sent to a file called "-", since that is default value of the "output" argument. We can add correct handling of stdout to the list of future work.

rtobar · 2020-08-06T08:51:12Z

daliuge-translator/test/dropmake/test_pg_gen.py

+
+        output_list = []
+
+        cwl_output = "/tmp/cwloutput/"


Not horribly important, but instead of using "/tmp" and hardcoded subdirectories there, it would be nicer to use tempfile and its methods to get temporary file/directory names, which then you'd remove at the end of the test (or otherwise the OS will automatically remove later on too).

I'll make this change too.

I've updated the code to use tempfile.mkdtemp() to create temporary directories for the CWL output and the clone of EAGLE_test_repo for input. Also, I shifted the existing use of shutil.rmtree() to delete the temporary directories to after the tests.

…eps from 'dlg cwl' tool so that 'unroll' and 'cwl' steps become composable.

…p file

…anslator

james-strauss-uwa · 2020-08-10T03:44:32Z

Thanks for your feedback. I'm happy with the changes we made here. The two issues remaining are:

Modify "dlg cwl" to handle output to stdout
Better feedback to users when they attempt to translate a graph with non-BashShellApp nodes.

I think we'll add these to technical debt. I'll add them to the YANDA Tech Debt page on confluence.

rtobar · 2020-08-10T04:29:01Z

@james-strauss-uwa the new changes look great! I'm happy for you to merge this into the master branch.

james-strauss-uwa and others added 25 commits March 20, 2020 11:17

Modify pg_viewer HTML to include a 'Export to CWL' button. This butto…

50d4e75

…n makes a GET request to server for CWL file. Note: at the moment, the reply is the original JSON of the graph, not its CWL export.

Add cwlgen to lg web. Added new route /pgt_cwl that responds with a C…

23ca8c6

…WL file for the requested graph. Note: At the moment, the CWL response is a trivial placeholder workflow with no steps.

Improved CWL exporter to have access to the pg_spec instead of the pg…

ba1d5db

…t. The pg_spec contains more information and I hope it is sufficient to build a CWL.

Command line tool description files are now created for BashShellApps…

fb44d01

…. The N command line tool descriptions plus the workflow description file are all added to a zip file for download.

Added cwlgen to list of translator dependencies

6984a29

Removed Python 3.4 from the TravisCI test matrix.

9f214ae

Adding steps to CWL workflow.

4a5e772

Further work on adding correct inputs and outputs to the CWL workflow…

e622c91

… and command line description files. Generated CWL is almost executable.

Ensure CWL files are in the root directory of the ZIP archive, rather…

8815e1b

… than use the path inherited from the server filesystem.

Fixed the output filenames for previous steps. The workflow is now ex…

ba60ead

…ecutable.

Moved CWL-related code from lgweb to dgl.dropmake.cwl. Added dlg tool…

0cf1ca3

… command for cwl translator (dlg cwl).

Add cwltool and gitpython

2c3991f

Add test for YAN-258

5e8ee34

Minor variable rename

6025b1d

Merge branch 'feature/export-cwl' of https://github.com/ICRAR/daliuge …

f4adf99

…into feature/export-cwl

Merge branch 'master' into feature/export-cwl

a5177e6

Sort dependencies alphabetically

e30258a

Signed-off-by: Rodrigo Tobar <rtobar@icrar.org>

Fix generation of workflows in 2.7

89b818e

cwlgen doesn't know how to serialize unicode objects in python 2.7, making our tests fail. This commit fixes that by turning unicode objects into str instances before giving them to cwlgen. Signed-off-by: Rodrigo Tobar <rtobar@icrar.org>

Print output from validation tool on error

39e91b3

Signed-off-by: Rodrigo Tobar <rtobar@icrar.org>

Removed some debugging messages

0f4285a

Added documentation for CWL-related methods.

b95477e

Fix for method that determines the version of graph in a file.

fdf2323

Merge remote-tracking branch 'origin/master' into feature/export-cwl

33dbc50

james-strauss-uwa requested a review from rtobar August 5, 2020 15:41

rtobar reviewed Aug 6, 2020

View reviewed changes

james-strauss-uwa added 2 commits August 7, 2020 14:04

Removed redundant SimpleManagerClient. Remove unroll and partition st…

543f600

…eps from 'dlg cwl' tool so that 'unroll' and 'cwl' steps become composable.

Use 'output' command line option to specify location of CWL output zi…

54cbb7e

…p file

Use tempfile to create temp directories used during testing of CWL tr…

5d29079

…anslator

james-strauss-uwa merged commit dae6406 into master Aug 10, 2020

rtobar deleted the feature/export-cwl branch July 7, 2021 07:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/export cwl #19

Feature/export cwl #19

james-strauss-uwa commented Aug 5, 2020

coveralls commented Aug 5, 2020 •

edited

Loading

rtobar left a comment

rtobar Aug 6, 2020

rtobar Aug 6, 2020

james-strauss-uwa Aug 7, 2020

rtobar Aug 7, 2020

james-strauss-uwa Aug 7, 2020

james-strauss-uwa Aug 7, 2020

james-strauss-uwa Aug 10, 2020 •

edited

Loading

rtobar Aug 6, 2020

james-strauss-uwa Aug 7, 2020

james-strauss-uwa Aug 10, 2020

james-strauss-uwa commented Aug 10, 2020

rtobar commented Aug 10, 2020

Feature/export cwl #19

Feature/export cwl #19

Conversation

james-strauss-uwa commented Aug 5, 2020

coveralls commented Aug 5, 2020 • edited Loading

rtobar 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

james-strauss-uwa Aug 10, 2020 • 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

james-strauss-uwa commented Aug 10, 2020

rtobar commented Aug 10, 2020

coveralls commented Aug 5, 2020 •

edited

Loading

james-strauss-uwa Aug 10, 2020 •

edited

Loading