Skip to content
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

UI Server graphql protobuf feed #3122

Merged
merged 3 commits into from
Jun 28, 2019
Merged

Conversation

dwsutherland
Copy link
Member

@dwsutherland dwsutherland commented Apr 23, 2019

The sibling pull-request to the UI Server graphql integration:
cylc/cylc-uiserver#34
And the Cylc 8 component implementation of the GraphQL PoC.
This change is to provide data in structure and format suitable for resolving GraphQL queries/mutations off of, for the consumption/control the web UI.

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 dependency is protobuf which is just installed via pip, and if the definition file ws_messages.proto is changed with development then the binary for generating the python module can be found here (not needed at runtime) and the following command used:

protoc -I=./ --python_out=./ ws_messages.proto

At present graphql (at the UI Server) resolves directly off the decoded protobuf messages.. and the protobuf message for the entire data structure can be requested using:

$ cylc client --no-input baz pb_entire_workflow

Which seems just as fast as $ cylc client --no-input baz api (the overhead probably setting up the runtime client):

real	0m0.298s
user	0m0.253s
sys	0m0.026s

At the moment the full data structure is recalculated in the mainloop on changes (just like state_summary_mgr updates), and an additional protobuf message is populated with this for encoding and transport.

This Pull Request

  • protobuf definition (data elements)
  • job-task data element separation
  • data structure population/updates
  • REQ/RES "endpoint" command added.
  • GraphQL endpoint - schema for both WS and UI Server
  • Ghost nodes - Generate elements via graph edges
  • Testing

Additional (to come or future PR?)

  • Incremental updates to data structure (using deltas)
  • deltas sent via PUB/SUB to UI Server and checksum skeleton data management.

@hjoliver hjoliver added this to the cylc-8.0a1 milestone Apr 23, 2019
@cylc cylc deleted a comment Apr 23, 2019
@cylc cylc deleted a comment Apr 23, 2019
@cylc cylc deleted a comment Apr 23, 2019
@cylc cylc deleted a comment Apr 23, 2019
@cylc cylc deleted a comment Apr 23, 2019
kinow
kinow previously requested changes Apr 24, 2019
Copy link
Member

@kinow kinow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few typos, unused imports, but nothing broken (except one name variable created in another loop that looks suspicious.

Finished reading the code diff in GitHub UI, and running a linter in my IDE. Reported issues found here, plus initial questions.

@dwsutherland amazing work! Will take a better look once the PR has been updated. This time will try to run the code.

It would be good, in my opinion, to have unit test for the new code. There are many methods that could be exercised without the need of a feature test, and some conditions in the if/else blocks are easier to test with custom test data and/or mocking.

👏 👏 🎉

lib/cylc/ws_messages.proto Outdated Show resolved Hide resolved
lib/cylc/ws_messages.proto Outdated Show resolved Hide resolved
lib/cylc/ws_data_mgr.py Outdated Show resolved Hide resolved
lib/cylc/ws_data_mgr.py Outdated Show resolved Hide resolved
lib/cylc/task_job_mgr.py Outdated Show resolved Hide resolved
bin/cylc-submit Outdated Show resolved Hide resolved
lib/cylc/scheduler.py Outdated Show resolved Hide resolved
lib/cylc/scheduler.py Outdated Show resolved Hide resolved
lib/cylc/scheduler.py Outdated Show resolved Hide resolved
lib/cylc/task_events_mgr.py Outdated Show resolved Hide resolved
@dwsutherland dwsutherland requested a review from kinow April 24, 2019 08:31
@cylc cylc deleted a comment Apr 24, 2019
@kinow
Copy link
Member

kinow commented Apr 26, 2019

Conflicts after the setup.py PR which modified install.sh. The dependency will need to go to the setup.py I think 👍

@cylc cylc deleted a comment Apr 26, 2019
@cylc cylc deleted a comment Apr 26, 2019
Copy link
Contributor

@matthewrmshin matthewrmshin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1st pass through 👀...

@dwsutherland
Copy link
Member Author

Closes #3023

Copy link
Member

@oliver-sanders oliver-sanders left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1st pass...

lib/cylc/network/client.py Outdated Show resolved Hide resolved
lib/cylc/prerequisite.py Outdated Show resolved Hide resolved
cylc/flow/config.py Outdated Show resolved Hide resolved
cylc/flow/network/client.py Outdated Show resolved Hide resolved
cylc/flow/network/server.py Outdated Show resolved Hide resolved
cylc/flow/ws_messages_pb2.py Outdated Show resolved Hide resolved
cylc/flow/ws_messages_pb2.py Show resolved Hide resolved
cylc/flow/ws_messages_pb2.py Show resolved Hide resolved
@dwsutherland
Copy link
Member Author

dwsutherland commented Jun 15, 2019

Working through this with light to getting it in ASAP.

Some comments on efficiency, there is a lot of logic in the form:

z = []
for x in [x for x in func(y)]:
    z.append(x)
return z

Which can be reduced to:

return [x for x in func(y)]

@oliver-sanders : something others have raised, but I guess the hint is the # TODO: as sorting and/or pagination ...etc will happen before the return... Or is this not a good idea?

i.e. sort by x ascending, return first n results.

Hmm... strike that (I think)
The sorting (...etc) will be identical for WS and UIS, so should be in the schema (as resolvers.py will arguably differ for obvious reasons).. Ok will fix

cylc/flow/config.py Outdated Show resolved Hide resolved
@cylc cylc deleted a comment Jun 27, 2019
@kinow
Copy link
Member

kinow commented Jun 27, 2019

@dwsutherland we had a quick demo this morning (UK time) for the end to end system. I synced the repositories, and found one issue in cylc-flow using your branch, and running the suite five.

2019-06-27T09:11:10Z INFO - DONE
Traceback (most recent call last):
  File "/home/kinow/Development/python/workspace/cylc-uiserver/venv/bin/cylc-run", line 25, in <module>
    main(is_restart=False)
  File "/home/kinow/Development/python/workspace/cylc-uiserver/venv/lib/python3.7/site-packages/cylc/flow/terminal.py", line 65, in wrapper
    function(*args, **kwargs)
  File "/home/kinow/Development/python/workspace/cylc-uiserver/venv/lib/python3.7/site-packages/cylc/flow/scheduler_cli.py", line 132, in main
    scheduler.start()
  File "/home/kinow/Development/python/workspace/cylc-uiserver/venv/lib/python3.7/site-packages/cylc/flow/scheduler.py", line 295, in start
    raise exc
  File "/home/kinow/Development/python/workspace/cylc-uiserver/venv/lib/python3.7/site-packages/cylc/flow/scheduler.py", line 266, in start
    self.run()
  File "/home/kinow/Development/python/workspace/cylc-uiserver/venv/lib/python3.7/site-packages/cylc/flow/scheduler.py", line 1545, in run
    has_updated = self.update_data_structure()
  File "/home/kinow/Development/python/workspace/cylc-uiserver/venv/lib/python3.7/site-packages/cylc/flow/scheduler.py", line 1592, in update_data_structure
    self.ws_data_mgr.initiate_data_model()
  File "/home/kinow/Development/python/workspace/cylc-uiserver/venv/lib/python3.7/site-packages/cylc/flow/ws_data_mgr.py", line 70, in initiate_data_model
    self.update_task_proxies()
  File "/home/kinow/Development/python/workspace/cylc-uiserver/venv/lib/python3.7/site-packages/cylc/flow/ws_data_mgr.py", line 364, in update_task_proxies
    prereq_obj = prereq.api_dump(self.workflow_id)
  File "/home/kinow/Development/python/workspace/cylc-uiserver/venv/lib/python3.7/site-packages/cylc/flow/prerequisite.py", line 258, in api_dump
    expression=temp,
UnboundLocalError: local variable 'temp' referenced before assignment

I fixed it by adding temp = None before the line with the error. But not sure if that fix is good enough, or if temp was supposed to be created in one of the loops before.

@dwsutherland
Copy link
Member Author

I fixed it by adding temp = None before the line with the error. But not sure if that fix is good enough, or if temp was supposed to be created in one of the loops before.

@kinow Sorry - there was a bug in it for about 30min (and it wouldn't have worked with the UI Server at that point anyway, as I had turned the protobuf message to JSON to test it)... All fixed now.. working on the UI Server

@hjoliver
Copy link
Member

MERGING! 💥 thanks @dwsutherland 💐 We'll put up a bunch of Issues for urgent follow-up...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants