-
Notifications
You must be signed in to change notification settings - Fork 18
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WS-UIS data and GraphQL integration #34
WS-UIS data and GraphQL integration #34
Conversation
Travis will likely fail, as cylc/cylc-flow is not a requirement yet. |
Indeed it failed with: In this case I think once everybody is back from holidays, perhaps we could prioritize cylc/cylc-flow#2990. That one has become a blocker for a few issues now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Came here to comment on setup.py
but checking out the code in the IDE a few issues popped in the warnings panel 👍
Codecov Report
@@ Coverage Diff @@
## master #34 +/- ##
==========================================
- Coverage 68.47% 61.15% -7.33%
==========================================
Files 2 2
Lines 92 121 +29
Branches 6 9 +3
==========================================
+ Hits 63 74 +11
- Misses 29 47 +18
Continue to review full report at Codecov.
|
@dwsutherland see my last commit with the temporary fix for this build. Now Travis is happy again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had a quick look by 👀 and 👓. Code looks OK in general. A few comments on style. For readability, I prefer explicit for
loops to list comprehensions when the logic does not fit in a line - but that is a personal preference that you can feel free to ignore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have not yet had a chance to try this out, as it is not trivial to set-up the environment given difficulty in getting certain installation requirements e.g. npm
& jupyterhub
from inside the Met Office, but I've had a scan through the code.
It looks good in general, but I have a few minor observations & questions.
c616bcc
to
70bedc2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is pretty exciting! Need to get it set up and running. Request structure looks good!
I'd kinda envisaged the GraphQL layer being in cylc-flow
itself rather than in the UI-Server, this way could work fine too but we will need to have a quick think about:
- How the UI-Server will work when connecting to workflows of different versions.
- How to make this work with (API-On-The-Fly)[https://github.com/Suite API - On-The-Fly cylc-flow#2123].
I guess here we have a GraphQL-ZMQ adapter layer so it's about ensuring that the adapter isn't brittle to changes.
Well we could have both, the data structure is present at both. They would be different though, because the
Potential conflicts when:
Exactly, protobuf helps alot (it was designed with these issues in mind!). But I can imagine similar issues even with only a
Will need to read up again, but GraphQL has introspection which can return all the information about the schema (including descriptions, which I need to do more of!), as shown in the |
I'm struggling quite a bit with this one trying to see how it can fit into my past visions of decoupling Cylc components. I've tried and kinda failed to outline some of it here, I might make more sense of things later... The issue I'm trying to get my head around is this:
I don't know what the best way to decouple things is, it was all very clear to me once but things have evolved so far since then I can't see it any more. Back when things were simpler and we were talking about one workflow talking to one GUI (in Electron) it was easy to see how decoupling the Cylc Flow and Cylc UI versions might work. The GUI can just request the API from the workflow and present it in a nice form for the user (e.g. the stop suite panel) and GraphQL can handle most upgrade requirements simply enough. Now we have one UIServer talking to multiple workflows things are more difficult as by defining the GraphQL schema at the UIServer we are defining the interface for all suites irrespective of their version. If we make changes to a Cylc suite (which we have form of) we will have to put a lot of adapter logic into the UIServer. GraphQL (and presumably ProtoBuff) can protect us from the most basic examples (fields added / removed) but the rest... I'm struggling to see a way forward at the moment, @dwsutherland could you help me out by outlining how you envisage us handling some example API changes e.g. (adding fields, changing arguments in ZMQ endpoints, etc)? Further thoughts on upgrading Cylc components: (a quick observation with no point to make really) Cylc-UI is instantaneously upgradeable, that is to say you can change versions at the press of a button or just by refreshing the page. Handling change here is easy. The Cylc-UIServer is much harder to upgrade, we have to wait for users to close their browser windows for the UIServer to become inactive for it to auto-shutdown (or have some sort of force upgrade system). So we probably want the dynamic components isolated in Cylc-UI where they are easy to change? |
Is it possible to define multiple workflow API data models under a single GraphQL schema? (With luck, we'll only have to back support a small number of workflow server APIs for each UI/UI-Server version. So even if we have to spell out all the data models explicitly for the supported workflow API data models, it should not be a big deal. But the top level data model must be able to handle this.) For upgrading a ui-server... I suppose it should be possible for use to configure the hub to tell all ui-servers running outdated software to shutdown and restart with the current software version. Browsers sessions can reconnect automatically. This does not sound any worse than what Outlook (web) does to my sessions everyday. |
@oliver-sanders: Thanks for bring this up, yes there will be ways to break this PR data model.. I guess before we posit any solutions, we need to describe problems that are unique to the current proposed data model...
Firstly I'm not exactly sure what you mean by tightly, because I don't really see how
doesn't apply to
The UIServer, of course, can ensure the version of GUI/
Maybe it's just Dev/Debug mode.. But during development you can have the UIServer running and change the source code, Tornado must reload or something because I never have to shut my browser down.
This setup is arguably how it should be! Why should the frontend have to deal with Workflow-GraphQL version inconsistencies in the backend? The UIServer will always be the one talking to all the user's workflows, because (and correct me if I'm wrong):
We could still have graphql at the workflow, however:
BTW - GraphQL is designed/often-used to take multiple input sources of different types (SQLDB, ProtoBuf, REST, MongoDB ...etc) and provide a coherent data access point. So, I think we can narrow down the question to: How do we envisage
Yes, ProtoBuf can easily handle adding more and more fields and I've made all the fields in the Leaving you with the question: How the hell will the UIServer handle this? If there are other breaking changes, then either the workflows or uiserver will have to contain back compatibility Just as a side note, we had the same issue with the REST Cherrypy API, where we had to duplicate the URLs with Also important to remember; it's only differing APIs that may cause issues, workflows can differ in version all they want otherwise.. Please call me out if I'm missing something or plain wrong! |
ef70e90
to
437973e
Compare
OK, it looks like I'm the only one who's concerned so should probably push ahead whatever. Perhaps discuss at the next video meet. Something else to think about which we came up with at a UK team meet was that we want out CLI client to have the ability to route via the UI server. I guess the UI server could just act as a TCP-Websocket buffer? |
Hmm... You need both, so how would the CLI discover a running UIServer? At the moment, I import the workflow runtime client (from Perhaps for uniformity we could put a graphql schema the workflow, and import some of this to the graphql schema from there into the UIServer... But it would still have to be designed to handle multiple workflows (defaulting to itself when running in a single workflow)... The UIServer is running with Tornado (which will be TCP-Websocket at some point, but for browser interaction), perharps we could start up a ZeroMQ stub in addition.... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @dwsutherland , any chance of including unit tests for the non-web related code? i.e. no need to bother with the Tornado handlers being tested.
@dwsutherland with the following query query {
tasks {
id, name, meta {
title
description
URL
}, meanElapsedTime, namespace, proxies {
id
task {
id
}
state
cyclePoint
spawned
depth
jobSubmits
latestMessage
outputs
prerequisites {
expression
satisfied
}
jobs {
id
}
parents {
id
}
}, depth
}
} am getting the error
The console running ERROR:tornado.application:Error: Object of type RepeatedScalarContainer is not JSON serializable Traceback (most recent call last):
File "/home/kinow/Development/python/workspace/cylc-uiserver/venv/lib/python3.7/site-packages/graphene_tornado/tornado_graphql_handler.py", line 95, in post
yield self.run('post')
File "/home/kinow/Development/python/workspace/cylc-uiserver/venv/lib/python3.7/site-packages/tornado/gen.py", line 729, in run
value = future.result()
File "/home/kinow/Development/python/workspace/cylc-uiserver/venv/lib/python3.7/site-packages/tornado/gen.py", line 736, in run
yielded = self.gen.throw(*exc_info) # type: ignore
File "/home/kinow/Development/python/workspace/cylc-uiserver/venv/lib/python3.7/site-packages/graphene_tornado/tornado_graphql_handler.py", line 112, in run
result, status_code = yield self.get_response(data, method, show_graphiql)
File "/home/kinow/Development/python/workspace/cylc-uiserver/venv/lib/python3.7/site-packages/tornado/gen.py", line 729, in run
value = future.result()
File "/home/kinow/Development/python/workspace/cylc-uiserver/venv/lib/python3.7/site-packages/tornado/gen.py", line 742, in run
yielded = self.gen.send(value)
File "/home/kinow/Development/python/workspace/cylc-uiserver/venv/lib/python3.7/site-packages/graphene_tornado/tornado_graphql_handler.py", line 208, in get_response
result = self.json_encode(response, pretty=self.pretty or show_graphiql)
File "/home/kinow/Development/python/workspace/cylc-uiserver/venv/lib/python3.7/site-packages/graphene_tornado/tornado_graphql_handler.py", line 272, in json_encode
return json.dumps(d, separators=(',', ':'))
File "/home/kinow/anaconda3/lib/python3.7/json/__init__.py", line 238, in dumps
**kw).encode(obj)
File "/home/kinow/anaconda3/lib/python3.7/json/encoder.py", line 199, in encode
chunks = self.iterencode(o, _one_shot=True)
File "/home/kinow/anaconda3/lib/python3.7/json/encoder.py", line 257, in iterencode
return _iterencode(o, 0)
File "/home/kinow/anaconda3/lib/python3.7/json/encoder.py", line 179, in default
raise TypeError(f'Object of type {o.__class__.__name__} '
TypeError: Object of type RepeatedScalarContainer is not JSON serializable
ERROR:tornado.access:500 POST /user/kinow/graphql/graphiql? (127.0.0.1) 13.41ms Is it something you are aware that could happen? I built the query in the browser, using GraphiQL's auto complete feature. Thanks! |
Appears to be the |
Another dummy user support question @dwsutherland (sorry!). At the moment we have 2 REST calls to populate the Suite Tasks. One to retrieve the current Suite, then one more to retrieve the Tasks in the suite. But as we are using GraphQL, I decided to try and use a single query now too. query ($name: String!) {
workflows (name: $name) {
id
name
owner
host
port
tasks {
id, name, meta {
title
description
URL
}, meanElapsedTime, namespace, proxies {
id
task {
id
}
state
cyclePoint
spawned
depth
jobSubmits
latestMessage
}, depth
}
}
} However, I am not able to pass the
Just wondering if there is anything special that needs to be done on the Resolver to accept variables? Or am I able to use a variable for any field in the Query? Thanks! |
Two things: a) Like the error suggests. there is no argument With cylc-ui use I'll try the other error you presented.. I often get browser cache issues when restarting the ui-server.. |
outputs bug fixed. |
Thanks a lot @dwsutherland ! Will update the code with your suggestions and rebase the code to try it again. |
44b6b14
to
4c4ff76
Compare
Mutations added last week. Some examples: ---------------------------------------------------
---------------------------------------------------
---------------------------------------------------
---------------------------------------------------
---------------------------------------------------
|
HI @dwsutherland We are going to try to crowd review and merge this and cylc/cylc-flow#3122 while @hjoliver and @kinow are in UK. Please resolve the conflict of this branch when you have the time to do so. |
2263eb1
to
3575acb
Compare
Done. I've got a few important changes inbound for both PR, will try and get them in before. |
Travis failing with:
@kinow confirmed it will be like this until both PRs are merged.. (right?) |
async resolvers (mutations & queries)
be78090
to
97f0f97
Compare
Ok pushed, however, Travis may still break as it imports |
MERGING! 💥 thanks @dwsutherland 💐 We'll put up a bunch of Issues for urgent follow-up... |
🍻 Thanks everyone for your input and critique! |
👏 👏 👏 Here's to you @dwsutherland 🍻 ! Well done! |
The sibling pull-request to the UI Server graphql protobuf feed:
cylc/cylc-flow#3122
And the Cylc8 UI Server component implementation of the GraphQL PoC.
There will be many more improvements/additions to be incorporated both in this and subsequent pull requests, however, for the sake of UI development and review this needs to be exposed.
The extra dependencies are:
(
colorama
due to thecylc scan
CL call, which will be deprecated as workflow discovery is already integrated)The instructions on installation are included in the README.md, and for this change cylc workflow library needs to be put into the python path (or venv site packages), I just source:
With
.npm_env
being my local node.js environment:There are three main files in this change:
schema.py
- GraphQL data structure and types definition (uses graphene)workflow_services_mgr.py
- Workflow discovery, REQ client established (and passed to data manager), and spawning (to come).data_mgr.py
- Uses established client/workflows to retrieve and process data structure (decode protobuf), and contains access & filtering methods used by the resolvers.In order to pass in the uiserver/data_manager as context to the resolver's I had to subclass the
graphene-tornado
GraphQL handler:The data update method is very crude at the moment; it pulls the full workflow data msg every 5 secs (which should be faster than CLI
$ cylc client --no-input baz pb_entire_workflow
as the client is already established by the WS manager)... This message is decoded/parsed as a protobuf object (class instance) and stored in a dictionary with "owner/workflow" as a key.One of the good features of the UI Server holding/maintaining a data structure is, without needing multiple requests, we can query and filter across workflows, for example:
query
variables
results
This applies to all fields (from root query), i.e. tasks, jobs, familes, edges... This could open up some possibilities for us.. (And obviously gives us the data for the "gscan" type web view)
A lot more to do, but this is a start 😃
This Pull Request
Additional (to come or future PR?)