Add subscriptions #3390

kinow · 2019-09-30T02:03:04Z

This is a small change with no associated Issue.

Requirements check-list

I have read CONTRIBUTING.md and added my name as a Code Contributor.
Contains logically grouped changes (else tidy your branch by rebase).
Does not contain off-topic changes (use other PRs for other changes).
Appropriate tests are included (unit and/or functional).
Appropriate change log entry included.
No documentation update required.

Sibling PRs:

kinow · 2019-09-30T02:06:04Z

To be updated soon. At the moment I have an issue with the context info. In the Query resolvers, the context is added in the Tornado handler.

However, that handler extends a class from graphene-tornado, which takes care to populate graphql_params, context, and other variables for the queries.

While for subscriptions, we don't extend this graphene-tornado, but tornado.web.WebsocketHandler. Meaning the info.context is not being set...

kinow

@dwsutherland finished the PR for the subscription { workflows {} }. Added some comments to the code. I think our only option would be to copy everything done for the Query, but wrapping around a while True: ... asyncio.sleep ? If you agree with that, then I will start the copy-and-paste and finish updating this PR.

kinow · 2019-09-30T22:14:22Z

cylc/flow/network/schema.py

+        args['exworkflows'] = [parse_workflow_id(w_id) for w_id in
+                               args['exids']]
+        resolvers = info.context.get('resolvers')
+        yield await resolvers.get_workflows(args)


This is pretty much the same as get_workflows - used for a Query.

With a difference that we return the coroutine here.

We could move the argument parsing to a common function called by both.

Hmm, but then we would also have to do that for all other resolvers?

Yes, all if it’s worth it... but if we move argument parsing to resolvers.py it would serve the same purpose... Then the schema would be pure schema

Then the schema would be pure schema

Ah, I think I'm starting to understand it. This would simplify the schema a bit more. Would it be alright to move this to a follow-up PR?

Yes, it also might open other avenues, hiding the nuts and bolts 🔩 of the driving data... i.e. defining the schema in GraphQL pseudo code, like a document which is not implementation language specific (which is a thing)

(Not that we want to go that way)

kinow · 2019-09-30T22:16:41Z

cylc/flow/network/schema.py

+                               args['exids']]
+        resolvers = info.context.get('resolvers')
+        yield await resolvers.get_workflows(args)
+        await asyncio.sleep(5.)


Most examples in graphql-ws use a 1. interval. However, doing so would send data to the UI too fast I think (?). At least for now, until we have the deltas and/or update the UI to handle updates selectively (i.e. update the workflow/graph data structure on the UI only if it's different.. though it'd be a bit hard for the tree at least)

So I've set it to 5 seconds... which isn't really much different than the polling we have right now 😬 but one step each time 🚶‍♂️

Depends on the query I suppose, and whether it's only updates feed to the resolver.. i.e.

query { workflows { id } }

would be ok.. All this will be driven by data updates in the future (which could be sub 1sec 😬 )

If we can't just send updated content then, this is another reason why views just subscribing to exactly what they need might be better (so not all data is sent every push)..

kinow · 2019-09-30T22:17:46Z

cylc/flow/network/schema.py

+        description=Workflow._meta.description,
+        ids=List(ID, default_value=[]),
+        exids=List(ID, default_value=[]),
+        resolver=subscribe_workflows)


This is pretty much the same as the class Queries we have already for a Query. Only difference being the resolver...

This duplication is ok..

kinow · 2019-09-30T22:18:23Z

cylc/flow/network/schema.py

+        resolver=subscribe_workflows)
+
+
+schema = Schema(query=Queries, subscription=Subscriptions, mutation=Mutations)


Schema untouched, with exception of a new subscription= value 👍 then the schema "understands" queries with subcription { ... }

kinow · 2019-10-01T23:53:28Z

cylc/flow/network/schema.py

+        while True:
+            yield await func(*args, **kwargs)
+            await asyncio.sleep(sleep_seconds)
+    return gen


@dwsutherland a function that converts the existing resolver methods into async-generators, expected for the websockets to work.

kinow · 2019-10-01T23:54:31Z

cylc/flow/network/schema.py

+        description=Workflow._meta.description,
+        ids=List(ID, default_value=[]),
+        exids=List(ID, default_value=[]),
+        resolver=to_subscription(get_workflows))


@dwsutherland now I can query

subscription { workflows }

Or

subscription { tasks }

However, querying

subscription { workflows { tasks } }

Doesn't bring me anything. Any clue what I could be missing?

Will have a look

Oh, never mind. Just realized why that's happening. In the class Workflow(ObjectType), it has the Tasks and the resolver is set there too. Hmmm, tricky to get this one working for subscriptions now 😞

I think someone had the same issue with the aiohttp version:
graphql-python/graphql-ws#12 (comment)

Looks as though they modified the handler to await all async nested fields..

👀 gonna read it again tomorrow. That method could be a life saver. Just need to understand the logic and see if that'd work for us too. Great find @dwsutherland !

@dwsutherland , quick test, and that actually worked. Will update the PR (just clean up current mess, move customizations to a separate file, etc) in a few minutes. Understood about 50% of the code, but will re-read it in the morning ☕ Thanks a ton for that. Otherwise I would still be scratching my head with no solution.

dwsutherland · 2019-10-02T02:37:37Z

Works great! thanks @kinow.. Tested it also:

You can have multiple subscriptions via new tabs, however, I think you need to rename the root name to modify it as I guess the previous one is still running? i.e.

subscription {
  jobs2: jobs (workflows: ["*|*"], states: ["submitted", "running"]) {
    id
    state
    submitNum
    batchSysName
    batchSysJobId
  }
}

After the nested async issue is solved, we can probably try get this one in...

We can modify & organise the code where needed afterwards... This will at least allow the UI to change to the new protocol (not sure if the proxy needs modifying?)

kinow · 2019-10-02T21:55:50Z

Was going to add a changelog for 8.0a2, but looks like we don't have a changelog for 8.0a1, only for 8.0a0. Should we add one retroactively @cylc/core ?

kinow · 2019-10-03T02:50:32Z

Just tried writing a very simple functional test. Copied 01-workflow.t and its suite directory.

Unfortunately it doesn't look like cylc client 'graphql' .... works with subscriptions. I am getting:

$ cylc client five 'graphql' < <(echo $JSON)
[
    {
        "error": {
            "message": "Subscriptions are not allowed. You will need to either use the subscribe function or pass allow_subscriptions=True",
            "traceback": [
                "Traceback (most recent call last):\n",
                "  File \"/home/kinow/Development/python/workspace/cylc-flow/venv/lib/python3.7/site-packages/promise/promise.py\", line 85, in try_catch\n    return (handler(*args, **kwargs), None)\n",
                "  File \"/home/kinow/Development/python/workspace/cylc-flow/venv/lib/python3.7/site-packages/graphql/execution/executor.py\", line 127, in promise_executor\n    return execute_operation(exe_context, exe_context.operation, root)\n",
                "  File \"/home/kinow/Development/python/workspace/cylc-flow/venv/lib/python3.7/site-packages/graphql/execution/executor.py\", line 176, in execute_operation\n    \"Subscriptions are not allowed. \"\n",
                "Exception: Subscriptions are not allowed. You will need to either use the subscribe function or pass allow_subscriptions=True\n"
            ]
        }
    }
]

Which is a command similar to what test_header translates the functional test to. The subscription is handled in cylc-uiserver, not in cylc-flow... from what I understand, cylc-uiserver's Tornado handler will receive a message via WebSockets, receive it, and then will use Cylc Flow's schema to resolve it.

So I think we only need the unit test here for the schema.

kinow · 2019-10-03T07:08:33Z

Coverage report not passing the threshold, but looking at the diff doesn't seem to have much else that can be tested without actually using a websocket + the ioloop to consume the async-generator 😥

dwsutherland · 2019-10-06T21:06:29Z

Was going to add a changelog for 8.0a2, but looks like we don't have a changelog for 8.0a1, only for 8.0a0. Should we add one retroactively @cylc/core ?

I think it's Ok that we don't hit the 95% mark with this one.. Because adding functioning GraphQL subscriptions at the cylc-flow/WS end is/could-be a different PR, and has to wait until #3389 is in (if it's going to use PUB/SUB).

This PR is at minimum for adding UIS subscriptions (although you could wait, and construct the handler after #3389 )...

Proper GraphQL subscriptions would be data-driven, not just a loop in the schema calling the query (IMO).

hjoliver · 2019-10-07T00:50:36Z

Was going to add a changelog for 8.0a2, but looks like we don't have a changelog for 8.0a1, only for 8.0a0. Should we add one retroactively @cylc/core ?

We do have a section in CHANGES.md for 8.0a1 - maybe your branch was out of date?

I've added one for 8.0a2 in #3402

kinow · 2019-10-07T00:59:12Z

Was going to add a changelog for 8.0a2, but looks like we don't have a changelog for 8.0a1, only for 8.0a0. Should we add one retroactively @cylc/core ?

We do have a section in CHANGES.md for 8.0a1 - maybe your branch was out of date?

I've added one for 8.0a2 in #3402

Oh, I think I simply missed that, sorry. 👏 👏 will add one for the subscriptions now. Thanks!

kinow · 2019-10-07T02:53:21Z

(change rebased onto master, no other code changes today 👍 )

kinow · 2019-10-07T04:48:22Z

<post-rebase-travis-kick />

dwsutherland

Works for me! 🎉 ... We can make the push data driven in future PRs..

As long as tests pass, good to merge for me..

kinow · 2019-10-10T04:12:49Z

Kicked Travis (quite sure it passed before, probably because I rebased it).

hjoliver

LGTM 👍

kinow added this to the cylc-8.0a2 milestone Sep 30, 2019

kinow requested a review from dwsutherland September 30, 2019 02:03

kinow self-assigned this Sep 30, 2019

kinow commented Sep 30, 2019

View reviewed changes

kinow mentioned this pull request Sep 30, 2019

Add subscriptions cylc/cylc-uiserver#82

Merged

kinow commented Oct 1, 2019

View reviewed changes

kinow force-pushed the add-subscriptions branch from d4e2503 to 2f61a51 Compare October 1, 2019 23:56

kinow force-pushed the add-subscriptions branch from 2f61a51 to fa0a576 Compare October 3, 2019 00:17

kinow marked this pull request as ready for review October 3, 2019 00:17

kinow force-pushed the add-subscriptions branch from fa0a576 to 0646ffc Compare October 3, 2019 02:53

kinow mentioned this pull request Oct 4, 2019

Update components' data structure incrementally via websockets cylc/cylc-ui#207

Closed

kinow mentioned this pull request Oct 7, 2019

Add subscriptions cylc/cylc-ui#280

Merged

Bruno P. Kinoshita and others added 5 commits October 7, 2019 15:52

Add subscriptions

b6c6a31

Yield resolved coroutine

b8d272c

Send data every 5 seconds (for now)

93a8abd

Use a wrapper to convert resolvers to async-generators

8fa34f1

Add unit test for to_subscription

305de13

kinow force-pushed the add-subscriptions branch from 0646ffc to 305de13 Compare October 7, 2019 02:52

dwsutherland approved these changes Oct 10, 2019

View reviewed changes

hjoliver approved these changes Oct 17, 2019

View reviewed changes

hjoliver merged commit 8564007 into cylc:master Oct 17, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add subscriptions #3390

Add subscriptions #3390

kinow commented Sep 30, 2019 •

edited

Loading

kinow commented Sep 30, 2019

kinow left a comment

kinow Sep 30, 2019

dwsutherland Sep 30, 2019

kinow Oct 1, 2019

dwsutherland Oct 1, 2019

kinow Oct 1, 2019

dwsutherland Oct 1, 2019 •

edited

Loading

kinow Sep 30, 2019

kinow Sep 30, 2019

dwsutherland Sep 30, 2019

dwsutherland Sep 30, 2019 •

edited

Loading

kinow Sep 30, 2019

dwsutherland Sep 30, 2019

kinow Sep 30, 2019

kinow Oct 1, 2019

kinow Oct 1, 2019

dwsutherland Oct 2, 2019

kinow Oct 2, 2019

dwsutherland Oct 2, 2019

kinow Oct 2, 2019

kinow Oct 2, 2019

dwsutherland commented Oct 2, 2019 •

edited

Loading

kinow commented Oct 2, 2019

kinow commented Oct 3, 2019

kinow commented Oct 3, 2019

dwsutherland commented Oct 6, 2019 •

edited

Loading

hjoliver commented Oct 7, 2019

kinow commented Oct 7, 2019 •

edited

Loading

kinow commented Oct 7, 2019

kinow commented Oct 7, 2019

dwsutherland left a comment

kinow commented Oct 10, 2019

hjoliver left a comment

		resolver=subscribe_workflows)


		schema = Schema(query=Queries, subscription=Subscriptions, mutation=Mutations)

Add subscriptions #3390

Add subscriptions #3390

Conversation

kinow commented Sep 30, 2019 • edited Loading

kinow commented Sep 30, 2019

kinow 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

dwsutherland Oct 1, 2019 • 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

dwsutherland Sep 30, 2019 • 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

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

dwsutherland commented Oct 2, 2019 • edited Loading

kinow commented Oct 2, 2019

kinow commented Oct 3, 2019

kinow commented Oct 3, 2019

dwsutherland commented Oct 6, 2019 • edited Loading

hjoliver commented Oct 7, 2019

kinow commented Oct 7, 2019 • edited Loading

kinow commented Oct 7, 2019

kinow commented Oct 7, 2019

dwsutherland left a comment

Choose a reason for hiding this comment

kinow commented Oct 10, 2019

hjoliver left a comment

Choose a reason for hiding this comment

kinow commented Sep 30, 2019 •

edited

Loading

dwsutherland Oct 1, 2019 •

edited

Loading

dwsutherland Sep 30, 2019 •

edited

Loading

dwsutherland commented Oct 2, 2019 •

edited

Loading

dwsutherland commented Oct 6, 2019 •

edited

Loading

kinow commented Oct 7, 2019 •

edited

Loading