Updates, HCA feedback #14

david4096 · 2018-01-10T00:13:51Z

Meant to clean up the repo and make some schema changes to accommodate recent feedback from Marcus at HCA.

Removes dead code
Begins alignment with TES (thanks @kellrott)
Improves comments and protobuf styling

Work in progress

thejmazz · 2018-01-11T22:43:58Z

with regards to changing workflow_params from google.protobuf.Struct to string:

Is there an example showing how "JSON you give a WDL executor is not typed, just strings", from here, it looks like JSON?

If it is set to string, would then stringified JSON be compatible?

tetron · 2018-01-11T22:54:19Z

Changing workflow_parameters from a structured record to a raw string with arbitrary format that has to be parsed is a step backwards in interoperability. This increases the friction involved in connecting a WES endpoint to other systems, as it imposes additional free text input/output format munging. And if the practical outcome is "everyone should use stringified JSON" then why make it a string field in the first place?

david4096 · 2018-01-12T19:31:27Z

Hi @thejmazz and @tetron! Thanks!!!! It looks like the input can be YAML or JSON, and so using an open string allows both cases.

https://github.com/openwdl/wdl/blob/master/versions/draft-2/SPEC.md#specifying-workflow-inputs-in-json

@briandoconnor @mckinsel any opinions regarding this field? I'll roll it back for now!

mckinsel · 2018-01-16T18:09:58Z

Hey everybody, thanks for the draft and comments.

I believe this suggestion started with my concern that valid values for workflow_params will vary by platform. The most pressing instance of this for me is that WDL inputs are not explicitly typed, and certain required type conversions can't be implemented easily in some platforms. After some discussion of the issue, I think we decided that this field needed to be maximally flexible.

I don't really feel too strongly about it, but I'm doubtful that there's much of an interoperability difference between an unconstrained string and unconstrained JSON.

Also, I see this change from a struct to a string in the proto file, but the swagger json already has workflow_params as a string. Is it possible for the two WES definitions to get out of sync, or am I looking in the wrong spot?

david4096 · 2018-01-17T17:08:38Z

@mckinsel sounds like a version issue, this PR will add the generated swagger to help avoid this. Sorry!

kellrott · 2018-01-17T18:38:10Z

proto/workflow_execution_service.proto

+}
+
+// Enumeration of states for a given workflow request
+enum state {


This is basically just recasting the TES state declaration ( https://github.com/ga4gh/task-execution-schemas/blob/master/task_execution.proto#L355 ) but with a different order for the values ( Initializing = 8; vs INITIALIZING = 2; ) and the string equivalents will be 'Initializing' vs 'INITIALIZING'.

kellrott · 2018-01-17T18:40:30Z

proto/workflow_execution_service.proto

+
+  // OPTIONAL
+  // A key-value map of arbitrary metadata outside the scope of the workflow_params but useful to track with this workflow request
+  map<string,string> key_values = 5;


Same concept as the TES tags ( https://github.com/ga4gh/task-execution-schemas/blob/master/task_execution.proto#L66 ) but with a different name

kellrott · 2018-01-17T18:42:42Z

proto/workflow_execution_service.proto

+  // The filesystem protocols supported by this service, currently these may include common
+  // protocols such as 'http', 'https', 'sftp', 's3', 'gs', 'file', 'synapse', or others as
+  // supported by this service.
+  repeated string supported_filesystem_protocols = 3;


We should have a discussion about syncing this concept with the one used in TES: https://github.com/ga4gh/task-execution-schemas/blob/master/task_execution.proto#L527

I created an issue #15

kellrott · 2018-01-17T18:44:39Z

proto/workflow_execution_service.proto

+
+// To execute a workflow, send a workflow request including all the details needed to begin downloading
+// and executing a given workflow.
+message workflow_request {


Protobuf style guide suggests camel case for message names ( https://developers.google.com/protocol-buffers/docs/style )

@kellrott

thanks @kellrott

@kellrott

key_values has been renamed to tags thanks @kellrott

@kellrott

thanks @kellrott !

Alter readme to reflect change and remove reference to git submodule

david4096 · 2018-01-29T19:37:34Z

Hi Everyone! I removed the work in progress title and am hoping to merge this today! It's mostly cosmetic changes that remove some dead code, improve style, and align with other GA4GH efforts. The repository should be a little easier to work with now (fewer directions, requirements).

Add requirements txt for installing cwltool Update swagger json to reflect changes

briandoconnor · 2018-01-30T01:53:29Z

@tetron I like the idea of having a standardized input JSON regardless of underlying execution system. But for right now, given that both Cromwell and CWLtool use different JSON param formats it means anyone implementing WES will need to translate from WES to the execution-specific format. It seems like we should be able to, as a community, come up with a common JSON parameterization format for CWL and WDL. Do you think that's possible? If we had that, and it happened to be the same format as the WES param JSON that would be the best case scenario.

briandoconnor · 2018-01-30T01:53:56Z

@david4096 do you have this swagger hosted somewhere? Would be great to look at this API through that interface

briandoconnor · 2018-01-30T01:56:42Z

FYI, @david4096 provided: https://david4096.github.io/workflow-execution-schemas/

briandoconnor · 2018-01-30T02:17:06Z

Some feedback @david4096 :

general
- I think we need better details in the endpoint descriptions (not a blocker for your pull request, just need this before release)
GET /ga4gh/wes/v1/service-info
- engine_versions -> workflow_engine_versions: Just to make it a little more clear
- auth_instructions_url: add this so a site can provide a URL a user goes to to log in and generate their auth token... I don't think we do anything more than this right now with auth (other than having certain endpoints that need an authorized bearer token)
- I thought the feedback from the HCA Driver was they wanted a named key-value reporting what extra params the workflow engine would take, their types, and default values? Is the approach right now to overload the tags with this?
POST /ga4gh/wes/v1/workflows
- I still would prefer workflow_params be a string until we can actually give people a JSON schema used across workflow engines. Either string or JSON I can't parse it at this point since WDL and CWL use different schemas and it's not like we're proposing any structure. I'd prefer a 1.1 version of WES to provide a common JSON format regardless workflow engine (like we do with the output for example).
- are we using tags to pass extra, optional "hints" to the workflow engine? See comment above in service-info endpoint.
- workflow_descriptor: we said this would, optionally, accept base64 encoded zip files for systems that support it. What happens if this is a zip? How will the execution system know which WDL/CWL inside the zip is to be executed? In that case do you need to pass in the path that you want to execute via the workflow_url param? Messy but meets the needs of HCA and Marcus

I think that's it. Need responses on these ASAP since we want to cut a release soon to have something stable to share with the larger Commons effort. I think you're really close though! Thanks David and everyone else that's worked hard to push this forward!!

david4096 · 2018-01-31T20:54:20Z

Thanks @briandoconnor I'll incorporate what I can from your comments and push the rest out to issues!

@briandoconnor

thanks @briandoconnor

@briandoconnor

thanks @briandoconnor!

The ServiceInfo endpoint returns default values for each settable parameter in WorkflowRequest.

david4096 · 2018-02-02T23:52:49Z

Hi everyone! I updated the OpenAPI description here to include @briandoconnor's comments. Thanks!

briandoconnor · 2018-02-02T23:54:46Z

Cool @david4096 ! I'll take a look when I get home tonight and merge

david4096 · 2018-02-03T00:03:51Z

FYI I left the workflow_params as a struct, which is a oneof an Object, ListValue, string, etc. which should support the needs of HCA.

david4096 added 3 commits January 9, 2018 08:36

Remove dead code

32e0a13

Remove reference to TES in CWLfile and proto

9a2771e

Rename to service

0f28f49

david4096 force-pushed the updates branch from 6e9b023 to 324b5f9 Compare January 10, 2018 00:46

Move service to top, comment formatting

0a7a1c9

david4096 force-pushed the updates branch from 324b5f9 to 0a7a1c9 Compare January 10, 2018 00:49

kellrott reviewed Jan 17, 2018

View reviewed changes

david4096 added 5 commits January 29, 2018 10:45

More proto styling

49c3f72

Align state messasge with TES

4bbd503

thanks @kellrott

Align tags message with TES

2a73995

key_values has been renamed to tags thanks @kellrott

Rename messages to camelcase to follow style guidelines

52762ce

thanks @kellrott !

Add script to edit move/edit swagger and change directory

8a169d7

Alter readme to reflect change and remove reference to git submodule

david4096 force-pushed the updates branch from 20699fa to 8a169d7 Compare January 29, 2018 18:56

Update readme, remove swagger docs

12ec025

david4096 force-pushed the updates branch from 08ca164 to 6f63800 Compare January 29, 2018 19:33

david4096 changed the title ~~Updates, HCA feedback (WIP)~~ Updates, HCA feedback Jan 29, 2018

Update readme

39d149d

Add requirements txt for installing cwltool Update swagger json to reflect changes

david4096 force-pushed the updates branch from 6f63800 to 39d149d Compare January 30, 2018 00:09

david4096 added 4 commits February 2, 2018 14:49

Change enginer_versions for clarity

8497809

thanks @briandoconnor

Add auth instructions URL

312f2b2

thanks @briandoconnor!

Add a field for pass additional workflow engine parameters

5c8ab3a

The ServiceInfo endpoint returns default values for each settable parameter in WorkflowRequest.

Add comments for using base64 encoding technique.

f14a2a9

briandoconnor merged commit e6962e8 into ga4gh:develop Feb 5, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updates, HCA feedback #14

Updates, HCA feedback #14

david4096 commented Jan 10, 2018 •

edited

Loading

thejmazz commented Jan 11, 2018

tetron commented Jan 11, 2018

david4096 commented Jan 12, 2018

mckinsel commented Jan 16, 2018

david4096 commented Jan 17, 2018

kellrott Jan 17, 2018

kellrott Jan 17, 2018

kellrott Jan 17, 2018

david4096 Jan 18, 2018

kellrott Jan 17, 2018

david4096 commented Jan 29, 2018

briandoconnor commented Jan 30, 2018

briandoconnor commented Jan 30, 2018

briandoconnor commented Jan 30, 2018

briandoconnor commented Jan 30, 2018

david4096 commented Jan 31, 2018

david4096 commented Feb 2, 2018

briandoconnor commented Feb 2, 2018

david4096 commented Feb 3, 2018

Updates, HCA feedback #14

Updates, HCA feedback #14

Conversation

david4096 commented Jan 10, 2018 • edited Loading

thejmazz commented Jan 11, 2018

tetron commented Jan 11, 2018

david4096 commented Jan 12, 2018

mckinsel commented Jan 16, 2018

david4096 commented Jan 17, 2018

kellrott Jan 17, 2018

Choose a reason for hiding this comment

kellrott Jan 17, 2018

Choose a reason for hiding this comment

kellrott Jan 17, 2018

Choose a reason for hiding this comment

david4096 Jan 18, 2018

Choose a reason for hiding this comment

kellrott Jan 17, 2018

Choose a reason for hiding this comment

david4096 commented Jan 29, 2018

briandoconnor commented Jan 30, 2018

briandoconnor commented Jan 30, 2018

briandoconnor commented Jan 30, 2018

briandoconnor commented Jan 30, 2018

david4096 commented Jan 31, 2018

david4096 commented Feb 2, 2018

briandoconnor commented Feb 2, 2018

david4096 commented Feb 3, 2018

david4096 commented Jan 10, 2018 •

edited

Loading