Add CylcTree and use Deltas #458
Conversation
|
The current code produces:
That's in the Tree view. The data is built on the first response from GraphQL endpoint, using the initial burst of data to build the workflow hierarchical structure (i.e. workflow, with cyclepoints as children, with families as children, etc etc). It is not using the WorkflowService. I tried using that in the other branch, and found some issues with how we were maintaining the state between Vue views/routes (fixed already). And also had issues around query merging. The query now is a I am also using query variables (see #234) and fragments as otherwise the query was getting really long and with repeated parts in the deltas (see #439). |
|
Next step: apply the deltas to the |
Codecov Report
@@ Coverage Diff @@
## master #458 +/- ##
===========================================
- Coverage 58.52% 45.07% -13.45%
===========================================
Files 51 39 -12
Lines 1015 985 -30
Branches 75 64 -11
===========================================
- Hits 594 444 -150
- Misses 408 528 +120
Partials 13 13
Continue to review full report at Codecov.
|
kinow
force-pushed
the
add-cylc-tree-ds
branch
2 times, most recently
from
5a9c5f6 to
790322a
Compare
kinow
force-pushed
the
add-cylc-tree-ds
branch
2 times, most recently
from
3805818 to
ce9576e
Compare
This comment has been minimized.
This comment has been minimized.
|
The
I guess that could be the left-over in the channel/queue. So safe to just ignore these messages. |
Performance benchmarkEnvironment: Ubuntu LTS, browser firefox in background with tab opened branch add-cylc-tree-dsThis branch with no workflows, consumes between 10 and 12 MB. fiveStarting Stopped workflow, waited a couple minutes, then pressed the GC button. The workflow is quite simple, so it is not easy to spot nay performance
complexThe complex workflow started with 28 MB. Going up to 32 MB, then back to 28 MB. Stopped the workflow, waited a couple minutes, then pressed the GC button. Below is the screenshot of 2 minutes profiling with the workflow. The workflow
Painting didn't happen a lot. And ~3 seconds scripting, and <200 ms rendering After this test, I did another test now scrolling the page, expanding/collpasing Memory kept under 40MB, and the app was responsive with no issues. branch masterThis branch with no workflows, consumes between 12 and 14 MB. fiveMemory varied between 11 and 17 MB. complexA five minute test with complex. Memory started at 39 MB, going to 70 MB in a few seconds. It then varied between 40 MB and 90 MB for the first two minutes. After that it reached 100 MB, and oscillated between 40 and 100 MB for the rest of the test. But looking at the 2 minutes profiling graph with
The sawtooth pattern is not as nice as with the deltas, and you an see that it spent a long time running long tasks (the ones in red). And spent near half of the time in scripting (juggling with the new data, placing/removing elements from the DOM). ConclusionThe deltas appear to reduce the memory on both cases. The memory on We dot not seem to have memory leaks on I changed the workflow The processing/scripting needed is way better with deltas and this branch. Users with slower computers, or using a browser with several tabs (e.g. youtube playing a video on another tab) would notice a huge improvement as the browser doesn't need to work as hard with deltas. |
|
Does this branch include the infinite tree work or is that to follow later? |
That will come later. Probably first simply doing something like |
|
Doing good progress on fragments. Mainly using GraphiQL and a bit of JS in the firefox console to play with ApolloClient. I am thinking possible alternatives for this branch so that we can use deltas in the rest of the system. Some ideas end up with issues in how we use the ApolloClient and queries in Cylc UI. @dwsutherland helped me with some questions about GraphQL today on Riot (thanks again!). So far the way forward appears to be similar to what we have here: 1 WebSocket connection, but 2 subscriptions. One subscription belonging to GScan + Dashboard data. And another subscription for the Tree data. The second subscription would exist only on Views that display workflow information, such as |
|
Approach I am trying in the next days: https://gist.github.com/kinow/d505938ccb5f3dfe87c585ddb333956a Use the Subscription from the Gist above. Instead of using the two fragments in the subscription, use it as a string template, i.e. export const WORKFLOW_SUBSCRIPTION = `
subscription OnWorkflowDeltasData ($workflowId: ID) {
deltas (workflows: [$workflowId]) {
$FRAGMENTS
}
}
`Then replace On the Tree view, On the Graph view, On the Workflow view, If the user adds another Tree widget, we check whether there is already a Tree fragment. If true, ignore. Otherwise we add the tree fragment. If the user adds a Graph widget, we check whether there is already a Graph fragment. If true, ignore. Otherwise we add the graph fragment to the subscription. When the user leaves the page, we stop the subscription. With this I believe we would have deltas working in the UI and ready for review. Users would have 1 extra subscription, but that appears to be the only way, as I was not able to have a
|
Codecov Report
@@ Coverage Diff @@
## master #458 +/- ##
===========================================
+ Coverage 57.07% 69.35% +12.27%
===========================================
Files 53 57 +4
Lines 1046 1266 +220
Branches 75 79 +4
===========================================
+ Hits 597 878 +281
+ Misses 435 367 -68
- Partials 14 21 +7
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
Build might stay broken for a little while until I finish experimenting with query fragments in the WorkflowService 👍 |
kinow
force-pushed
the
add-cylc-tree-ds
branch
3 times, most recently
from
3bcf2df to
05f63cb
Compare
|
Deltas working on the Tree view, and on the Workflow view, for the Tree component. However, at the moment it is blocked by #454 , as I'm getting more and more subscriptions added. And adding/removing tree widgets, the UI is getting confused. That happens because the deltas are always applied, but if you have removed the widgets, then you must stop the subscription (hence #454). Otherwise it will keep trying to update the tree, just to find it |
|
Hmm, or maybe I should just merge that PR with this one, and rebase later if that one is merged first. 💭 |
|
Sorry @kinow that PR slipped through the gaps, will try to get in soon. |
kinow
force-pushed
the
add-cylc-tree-ds
branch
2 times, most recently
from
67cd229 to
27f5aa0
Compare
…nt has been removed
Create a single Delta Susbcription in the WorkflowService, while keeping the existing code untouched (as much as possible). Add tests for subscriptions, which are not enforcing the correct number of susbcriptions yet (follow-up), but are working and just need to be updated to match the expected number of subscriptions and the TODO/FIXME marker to be removed. Update other tests and offline mode files to work as before.
…vert changes to simplify the code review
…s. Remove it when no more tree widgets.
|
Rebased following our 0.2 release 👍 |
|
Sorry, I'm still stuck building this: $ git fetch bruno
$ git checkout bruno/add-cylc-tree-ds
HEAD is now at 8d58958 Keep 1 susbcription with deltas only, no matter number of tree widgets. Remove it when no more tree widgets.
$ rm -rf node_modules/
$ yarn install
yarn install v1.22.4
...
✨ Done in 21.49s.
$ yarn run build
yarn run v1.22.4
$ vue-cli-service build
⠸ Building for production...
ERROR Failed to compile with 1 errors 16:01:53
This dependency was not found:
* @/components/cylc/tree/Tree in ./src/components/cylc/workflow/index.js
To install it, you can run: npm install --save @/components/cylc/tree/Tree
ERROR Build failed with errors.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.I've tried with an entirely new Conda env but get the same result. Env info: $ conda env export | grep node
- configurable-http-proxy=4.2.1=node12_hbce0df8_0
- nodejs=12.4.0=h6de7cb9_0
$ which node
/Users/oliver/miniconda3/envs/cylc8/bin/node
$ node --version
v12.4.0Anything obviously off? |
It works for me 🤔 |
|
I cannot think of a reason for this error @oliver-sanders 😞 It works for me (similar Node version, but not using Conda, using nvm instead), the CI can run the tests (I believe it's Ubuntu latest with Node via apt?), and @hjoliver also successfully built it with Linux (and npm installed via Conda too I think?). I googled for the error plus "mac", but couldn't find anything that looked useful. It could be a dependency missing in your environment, maybe we can fix that by updating the dependency or adding it to the Could you run `yarn list`yarn list v1.22.4 The list is quite long. I think you can start by comparing dependencies that start with If you have access to another computer or VM with Linux, it might be worth trying the same commands you are using on Mac (I think you are on mac os? otherwise please ignore this) and see if everything works in that Linux box. |
$ diff mine his
--- mine 2020-07-31 08:36:39.000000000 +0100
+++ his 2020-07-31 08:39:29.000000000 +0100
@@ -7264,4 +7264,4 @@
│ ├─ tslib@^1.9.3
│ └─ zen-observable@^0.8.0
└─ [email protected]
-Done in 0.80s.
+Done in 1.46s.Dammit. I think I might have a handle on the failure, building on a Linux VM to confirm... Looks like a compiler configuration issue... Deleted and reinstalled compilers, fixed installation warning... $ diff new his
--- mine 2020-07-31 08:56:05.000000000 +0100
+++ his 2020-07-31 08:39:29.000000000 +0100
@@ -550,13 +550,11 @@
│ ├─ resolve-from@^5.0.0
│ └─ [email protected]
├─ @istanbuljs/[email protected]
-├─ @lumino/[email protected]
+├─ @lumino/[email protected]
├─ @lumino/[email protected]
-│ ├─ @lumino/algorithm@^1.2.3
-│ └─ @lumino/[email protected]
+│ └─ @lumino/algorithm@^1.2.3
├─ @lumino/[email protected]
│ ├─ @lumino/algorithm@^1.2.3
-│ ├─ @lumino/[email protected]
│ ├─ @lumino/coreutils@^1.4.2
│ ├─ @lumino/disposable@^1.3.5
│ ├─ @lumino/domutils@^1.1.7
@@ -566,7 +564,6 @@
├─ @lumino/[email protected]
├─ @lumino/[email protected]
│ ├─ @lumino/algorithm@^1.2.3
-│ ├─ @lumino/[email protected]
│ ├─ @lumino/coreutils@^1.4.2
│ ├─ @lumino/disposable@^1.3.4
│ ├─ @lumino/domutils@^1.1.7
@@ -580,7 +577,6 @@
│ └─ @lumino/widgets@^1.9.7
├─ @lumino/[email protected]
│ ├─ @lumino/algorithm@^1.2.3
-│ ├─ @lumino/[email protected]
│ └─ @lumino/signaling@^1.3.5
├─ @lumino/[email protected]
├─ @lumino/[email protected]
@@ -589,18 +585,14 @@
├─ @lumino/[email protected]
├─ @lumino/[email protected]
│ ├─ @lumino/algorithm@^1.2.3
-│ ├─ @lumino/[email protected]
│ └─ @lumino/collections@^1.2.3
├─ @lumino/[email protected]
├─ @lumino/[email protected]
-│ ├─ @lumino/algorithm@^1.2.3
-│ └─ @lumino/[email protected]
+│ └─ @lumino/algorithm@^1.2.3
├─ @lumino/[email protected]
-│ ├─ @lumino/algorithm@^1.2.3
-│ └─ @lumino/[email protected]
+│ └─ @lumino/algorithm@^1.2.3
├─ @lumino/[email protected]
│ ├─ @lumino/algorithm@^1.2.3
-│ ├─ @lumino/[email protected]
│ ├─ @lumino/commands@^1.10.1
│ ├─ @lumino/coreutils@^1.4.2
│ ├─ @lumino/disposable@^1.3.5
@@ -7272,4 +7264,4 @@
│ ├─ tslib@^1.9.3
│ └─ zen-observable@^0.8.0
└─ [email protected]
-Done in 1.01s.
+Done in 1.46s.Tried going back to npm: `npm run build```` $ npm run build
⠙ Building for production... ERROR Failed to compile with 97 errors 09:16:33 These dependencies were not found:
To install them, you can run: npm install --save core-js/modules/es6.array.sort core-js/modules/es6.regexp.to-string core-js/modules/es6.string.anchor core-js/modules/es6.string.fixed core-js/modules/es6.string.link core-js/modules/es6.string.small core-js/modules/es6.string.sub core-js/modules/es7.string.pad-start core-js/modules/es7.symbol.async-iterator @/components/cylc/tree/Tree npm ERR! A complete log of this run can be found in: |
|
GOT IT! (I think). Sooo, in a way it's kinda a Mac problem, or more specifically an AFS filesystem problem. AFS is not case-sensitive. There are other filesystems which have the same quirk (e.g. FAT32) and although NTFS is case-sensitive apparently Windows decides to shield users from this making it appear case-insensitive to the shell. When we import files in Vue we don't specify the extension (there is some internal wrangling presumably handling This creates a problem when you have two files with the same name but different extensions. Take a look as the FS tree: To AFS tldr: If we rename |
…ylc-tree.js This matches the class inside cylc-tree.js, CylcTree. Also prevents issues with file systems that are case insensitive (bug found by @oliver-sanders).
|
@oliver-sanders renamed Did a quick test with Thanks for the great detective work! |
|
✨ Done in 39.76s. 🎉 |
|
Added a new commit that updates the offline mock service (i.e. no need to worry about the change here). commit ca68fa49ae8300707aaf48fe9c44ec661cd95db8 (HEAD -> add-cylc-tree-ds)
Author: Bruno P. Kinoshita <[email protected]>
Date: Mon Aug 3 10:36:47 2020 +1200
Update offline mock service, no need to use a Promise
diff --git a/src/services/mock/workflow.service.mock.js b/src/services/mock/workflow.service.mock.js
index c385d5e..95e6f22 100644
--- a/src/services/mock/workflow.service.mock.js
+++ b/src/services/mock/workflow.service.mock.js
@@ -65,18 +65,15 @@ class MockWorkflowService extends GQuery {
this.subscriptions.push({
subscription
})
- return new Promise((resolve, reject) => {
- subscriptionOptions.next({
- data: {
- deltas: {
- added: {
- workflow: checkpoint.workflows[0]
- }
- },
- workflows: checkpoint.workflows
- }
- })
- resolve(subscription)
+ subscriptionOptions.next({
+ data: {
+ deltas: {
+ added: {
+ workflow: checkpoint.workflows[0]
+ }
+ },
+ workflows: checkpoint.workflows
+ }
})
}
It matches the signature of the "online" service, and also avoids the unnecessary |
There was a problem hiding this comment.
I've finally got this working again (after dealing with bugs totally un-related to this change 🤢) 😌!
Inspecting the websocket messages the "stop" signals are sent correctly and I am unable to break it!
Only one sub is created per-tree window per workflow 👍.
The graph view is very unhappy but I think this is SoD related and the topic for further work once we've worked out how we are going to re-align the data-store / subs to address the changes.
CHANGES.md
Outdated
| @@ -57,6 +57,9 @@ increase, decrease, and reset UI font size. | |||
| view using graphiql.js that sends queries to the backend graphiql | |||
| endpoint. | |||
|
|
|||
| [#458](https://github.com/cylc/cylc-ui/pull/458) - Add CylcTree and | |||
| use deltas for the Tree view and component. | |||
There was a problem hiding this comment.
Just realized I need to move it to 0.3 version. Sending a new commit in a few minutes.
|
Added a new commit moving the change log entry to 0.3 👍 no other changes, shouldn't break CI or change any code/feature here. |
|
(Was using pre-SOD cylc-flow and cylc-uiserver, so this merge might make things a little broken for a while ...) |
Closes #420
Related to #453
Related to previous (now closed) PR #450
CylcTree
This branch contains the work derived from #450. Mainly the data structure
CylcTree, which is simply an array+hierarchy, with a helper map for quick access of elements in the tree (useful to quickly add/remove with deltas).Requirements check-list
CONTRIBUTING.mdand added my name as a Code Contributor.