UIS Delta subscriptions & GraphQL null stripping back-end #3500

dwsutherland · 2020-02-08T09:53:31Z

These changes partially address:
https://github.com/cylc/cylc-admin/blob/master/docs/proposal-subscriptions.md
As a back-end solution required to bring the proposal to fruition.

This PR is the prerequisite to the UI Server implementation: cylc/cylc-uiserver#118
(They should be merge together, as there is one change that is breaking to the BaseResolvers)

Labeled POC, but is functional and probably good to go if deemed desirable.

Features:

Deltas of a single main loop iteration grouped into a single message, and published as topic all. This grouping is somewhat artificial, but ensures that any related deltas (i.e. new edges and their nodes) do not arrive at the UIS (and hence WebUI) in separate pushes (helps WUI in reducing incomplete/frequency-of view updates).
The schema definition of the GraphQL delta subscription and resolving async generator. The resolver creates and monitors a queue, populated during delta application, and maps the Protobuf delta to the yielded GraphQL subscription ObjectType.
Bespoke GraphQL backend and middleware to set undesired/empty field values to null, and strip these out of the execution result.

Future:

Workflow Scheduler ZMQ GraphQL subscriptions. As far as I know, there are no supported packages for ZMQ+GraphQL and the current REQ/RES GraphQL end-point is something I/we created for Cylc, So we need to write a module combining our currently implemented PUB/SUB and the execution of our schema, possibly in the same way as Tornado does with graphql-ws or likely something more bespoke.
Make strip_null/strip_values an argument of the query.

Requirements check-list

I have read CONTRIBUTING.md and added my name as a Code Contributor.
Contains logically grouped changes.
Does not contain off-topic changes (use other PRs for other changes).
Already covered by or modifies existing tests (and/or belongs to future work).
No change log entry required as invisible to users.
No documentation update required.

oliver-sanders

Scanning through the code, looks good so far, a couple of small comments.

cylc/flow/network/__init__.py

cylc/flow/data_store_mgr.py

cylc/flow/network/graphql.py

cylc/flow/network/subscriber.py

cylc/flow/network/graphql.py

oliver-sanders · 2020-03-16T00:19:54Z

@dwsutherland, hi trying to catch up, what's the status of this one? Still POC?

dwsutherland · 2020-03-16T02:33:54Z

@dwsutherland, hi trying to catch up, what's the status of this one? Still POC?

Just a few things:

Failures that cropped up in Travis after last commit.
Work on features: UIS Delta subscriptions & GraphQL null stripping back-end cylc-uiserver#118 (comment)
Get confirmation from @kinow, that the feed is satisfactory.

dwsutherland · 2020-03-30T03:03:01Z

FYI - Ran into an issue, which probably should've been addressed earlier (but ok to address here); the job pool is not populated post restart. This means when stopping --now active jobs post restart would update states/times/messages, and previously we had caught this with the scaffolding #3536 (which helped put the core components of the new data provision in place)...

We should probably (like the task pool), repopulate the job pool with the latest submit from the DB:

cylc-flow/cylc/flow/scheduler.py

Lines 572 to 573 in 0d9d333

    
           self.suite_db_mgr.pri_dao.select_task_pool_for_restart( 
        
               self.pool.load_db_task_pool_for_restart, self.options.checkpoint)

So this is what I'm working on...

Although, we could defer this to another PR and just put some scaffolding back in place..

Thoughts?

kinow · 2020-03-30T03:12:48Z

Although, we could defer this to another PR and just put some scaffolding back in place..

Thoughts?

If you think that would be easier or quicker to review that change in another PR, then +1 for that. We can review that before the deltas (which I at least will spend some more time writing JS code for Cylc UI to use and test this PR)

dwsutherland · 2020-03-30T09:38:30Z

Although, we could defer this to another PR and just put some scaffolding back in place..
Thoughts?

If you think that would be easier or quicker to review that change in another PR, then +1 for that. We can review that before the deltas (which I at least will spend some more time writing JS code for Cylc UI to use and test this PR)

Done... In order not to make this PR too bloated, check added to the application of update deltas..
The loading of active task's job on restart can be a different/parallel PR.

kinow · 2020-03-30T21:02:23Z

Done

And done as in ready for tests with the UI again? Or done as in updated the PR but still gotta more things to be done? 😀

hjoliver · 2020-03-30T21:40:32Z

@dwsutherland - just trying to catch up here. Are you saying that restart won't work on this branch (and you can fix it in a follow-upon this)? But it did work before you removed the "scaffolding"? Can you explain what you meant by "scaffolding"? (I know I reviewed that PR, I wondered what you meant by scaffolding there too, but it seemed to be just removing undesirable exception catches as I recall).

dwsutherland · 2020-03-30T23:20:49Z

@dwsutherland - just trying to catch up here. Are you saying that restart won't work on this branch (and you can fix it in a follow-upon this)? But it did work before you removed the "scaffolding"? Can you explain what you meant by "scaffolding"? (I know I reviewed that PR, I wondered what you meant by scaffolding there too, but it seemed to be just removing undesirable exception catches as I recall).

Before this PR, on the application of deltas, I use to set a default element (just ID) and merge with the updates if it didn't exist in the data-store... However that meant we were potentially missing bugs (like this post restart existing job state update)..

Restarts still work. However, until we load job data from the DB we would miss displaying active task jobs post restart.

dwsutherland · 2020-04-03T02:17:48Z

Restarts still work. However, until we load job data from the DB we would miss displaying active task jobs post restart.

PR to fix: #3550

oliver-sanders · 2020-05-18T13:25:10Z

A couple of comments on cylc/flow/network/graphql.py which should be quick to sort out, I think this is ready.

I don't know if @hjoliver will get the time for a code review.

hjoliver · 2020-05-18T22:06:15Z

I don't know if @hjoliver will get the time for a code review.

I'll do my best, in the next two days. I already spent a good few hours on it a week or so ago, mostly to understand the method, but had to move on to other things. Thanks for your thorough review though!

oliver-sanders

🎉

oliver-sanders · 2020-05-20T14:36:36Z

Will leave for @hjoliver to review/merge on the alpha2 timeframe.

hjoliver · 2020-05-20T22:22:03Z

Brilliant, thanks for all the hard work on this @dwsutherland and the heavy duty reviewing @oliver-sanders. I'll take a final look and hit merge as soon as I can.

hjoliver · 2020-05-20T22:22:28Z

(and heavy duty functional testing @kinow !)

kinow

A few minor issues after enabling strip null in the UI. Working again (thanks to @dwsutherland !)

kinow

Synced today, and tested with the UI. Everything worked fine, re-approved! 🎉

Nested deltas, delta-store resolving.

hjoliver · 2020-06-18T07:20:42Z

cylc/flow/tests/network/test_server.py

@@ -77,7 +77,7 @@ def setUp(self) -> None:
                stopcp=None,
                check_point=True
            )
-            assert 0 == warnings
+            assert warnings == 0


He heh, there's something hilarious about the original line 🤣

hjoliver

🥇

* master: (61 commits) prepare 8.0a2 release CYLC_CONF_PATH: modify interpretation * if specified it must exist * if it's not readable you get and error because thats your fault * it can specify a file or a directory * also use pathlib because it's pretty scheduler_cli: add asyncio notes tests: fix version dependent test prepare 8.0a2 release tests: fix tests/f/cylc-ping/03 scheduler: shutdown logic for paritally initiated flows tests/i: tidy test utilities away out of sight pytest: exclude un-collectable test file scheduler_cli: simplify cli teardown tests/i: simplify teardown pycodestyle++ tests/i: suppress NFS teardown issues tests/i: fix asyncio event loop issues asyncio: shut down workflow tasks cleanly tests/i: fix nfs issues with integration testing cleanup/teardown tests/i: forward port test changes from cylc#3500 tests/i: clean tests tests: documentation setup: add missing test dependency ...

dwsutherland added the POC Proof of Concept label Feb 8, 2020

dwsutherland added this to the cylc-8.0a2 milestone Feb 8, 2020

dwsutherland requested review from kinow and oliver-sanders February 8, 2020 09:53

dwsutherland self-assigned this Feb 8, 2020

dwsutherland force-pushed the uis_delta_subs branch 2 times, most recently from 91b38c4 to 892af23 Compare February 8, 2020 12:23

dwsutherland mentioned this pull request Feb 8, 2020

UIS Delta subscriptions & GraphQL null stripping back-end cylc/cylc-uiserver#118

Merged

6 tasks

dwsutherland force-pushed the uis_delta_subs branch from 892af23 to 86cbb92 Compare February 17, 2020 04:16

oliver-sanders reviewed Feb 17, 2020

View reviewed changes

cylc/flow/network/__init__.py Outdated Show resolved Hide resolved

cylc/flow/data_store_mgr.py Show resolved Hide resolved

cylc/flow/network/graphql.py Outdated Show resolved Hide resolved

oliver-sanders reviewed Feb 17, 2020

View reviewed changes

cylc/flow/network/subscriber.py Outdated Show resolved Hide resolved

cylc/flow/network/graphql.py Outdated Show resolved Hide resolved

dwsutherland force-pushed the uis_delta_subs branch from 86cbb92 to 836d31c Compare February 18, 2020 01:49

kinow mentioned this pull request Feb 20, 2020

Infinite tree cylc/cylc-ui#345

Closed

12 tasks

dwsutherland force-pushed the uis_delta_subs branch from 836d31c to 224f0fa Compare February 24, 2020 21:40

kinow mentioned this pull request Mar 2, 2020

Incremental updates with GraphQL deltas cylc/cylc-ui#420

Closed

dwsutherland force-pushed the uis_delta_subs branch 2 times, most recently from 9954e97 to a9277b5 Compare March 10, 2020 07:43

dwsutherland mentioned this pull request Mar 26, 2020

Remove job pool scaffolding #3536

Merged

6 tasks

dwsutherland force-pushed the uis_delta_subs branch from a9277b5 to 66252f6 Compare March 27, 2020 02:27

dwsutherland force-pushed the uis_delta_subs branch from 66252f6 to 75b691c Compare March 30, 2020 09:29

dwsutherland force-pushed the uis_delta_subs branch from 75b691c to a321b56 Compare April 3, 2020 02:48

dwsutherland modified the milestones: cylc-8.0a3, cylc-8.0a2 May 18, 2020

dwsutherland force-pushed the uis_delta_subs branch from 20a779f to 92fdab2 Compare May 19, 2020 10:49

oliver-sanders approved these changes May 20, 2020

View reviewed changes

dwsutherland force-pushed the uis_delta_subs branch 3 times, most recently from 282bc63 to 268d5ad Compare May 25, 2020 00:40

kinow approved these changes May 25, 2020

View reviewed changes

dwsutherland force-pushed the uis_delta_subs branch 2 times, most recently from 36b4232 to a3218bd Compare May 27, 2020 11:07

kinow approved these changes May 28, 2020

View reviewed changes

dwsutherland added 6 commits June 9, 2020 21:09

Cylc GraphQL backend & grouped deltas

519bbab

delta added/updated split

adfe353

Add null stripping GraphQL args (client decides).

bd64b5d

Nested deltas, delta-store resolving.

Delta driven root subscriptions

42c460a

ignore delta interval added

e664ba6

Delta review fixes, stripNull fix & cascade

04d5cb3

dwsutherland force-pushed the uis_delta_subs branch from a3218bd to 04d5cb3 Compare June 9, 2020 09:15

hjoliver reviewed Jun 18, 2020

View reviewed changes

hjoliver approved these changes Jun 18, 2020

View reviewed changes

hjoliver merged commit 171f4fb into cylc:master Jun 18, 2020

oliver-sanders added a commit to oliver-sanders/cylc-flow that referenced this pull request Jun 18, 2020

tests/i: forward port test changes from cylc#3500

6c79c30

oliver-sanders mentioned this pull request Jun 18, 2020

tests: integration test battery #3654

Merged

8 tasks

oliver-sanders added a commit to oliver-sanders/cylc-flow that referenced this pull request Jun 19, 2020

tests/i: forward port test changes from cylc#3500

cd7031e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UIS Delta subscriptions & GraphQL null stripping back-end #3500

UIS Delta subscriptions & GraphQL null stripping back-end #3500

dwsutherland commented Feb 8, 2020 •

edited by oliver-sanders

oliver-sanders left a comment

oliver-sanders commented Mar 16, 2020

dwsutherland commented Mar 16, 2020

dwsutherland commented Mar 30, 2020 •

edited

kinow commented Mar 30, 2020

dwsutherland commented Mar 30, 2020 •

edited

kinow commented Mar 30, 2020

hjoliver commented Mar 30, 2020

dwsutherland commented Mar 30, 2020

dwsutherland commented Apr 3, 2020

oliver-sanders commented May 18, 2020 •

edited

hjoliver commented May 18, 2020

oliver-sanders left a comment

oliver-sanders commented May 20, 2020

hjoliver commented May 20, 2020

hjoliver commented May 20, 2020

kinow left a comment

kinow left a comment

hjoliver Jun 18, 2020

hjoliver left a comment

UIS Delta subscriptions & GraphQL null stripping back-end #3500

UIS Delta subscriptions & GraphQL null stripping back-end #3500

Conversation

dwsutherland commented Feb 8, 2020 • edited by oliver-sanders

oliver-sanders left a comment

Choose a reason for hiding this comment

oliver-sanders commented Mar 16, 2020

dwsutherland commented Mar 16, 2020

dwsutherland commented Mar 30, 2020 • edited

kinow commented Mar 30, 2020

dwsutherland commented Mar 30, 2020 • edited

kinow commented Mar 30, 2020

hjoliver commented Mar 30, 2020

dwsutherland commented Mar 30, 2020

dwsutherland commented Apr 3, 2020

oliver-sanders commented May 18, 2020 • edited

hjoliver commented May 18, 2020

oliver-sanders left a comment

Choose a reason for hiding this comment

oliver-sanders commented May 20, 2020

hjoliver commented May 20, 2020

hjoliver commented May 20, 2020

kinow left a comment

Choose a reason for hiding this comment

kinow left a comment

Choose a reason for hiding this comment

hjoliver Jun 18, 2020

Choose a reason for hiding this comment

hjoliver left a comment

Choose a reason for hiding this comment

dwsutherland commented Feb 8, 2020 •

edited by oliver-sanders

dwsutherland commented Mar 30, 2020 •

edited

dwsutherland commented Mar 30, 2020 •

edited

oliver-sanders commented May 18, 2020 •

edited