Add subscriptions #82

kinow · 2019-09-26T01:53:42Z

Closes #14

Will use a file that is still under review, but we have tested and it works for what we need (and looks like the code review will take much longer in graphql-ws)
Will use the simplest approach available, of just looping and asyncio.sleeping for a few seconds, then sending more data. This can be improved later to send data only when there is data available, but this will be an improvement for later.

Sibling PRs:

kinow · 2019-09-26T02:03:41Z

Work in progress, will start adding now the code to have subscriptions. But this was what was more obvious to me that was necessary for this change. More to come in the next days 👍

codecov-io · 2019-09-26T02:14:24Z

Codecov Report

Merging #82 into master will increase coverage by 3.64%.
The diff coverage is 97.22%.

@@            Coverage Diff             @@
##           master      #82      +/-   ##
==========================================
+ Coverage   48.81%   52.45%   +3.64%     
==========================================
  Files           6        6              
  Lines         338      366      +28     
  Branches       57       57              
==========================================
+ Hits          165      192      +27     
- Misses        170      171       +1     
  Partials        3        3

Impacted Files	Coverage Δ
cylc/uiserver/main.py	`66.66% <100%> (+0.59%)`	⬆️
cylc/uiserver/handlers.py	`79.26% <96.66%> (+8.35%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ed4574b...86fc8a8. Read the comment docs.

dwsutherland · 2019-09-26T04:12:45Z

I think we can add the UIS Subscriptions machinery (modules, endpoints ...etc) to this PR (like you have), but develop the GraphQL schema in cylc-flow..
(even for the simplest case because we can just test/dev locally with cylc-uiserver & cylc-flow)

This way we design it, from the beginning, to be able to plug into either WS or UIS subscription resolvers...

kinow · 2019-09-26T04:16:07Z

Oh that's helpful. I saw you define resolvers in cylc flow, then extend it here and add only the specific for uiserver. Clever.

So this ia pretty much it for uiserver for now? We leave it like this and now start working on chema changes in cylc flow??

dwsutherland · 2019-09-26T04:16:15Z

cylc-flow might need allow_subscriptions sorted in the handler.

dwsutherland · 2019-09-26T04:17:49Z

Oh that's helpful. I saw you define resolvers in cylc flow, then extend it here and add only the specific for uiserver. Clever.

So this ia pretty much it for uiserver for now? We leave it like this and now start working on chema changes in cylc flow??

I think so, once you have this one working locally with the simple case in cylc-flow (and GraphiQL working).

cylc/uiserver/main.py

kinow · 2019-09-26T23:14:16Z

@dwsutherland I've added the template to render websocket subscriptions in GraphiQL.

Moved everything websockets related under the cylc.uiserver.websockets. This package is temporary, excluded in pycodestyle, and should either disappear or be simplified once we have the PR merged (could have called it subscriptions, but not good naming things :).

The UI is still working, tested with a running workflow. Then going to /user/kinow/subscriptions I get the normal message that an HTTP GET is not able to talk WebSockets. And submitting a query like

subscription {
  workflows {
    id
  }
}

I can see that the message was submitted to the backend, parsed, interpreted, and rejected because the schema is not ready to handle it yet.

I think we are done here for now. And now the only missing parts are in the cylc-flow repository. What do you think @dwsutherland ?

dwsutherland · 2019-09-26T23:40:59Z

I think we are done here for now. And now the only missing parts are in the cylc-flow repository. What do you think @dwsutherland ?

Yes, although perhaps we could leave this one open until we have put a minimal Subscription into cylc-flow?

kinow · 2019-09-26T23:42:40Z

I think we are done here for now. And now the only missing parts are in the cylc-flow repository. What do you think @dwsutherland ?

Yes, although perhaps we could leave this one open until we have put a minimal Subscription into cylc-flow?

Yup, I'm planning to mark it ready for review once we get at least one subscription working, so that we can test it from point (ui + hub) to point (cylc-flow with running workflow data).

kinow

@dwsutherland this one won't change much for now. I've added some comments to the code to help reviewers 👍

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

.travis.yml

@@ -24,7 +24,7 @@ python: 3.7

 before_install:
  - pip install pycodestyle
-  - pycodestyle --exclude=".git,.tox,__pycache,venv,.eggs,build,jupyterhub_config.py"
+  - pycodestyle --exclude=".git,.tox,__pycache,venv,.eggs,build,jupyterhub_config.py,cylc/uiserver/websockets/"


This is due to the graphene-tornado class that we copied from an open PR. Mainly due to line number that is greater than 80 there 👍 so we need to ignore that for now.

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

cylc/uiserver/handlers.py

@@ -145,9 +150,58 @@ def prepare(self):
        super().prepare()


+class SubscriptionHandler(websocket.WebSocketHandler):


Note: copied from the PR's example.

cylc/uiserver/handlers.py

kinow · 2019-09-30T22:28:45Z

cylc/uiserver/handlers.py

+        return wider_context
+
+
+class GraphiQLHandler(UIServerGraphQLHandler):


From the PR's example. This handler renders a custom React app that talks WebSockets. So that the query is reloaded automatically 👍

kinow · 2019-09-30T22:29:08Z

cylc/uiserver/websockets/template.py

+from string import Template
+
+
+def render_graphiql(base_url: str) -> str:


From the PR's example.

Modified to create the WebSocket subscription URL using JS for hostname and port, and Python for the base URL.

kinow · 2019-09-30T22:29:29Z

cylc/uiserver/websockets/tornado.py

+setup_observable_extension()
+
+
+class TornadoConnectionContext(BaseConnectionContext):


From the PR's example, with one modification.

cylc/uiserver/websockets/tornado.py

cylc/uiserver/handlers.py

cylc/uiserver/websockets/tornado.py

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

Added some unit tests. Should be ready for review now.

kinow · 2019-10-09T00:13:37Z

Juts added a new commit with the changelog.

dwsutherland

LGTM 👍 .. Worked locally ( I don't think this breaks existing functionality )

kinow · 2019-10-17T02:26:28Z

Oh, got one conflict in the handlers, will fix it in a few minutes 👍

cylc/uiserver/websockets/template.py

kinow · 2019-10-20T21:52:52Z

I think I found one possible solution. A mix of more JS, and just a little Python. We still replace the base_url (required as we need to know what value goes in /user/$USER/), but the protocol+host+port can be done in JS.

The WebSocket client is created on the client-side, so it should be safe to ignore JupyterHub configurations like c.JupyterHub.port and ...host, and just grab whatever is specified in the browser.

Test with default port 8000 after running jupyterhub.

Then the result after running jupyterhub --ip=10.0.2.15 --port=4444:

kinow · 2019-12-03T10:40:40Z

Rebased and conflicts fixed.

Naming it graphql_ws or tornado could cause issues loading Python code.

…ow in our project)

…on (base_url)

hjoliver

LGTM 👍

kinow added the enhancement label Sep 26, 2019

kinow added this to the 0.2 milestone Sep 26, 2019

kinow requested a review from dwsutherland September 26, 2019 01:53

kinow self-assigned this Sep 26, 2019

kinow force-pushed the add-subscriptions branch 4 times, most recently from a5a89df to bdef1dc Compare September 26, 2019 02:22

dwsutherland reviewed Sep 26, 2019

View reviewed changes

cylc/uiserver/main.py Outdated Show resolved Hide resolved

matthewrmshin mentioned this pull request Sep 26, 2019

Add subscriptions #81

Closed

kinow commented Sep 30, 2019

View reviewed changes

kinow marked this pull request as ready for review September 30, 2019 22:34

kinow mentioned this pull request Sep 30, 2019

Add subscriptions cylc/cylc-flow#3390

Merged

6 tasks

kinow force-pushed the add-subscriptions branch from 06dd5f1 to fe5af34 Compare October 2, 2019 06:26

kinow commented Oct 2, 2019

View reviewed changes

cylc/uiserver/websockets/tornado.py Show resolved Hide resolved

This was referenced Oct 4, 2019

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

Closed

Add subscriptions cylc/cylc-ui#280

Merged

kinow force-pushed the add-subscriptions branch from 5f34fca to 92fb33f Compare October 9, 2019 00:13

dwsutherland approved these changes Oct 10, 2019

View reviewed changes

kinow commented Oct 17, 2019

View reviewed changes

cylc/uiserver/websockets/template.py Outdated Show resolved Hide resolved

kinow force-pushed the add-subscriptions branch from 92fb33f to d9bd127 Compare October 17, 2019 03:29

kinow force-pushed the add-subscriptions branch from ef7a6f1 to be86139 Compare December 3, 2019 10:39

Bruno P. Kinoshita and others added 17 commits December 4, 2019 10:48

Add graphql_ws dependency

fc2b947

Add graphql-ws pending PR file for Tornado

85c4a7f

Ignore WIP PR file in pycodestyle

f0609ac

Add SubscriptionHandler

9173b04

Optimize imports

4410f25

Create a websockets package.

4201bec

Naming it graphql_ws or tornado could cause issues loading Python code.

Rename file to match PR's file name

82fce39

Add graphiql handler with React app

704e3d6

pycodestyle ignore all files under websockets package

d834d75

Add context property to SubscriptionHandler

75fb8a7

Pass context to the callback handler

9efc95a

Patch the execute method for now

251325d

Add code to resolve awaitable fields in subscription returned data

27bab66

Ignore tests and code from websockets package (3rd party copied for n…

7056bab

…ow in our project)

Add unit tests for SubscriptionHandler

f308470

Add changelog

d4c885f

Create WebSockets susbcription URL using both JS (host+port) and Pyth…

86fc8a8

…on (base_url)

kinow force-pushed the add-subscriptions branch from be86139 to 86fc8a8 Compare December 3, 2019 21:48

hjoliver approved these changes Dec 4, 2019

View reviewed changes

hjoliver merged commit 23473ad into cylc:master Dec 4, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add subscriptions #82

Add subscriptions #82

kinow commented Sep 26, 2019 •

edited

Loading

kinow commented Sep 26, 2019

codecov-io commented Sep 26, 2019 •

edited

Loading

dwsutherland commented Sep 26, 2019 •

edited

Loading

kinow commented Sep 26, 2019

dwsutherland commented Sep 26, 2019

dwsutherland commented Sep 26, 2019

kinow commented Sep 26, 2019

dwsutherland commented Sep 26, 2019

kinow commented Sep 26, 2019

kinow left a comment

kinow Sep 30, 2019

kinow Sep 30, 2019

kinow Sep 30, 2019

kinow Sep 30, 2019

kinow Oct 20, 2019

kinow Sep 30, 2019

kinow commented Oct 3, 2019

kinow commented Oct 9, 2019

dwsutherland left a comment

kinow commented Oct 17, 2019

kinow commented Oct 20, 2019

kinow commented Dec 3, 2019 •

edited

Loading

hjoliver left a comment

		@@ -145,9 +150,58 @@ def prepare(self):
		super().prepare()


		class SubscriptionHandler(websocket.WebSocketHandler):

		return wider_context


		class GraphiQLHandler(UIServerGraphQLHandler):

		from string import Template


		def render_graphiql(base_url: str) -> str:

		setup_observable_extension()


		class TornadoConnectionContext(BaseConnectionContext):

Add subscriptions #82

Add subscriptions #82

Conversation

kinow commented Sep 26, 2019 • edited Loading

kinow commented Sep 26, 2019

codecov-io commented Sep 26, 2019 • edited Loading

Codecov Report

dwsutherland commented Sep 26, 2019 • edited Loading

kinow commented Sep 26, 2019

dwsutherland commented Sep 26, 2019

dwsutherland commented Sep 26, 2019

kinow commented Sep 26, 2019

dwsutherland commented Sep 26, 2019

kinow commented Sep 26, 2019

kinow left a comment

Choose a reason for hiding this comment

kinow Sep 30, 2019

Choose a reason for hiding this comment

kinow Sep 30, 2019

Choose a reason for hiding this comment

kinow Sep 30, 2019

Choose a reason for hiding this comment

kinow Sep 30, 2019

Choose a reason for hiding this comment

kinow Oct 20, 2019

Choose a reason for hiding this comment

kinow Sep 30, 2019

Choose a reason for hiding this comment

kinow commented Oct 3, 2019

kinow commented Oct 9, 2019

dwsutherland left a comment

Choose a reason for hiding this comment

kinow commented Oct 17, 2019

kinow commented Oct 20, 2019

kinow commented Dec 3, 2019 • edited Loading

hjoliver left a comment

Choose a reason for hiding this comment

kinow commented Sep 26, 2019 •

edited

Loading

codecov-io commented Sep 26, 2019 •

edited

Loading

dwsutherland commented Sep 26, 2019 •

edited

Loading

kinow commented Dec 3, 2019 •

edited

Loading