-
Notifications
You must be signed in to change notification settings - Fork 27
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
graph view and central data store #1108
Conversation
Task modifiers and progress indicators have been disabled so far due to a couple of issues with them:
I've re-worked the task icon to address this. It's a WIP, I'm aware that the modifiers are too small and the progress indicator isn't calibrated. More fun for later. |
I have identified the issue which is preventing the Tree and Graph views from co-existing, written up here - #1110 |
Update:
Up next:
|
Fantastic 🎉 |
Update: and then there were three
|
Have just discovered a roadblock after converting GScan to use the central data store. The UIS is issuing an erroneous updated delta after the pruned delta issued when a workflow is removed. Consequently if you remove a workflow it will briefly disappear from GSscan before being subsequently re-added. Hopefully a quick fix 🤞. |
Semi-accidental side-effect of the changes made here (and also the current lack of data store housekeeping 🙈). If you have hierarchical workflows, load one of them, then change the URL to go up a level, the tree view will now display a multi-workflow tree: We can also do this in GScan by setting This will break once housekeeping is hooked up again, however, it demonstrates that this should be easy to implement later on. Have written up the next steps for this on in #1126. [update] Housekeeping is now implemented, the free lunch is over. |
Update: This ballooned quite a lot as a few issues caused roadblocks, but nearly there.
Up Next:
|
f730687
to
25eb8b9
Compare
Rebased and deconflicted. |
25eb8b9
to
f5eb9a2
Compare
A bit more spit and polish required so I've not put this up for review yet, but this should now be fully functional! One thing I've left to do is to make the task modifiers a bit more visible in the tree view. Any early review / feedback welcomed. I've not managed to solve the WASM packaging issue (see the top of the README), if anyone has any ideas please do say as the best I can think to do is to shim a |
6f10ec7
to
38f6da3
Compare
The last commit should fix the Firefox test. The one before helps with debugging Cypress tests by uploading any screenshots of the test failures as artefacts. |
// This is a recursive function which will be called up to 10 times. We can't | ||
// do this in a for loop as otherwise the waits aren't chained correctly. | ||
if (depth > 10) { | ||
expect('graph loaded').to.equal(true) |
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.
Unless I am missing something, this is 'graph loaded' === true
which will always fail. Presumably it's not being hit?
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.
Yep, this test is guaranteed to fail. It gets called if the graph doesn't load in time.
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.
Ah, I understand now. According to the chai docs you should be able to do
expect('graph loaded').to.equal(true) | |
expect.fail('graph did not load') |
Co-authored-by: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com>
state: jobState | ||
} | ||
} | ||
if (jobState === 'running') { // TODO constant |
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.
Comment is at best cryptic, at worst suggests somthing else you want to do.
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.
My review is largely functional, but I have had a quick (per file, it took ages) poke through the changes.
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've had another good functional review yesterday and this morning. Nothing I have found will block this merge and so approving. Couple of small notes, whether they require fixing is up for debate and can certainly be done in follow-ups...
- Transpose, I noticed, in my case where my simple linear graph (
task1 => task2...=>last-task
) had stopped the transpose no longer worked and simply shifted the arrow a little. I couldn't replicate this while the workflow was playing so I expect it is no big deal. - When pulling the graph view to a vertical pane on the right of the window, the graph didn't auto-centre and so it was tricky to see the tasks. Again, no big deal, since clicking the centre button myself fixed this issue.
- The timer countdown animation continues to go down when the task is held.
Code-wise, thanks for all the comments. It has helped my understanding of the changes and made things much easier. My vue is still in the learning stages, I've not spotted anything that needs changing.
Data-store change-wise, the changes look good to me, I have looked at the structure and it makes sense to me.
Subscription changes-wise, I'm still a little lacking in understanding as to why we have an onAdded and onUpdated as it looks to me that the same info is coming in on both, although this is not really related to your pr and is just something I don't fully understand and will pick your brains about another time.
All in all, very impressive pr 🥇 👏 Any comments I have are very minor.
[ | ||
new GScanCallback() | ||
] | ||
[] |
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.
Nice that these callbacks have gone. I was wondering about these when doing the log view.
@datamel could you elaborate on this one:
Is that a running task that was held whilst it was running? Or was the task status not running but the clockface showing erroneously? |
This one, I guess maybe problem with the held icon being there rather than the clockface? |
Could do with a screenshot or something as I'm not sure I understand. "Holding" a task doesn't actually pause its execution, it just suppresses new job submissions so I would expect the clockface to continue to count-down for a running task even if it's held. |
ee6434d
to
ddd3671
Compare
e3b82f2
to
18e1ac8
Compare
The last commit should enable component tests in CI. Will add comparisons to known-good-output with #178 to ensure task/job/graph-node icons aren't accidentally changed. |
18e1ac8
to
3d28f92
Compare
Ok, that was a total failure, I'll get the component tests sorted out in the next PR. |
Features:
Basic graph view now implemented, will push family expand / collapse and other features into future issues.
Once finished all views will be powered off of the central data store with no local stores or callbacks.
Sorting is now performed in the central data store. Where views require a different sort order (e.g. GScan) they use computed properties.
Added workflow icon and status column whilst converting to run on the central data store.
The way custom task outputs are listed had to be re-written as a result of the migration to the central data store. The easiest way to close outputs: task outputs can be mapped onto all jobs #1129 was to use messages, closes Display non-standard non-output messages #595 in the process.
Bugfixes:
Once finished will have removed the local callbacks from which this error can occur.
Fixed bug by re-creating the query object before submitting it.
GScan sorting has been re-implemented.
This had the same root-cause as the issue which made the old task component fall apart when used in the graph. Solved in the re-write.
Task outputs are now filtered by job messages to ensure they are correctly associated with the job which created them.
Partially addresses:
All workflow are now present in a single tree. From the perspective of most views, it doesn't really matter if they are given a workflow (e.g.
~u/w/run1//
) or workflow-part (e.g.~u/w
) node.The tree view now supports multiple workflow nodes. If there is only one workflow (as at present) it will strip that workflow node (so that cycle points are at the top level).
The graph view added here will also run off of offline data when it is added at the UIS.
Needed a toolbar for the graph view so created a reusable component.
Proof of conceptImplementation of the graph view running directly off of a central data store.Overview
Features
Vue ❤️ SVG
Vue can work with SVG document just as well as HTML ones.
This means we can reuse our existing "Task" and "Job" components with an SVG approach. We can even use the mutation menus in exactly the same way as we would normally.
Reactive SVG 💥
How It Works
The graph is contained in a single reactive SVG document which lives for the life of the graph view.
A timer calls a refresh function (every 2 seconds at the time of writing).
The refresh function lists nodes and edges from the data store and constructs a hash representing the current graph. The hash is compared to the pervious one, if they differ the layout needs to be updated.
The graph nodes (i.e. tasks) are added into the SVG document. There's no layout information so they sit on top of each other in a stack.
Next we define the graph in GraphViz "dot" format. In Cylc 7 we gave nodes text labels e.g.
answer\n42
, here I've given nodes HTML-like labels which allows us to control their structure e.g:You'll notice there are width and height values in the table. These have been extracted from from the rendered nodes in the SVG document. This way the dimensions of the nodes in the SVG document exactly matches the dimensions in the GraphViz document.
Also note those "PORT" attributes, these control where GraphViz routes edges from and to. I've placed these ports above and below the the task icon which constrains the edges to a fairly small space making for more regular graphs.
We then get GraphViz to layout the graph, but rather than rendering it, we get GraphViz to give us the layout in JSON format. If it were rendered it would look like this:
The graph holds the structure we want but with the details missing.
We then extract the node coordinates from the GraphViz output and convert them into SVG transforms which can be applied to the nodes in the SVG document.
Finally we extract the edge information from the GraphViz output, convert them into SVG format and add them to the SVG. We now have a graph that looks like this:
It's exactly the same graph as GraphViz produced but we've taken control of the rendering. This way we can have a perpetual SVG document which we update rather than continuously creating / destroying it. This is more efficient but it also means that we can elegantly preserve things like user-selection and zoom/pan coordinates.
Data Store
Currently (on master) the data store contains a flat store of nodes (the lookup).
The tree view wants these nodes in a tree structure so it registers a callback which allows it to intercept every added/updated/pruned delta and maintain its own local data store.
The GScan view also wants these nodes in a tree structure so registers a similar callback.
Many views are likely to want a tree structure as this tree is quite fundamental to Cylc:
Or to put it another way:
I've generalised the approach to create a single callback which handles the whole universal ID (UID) and manages a new bit of the central data store to reflect this tree structure. It's kinda like a mix of the central data store and the tree view data store.
It's completely de-coupled from the existing store as I plan to either update the callbacks for the other views to feed off of the new bit of the store, or, if possible, run them off of the new bit of the store directly.
Most of the approach is inherited from the current implementation, it uses the same node merging etc, but the structure has changed. E.G. if we added this node to an empty store:
We would get a data store which looks like this:
The graph view is running directly off of this data store, no local callbacks needed, the one generic callback can power all views. This reduces the delta handling overheads as we currently have two callbacks per view meaning each delta is processed twice per open view (GScan counts as a view).
Running It
There are a couple of issues at present which you'll have to work around if you want to play with this.
I haven't managed to get webpack to play with WASM nicelyThe first time you build on this branch run the following command (remember to
yarn install
first):I've run into an issue with subscription merging, it will either run the tree subscription OR the graph one.I think something is going wrong in the GraphQL subscription merging (or I wrote it wrong, equally likely!). Haven't taken a look yet, for now the workaround is to first close the tree view and then open the graph view.
The graph doesn't auto centre (yet), you'll need to zoom out and pan around.IgnoreError: <path> attribute d: Unexpected end of attribute.
warnings in the console.They appear to be harmless, need to sort them out.
Requirements check-list
CONTRIBUTING.md
and added my name as a Code Contributor.