Clarify logs (expected to be URLs) and paging (use page_token). #30

tetron · 2018-06-05T19:27:29Z

Also clarify that timestamps should be ISO 8601.

Follow up from Toronto meeting.

Corresponding implementation here common-workflow-language/workflow-service#21

paging @geoffjentry @mckinsel @jaeddy @briandoconnor @dglazer

Also clarify that timestamps should be ISO 8601.

dglazer · 2018-06-05T19:43:11Z

openapi/workflow_execution_service.swagger.yaml

            page of results.
          in: query
          required: false
          type: string
        - name: tag_search
          description: |-
            OPTIONAL
-            For each key, if the key's value is empty string then match workflows that are tagged with
-            this key regardless of value.
+            Only return workflow submissions where this key is present in "tags" (regardless of tag value).


making sure I understand:
a) we let you search for "key K exists", but don't provide any way to search for "key K has value V"
b) we let you search for a single key, but don't provide any way to search for "key J and/or K exist"
Is that right? If so, it seems like a reasonable place to start, but I could be talked into doing more or less depending on what our initial driver projects need.

I don't have strong feelings about this, I was just trying to reword it to make it clearer what behavior was being specified.

I don't care strongly either way but without claiming to speak for @mckinsel - what we've seen from the group he works with using our stuff is they want both K and K/V pair as K is sometimes used as a straight tag and K/V pair is used as a, well, K/V pair.

If we need to support both search for "K exists" and "K with value" exists, then I think tag_search needs to allow specification of that. For example, tag_search is structure that contains "key" field (required if tag_search is specified) and "value" field (optional; if not present we only check key existence.

From my naive point of view, this seems a bit ornate. I'd like to understand the use case for "key + value" and why a simple set of string tags is not sufficient.

It'd also be helpful to clarify how these tags are related (if at all) to the "tags" field of WorkflowRequest:

tags: type: object additionalProperties: type: string title: |- OPTIONAL A key-value map of arbitrary metadata outside the scope of the workflow_params but useful to track with this workflow request

Meta comment: user kbergin made a comment and it’s been listed as pending for a few hours. Who can unblock this?

@tetron do you have the ability to approve the comment from @kbergin ?

https://help.github.com/articles/reviewing-proposed-changes-in-a-pull-request/

Before you submit your review, your line comments are pending and only visible to you.

That might be what's going on?

Oh I bet you’re right. I was going to say, I’d never seen a setting requiring comment approvals.

So sorry for the meta-comment confusion!

geoffjentry · 2018-06-05T20:50:21Z

👍 modulo if any changes are requested by drivers on the K/V issue

ddietterich · 2018-06-06T13:28:48Z

openapi/workflow_execution_service.swagger.yaml

@@ -75,24 +75,24 @@ paths:
        - name: page_size
          description: |-
            OPTIONAL
-            Number of workflows to return in a page.
+            The preferred number of workflow submissions to return in a page.
+            The actual number of items returned is implementation dependent.


I think it is important for clients to know the maximum number of workflows they will need to handle in a response. That affects structure allocation. The number could be fewer, such as at the end of the listing, but shouldn't be more.

ddietterich · 2018-06-06T13:30:58Z

openapi/workflow_execution_service.swagger.yaml

@@ -44,7 +44,7 @@ paths:
  /workflows:
    get:
      summary: |-
-        List the workflows, this endpoint will list the workflows in order of oldest to newest.
+        List submitted workflows.  The ordering of this list is implementation dependent.


The implementation really has to provide some kind of ordering; maybe implementation-specific. Otherwise, the semantic below of returning to the "first" result has no meaning.

ddietterich · 2018-06-06T13:35:52Z

openapi/workflow_execution_service.swagger.yaml

            page of results.
          in: query
          required: false
          type: string
        - name: tag_search
          description: |-
            OPTIONAL
-            For each key, if the key's value is empty string then match workflows that are tagged with
-            this key regardless of value.
+            Only return workflow submissions where this key is present in "tags" (regardless of tag value).


If we need to support both search for "K exists" and "K with value" exists, then I think tag_search needs to allow specification of that. For example, tag_search is structure that contains "key" field (required if tag_search is specified) and "value" field (optional; if not present we only check key existence.

From my naive point of view, this seems a bit ornate. I'd like to understand the use case for "key + value" and why a simple set of string tags is not sufficient.

tetron · 2018-06-07T19:52:28Z

I'm going to pull the text changes related to tags (I was only trying to clarify what was there and not propose new behavior, but clearly we need to talk about it more) and we can discuss it separately.

Be more precise based on feedback. Revert text changes related to tags, will be brought up in a separate PR.

tetron · 2018-06-07T20:10:03Z

Updated to remove text changes related tags and to reflect comments from @ddietterich

jaeddy

A few questions/clarifications, but otherwise looks fine to me.

jaeddy · 2018-06-11T18:02:17Z

openapi/workflow_execution_service.swagger.yaml

          in: query
          required: false
          type: integer
          format: int64
        - name: page_token
          description: |-
            OPTIONAL
-            Token to use to indicate where to start getting results. If unspecified, returns the first
+            Token to use to indicate where to start getting results. If unspecified, return the first
            page of results.
          in: query
          required: false


Does "page_token" need a type? I assume string (to match "next_page_token").

It does have type: string, I think you just missed it (it is on line 100, which is just below the context provided for this line comment).

jaeddy · 2018-06-11T18:09:46Z

openapi/workflow_execution_service.swagger.yaml

+            than "page_size", but it may return fewer.  Clients should
+            not assume that if fewer than "page_size" items is
+            returned that all items have been returned.  The
+            availability of additional pages is indicated by the value


Is there a "default" behavior for returning workflows when neither "page_size" or "page_token" are specified? Does the WES implementation itself specify a default page size?

Also, if 'Clients should not assume that if fewer than "page_size" items is returned that all items have been returned,' is the "system_state_counts" property in ServiceInfo a more reliable way to check the total count of submitted workflows?

I guess it isn't clear, the intended default behavior is to start on the first page and the page size is arbitrary.

jaeddy

Some points that could use a bit more clarification, but otherwise looks good.

jaeddy · 2018-06-15T19:10:41Z

openapi/workflow_execution_service.swagger.yaml

      stdout:
        type: string
-        title: Sample of stdout (not guaranteed to be entire log)
+        title: |-
+          A URL to retrieve standard output logs of the workflow run or


Might be useful to provide client some indication of what protocol is required to access log URLs (e.g., http, gs, s3, etc.). I'm thinking that supported_filesystem_protocols in ServiceInfo should be a map instead of an array (e.g., {"workflows": ["http", "file"], "tasks": ["gs"], "logs": ["gs"]}) — but I'll open a separate issue to discuss.

tetron · 2018-06-18T14:47:26Z

In terms of process, how do we wrap this up? I see approval from @geoffjentry and @jaeddy so I guess @mckinsel is supposed to also weigh in?

dglazer · 2018-06-18T15:06:00Z

Yes in terms of process -- Marcus, can you either say +1 or let us know your concerns? (Or choose to defer to the other reviewers, which is also fine.)

…

On Mon, Jun 18, 2018 at 7:47 AM Peter Amstutz ***@***.***> wrote: In terms of process, how do we wrap this up? I see approval from @geoffjentry <https://github.com/geoffjentry> and @jaeddy <https://github.com/jaeddy> so I guess @mckinsel <https://github.com/mckinsel> is supposed to also weigh in? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#30 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFjg3aQl67QWGqyUQzW2jo-3GAiJT7vLks5t972DgaJpZM4Ubau1> .

mckinsel

Yep, looks good to me.

Clarify logs (expected to be URLs) and paging (use page_token).

27b1701

Also clarify that timestamps should be ISO 8601.

tetron requested review from mckinsel, geoffjentry and briandoconnor June 5, 2018 19:41

dglazer reviewed Jun 5, 2018

View reviewed changes

ddietterich reviewed Jun 6, 2018

View reviewed changes

jaeddy mentioned this pull request Jun 6, 2018

Rename misleading "workflows" endpoints as "workflow-runs" #32

Merged

Update description for listing workflows and paging.

9835fdd

Be more precise based on feedback. Revert text changes related to tags, will be brought up in a separate PR.

tetron mentioned this pull request Jun 7, 2018

Replace tag_search with tag_key and tag_value. #33

Closed

This was referenced Jun 11, 2018

Add a limit to paging #40

Closed

URLs for stderr/stdout #41

Closed

jaeddy reviewed Jun 11, 2018

View reviewed changes

jaeddy approved these changes Jun 15, 2018

View reviewed changes

mckinsel approved these changes Jun 18, 2018

View reviewed changes

geoffjentry merged commit d1cb6aa into develop Jun 18, 2018

geoffjentry deleted the logs-and-paging branch June 18, 2018 17:10

geoffjentry mentioned this pull request Jul 2, 2018

Implement correct paging protocol in WES2Cromwell broadinstitute/cromwell#3843

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clarify logs (expected to be URLs) and paging (use page_token). #30

Clarify logs (expected to be URLs) and paging (use page_token). #30

tetron commented Jun 5, 2018

dglazer Jun 5, 2018

tetron Jun 5, 2018

geoffjentry Jun 5, 2018

ddietterich Jun 6, 2018

jaeddy Jun 6, 2018

geoffjentry Jun 6, 2018

geoffjentry Jun 7, 2018

tetron Jun 7, 2018

geoffjentry Jun 7, 2018

kbergin Jun 7, 2018

geoffjentry commented Jun 5, 2018

ddietterich Jun 6, 2018

ddietterich Jun 6, 2018

ddietterich Jun 6, 2018

tetron commented Jun 7, 2018

tetron commented Jun 7, 2018

jaeddy left a comment

jaeddy Jun 11, 2018

tetron Jun 18, 2018

jaeddy Jun 11, 2018

tetron Jun 18, 2018

jaeddy left a comment

jaeddy Jun 15, 2018

tetron commented Jun 18, 2018

dglazer commented Jun 18, 2018 via email

mckinsel left a comment

Clarify logs (expected to be URLs) and paging (use page_token). #30

Clarify logs (expected to be URLs) and paging (use page_token). #30

Conversation

tetron commented Jun 5, 2018

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

geoffjentry commented Jun 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tetron commented Jun 7, 2018

tetron commented Jun 7, 2018

jaeddy 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

jaeddy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tetron commented Jun 18, 2018

dglazer commented Jun 18, 2018 via email

mckinsel left a comment

Choose a reason for hiding this comment