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

async scan updates for the UI #493

Merged
merged 3 commits into from
Sep 14, 2020
Merged

async scan updates for the UI #493

merged 3 commits into from
Sep 14, 2020

Conversation

kinow
Copy link
Member

@kinow kinow commented Sep 9, 2020

This is a small change with no associated Issue.

Related to cylc/cylc-flow#3724 and cylc/cylc-uiserver#150

This PR contains a patch that allows the UI to show stopped workflows, added in the two issues above.

  • Allow the Cylc UI app to work with latest Cylc Async Scan changes, showing stopped workflows
  • Change the opacity of workflow items in GScan (opacity: 0.5)

Requirements check-list

  • I have read CONTRIBUTING.md and added my name as a Code Contributor.
  • Contains logically grouped changes (else tidy your branch by rebase).
  • Does not contain off-topic changes (use other PRs for other changes).
  • Already covered by existing tests.
  • Appropriate change log entry included.
  • No documentation update required.

@kinow
Copy link
Member Author

kinow commented Sep 9, 2020

Assigning @dwsutherland and @oliver-sanders as both are familiar with the async scan PR's. Found a few strange behaviours in the UI, that can be fixed I think, but thought it would be good to check what we should be doing about them.

  1. The GraphQL endpoint returns the initial data burst for a stopped workflow

This breaks the code of the UI as we were validating that the initial data burst would contain information to build the workflow tree.

I think that's intentional? I've commented the throw new Error, and replaced by a silent return to fix it.

The Lumino tree widget is still added. Should we not add it?

Or maybe we leave as-is, until we are working on #332?

  1. GScan subscription does not cope well when a workflow is stopped.

image

With deltas, we have a payload with data about the workflow shutdown. The query of GScan doesn't use deltas, so it never realizes that the workflow has stopped.

I can add a wrapper <div v-if="node.status !== 'stopped'">...</div> to prevent the icons being displayed. But we could still have some JS code running for that.

Or we can just wait until GScan is re-written with deltas?

  1. Reloading the page for a workflow stopped, shows old data

Here I have five running.

image

Here's the same view after I've stopped the workflow.

image

And finally, here's after I refresh the page (F5).

image

We get the last state of the workflow in the initial data burst, and the UI simply builds the tree.

We could add an if somewhere to prevent that when the workflow status is stopped… but not sure if that's the best option?

@kinow
Copy link
Member Author

kinow commented Sep 9, 2020

@dwsutherland the code changes here are small, but you can do just a functional test to review this PR 👍 I will update the unit tests later, after we clear the questions above.

@dwsutherland
Copy link
Member

This breaks the code of the UI as we were validating that the initial data burst would contain information to build the workflow tree.
I think that's intentional? I've commented the throw new Error, and replaced by a silent return to fix it.
The Lumino tree widget is still added. Should we not add it?

Yes, the burst is intentional, as registered and never run workflows won't have any data to form a tree (and never will, without a workflow DB).. But you still need the burst of whatever info we can gather about it.

So I'd say either, just don't add the widget, or add it with blank data so it can spring to life when the workflow starts running (?)

I can add a wrapper

...
to prevent the icons being displayed. But we could still have some JS code running for that.

IMHO - The icons for stopped workflows are fine, as it just shows the last state the workflow was in on shutdown... And can differentiate between registered but never run and run but stopped (or DB loaded) workflows.. The (?) icon could probably be more informative, and perhaps the whole row (name and icons) faded for stopped workflows (but that'll be for another PR).

We could add an if somewhere to prevent that when the workflow status is stopped… but not sure if that's the best option?

Built tree is probably fine? Or is that a problem

@kinow
Copy link
Member Author

kinow commented Sep 9, 2020

Thanks for the answers @dwsutherland !

All good for me. On the last one only:

We could add an if somewhere to prevent that when the workflow status is stopped… but not sure if that's the best option?

Built tree is probably fine? Or is that a problem

That's a problem I think, because at the moment, without refreshing, you won't see anything in the lumino tab, because we are cleaning the tree when we receive a deltas indicating workflow shutdown.

From your previous answers, I think we could then not remove the items when we receive a shutdown, and instead leave them in the UI and just change the workflow status in GScan?

@dwsutherland
Copy link
Member

From your previous answers, I think we could then not remove the items when we receive a shutdown, and instead leave them in the UI and just change the workflow status in GScan?

Yeah, sounds good 👍

One other thing, does this mean a subscription for each (dead or alive) workflow will be created? Any down sides to that?

@kinow
Copy link
Member Author

kinow commented Sep 9, 2020

One other thing, does this mean a subscription for each (dead or alive) workflow will be created? Any down sides to that?

I don't think so. Only when you click on the workflow in GScan, then a new subscription will be created for that workflow (dead or alive 🤠 ). Otherwise you should have 1 or 2 subscriptions depending on the view you are browsing (GScan subscription {workflows } and if you have a tree widget the subscription { deltas }).

Copy link
Member

@dwsutherland dwsutherland left a comment

Choose a reason for hiding this comment

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

Works great, approved subject to tests and discussion changes 👍

@kinow
Copy link
Member Author

kinow commented Sep 9, 2020

Thanks @dwsutherland , will wait for @oliver-sanders to have time to take a look at the questions and code change. @hjoliver if you have time and would like to just give your opinion. Don't need to worry too much about code as @oliver-sanders can do a quick code review, just your opinion if there's anything else I should change in this PR for showing stopped workflows 👍 (all good if you don't have time, we can improve later in other PRs)

@oliver-sanders
Copy link
Member

GScan subscription does not cope well when a workflow is stopped.

This is an interesting one, I think you are handling it just fine.

Context:
In the Cylc7 GScan if a workflow stops with running tasks its task status icons (state total square thinggies) remain (they do get a kinda checkerboard pattern though).

If you were to take a look in the suite database those tasks would still be in the pool, they were "active" tasks when the workflow shut down so until it is restarted they must be considered to be "active".

Future work:

  • We will need to do some work at the UI Server to query the database so that we get these icons when the UIS starts up.
  • I think stopped workflows should show in GScan with some transparency, this will help convey that these "task status icons" are showing the status of a stopped workflow.

@oliver-sanders
Copy link
Member

oliver-sanders commented Sep 9, 2020

Reloading the page for a workflow stopped, shows old data

Similar answer as for the last question. This behaviour is mostly correct, this data could alternatively come from the database.

We need two distinct "modes" for viewing workflows:

  • n={0,1,2,3} - show all tasks N edges from active tasks.
  • n=cycle - show tasks by cycle point (I think we have called this N=max sometimes)

For live workflows it makes sense to default to n=1 (ala cylc gui) for stopped workflows it makes sense to default to n=cycle (ala cylc review).

From the UI perspective is shouldn't make much of a difference whether a workflow is running or not, its the UI Server which picks up all this horrible complexity and has to work out whether it can get data from a live scheduler or whether it has to go to the database instead.

The UI should just display what data it gets given! So all good here I think.

@kinow
Copy link
Member Author

kinow commented Sep 13, 2020

I think stopped workflows should show in GScan with some transparency, this will help convey that these "task status icons" are showing the status of a stopped workflow.

Great idea @oliver-sanders . Set it to opacity: 0.5. It looks good in my monitor/browser, but let me know if this value should be tweaked. It's much easier to see what's running this way (as a complement to icons/colors)

image

@hjoliver
Copy link
Member

Oooh, that's very nice!

@kinow
Copy link
Member Author

kinow commented Sep 13, 2020

The UI should just display what data it gets given! So all good here I think.

Right. I am only concerned about consistency now.

If I am looking at Cylc UI with a single lumino widget showing the tree view of a workflow, and then I stop it, the lumino widget won't have anything.

But if I open the same URL in another tab, then it will display the latest state of the workflow.

I'm adding two more commits here:

  1. when a deltas.shutdown is received, instead of clearing the CylcTree object (that reactively populates the lumino widget tree), I will simply set the state of the workflow to stopped and leave the UI elements
  • We need to distinguish a workflow running or not, so that if the workflow is restarted, we know that we need to re-create the existing CylcTree object (i.e. when the user stopped, it could have been delta number 100, and now that we restarted, the UI elements are still showing delta 100, but the first delta received could be number 400, so it would fail to apply the delta and leave the old data forever in the UI)
  1. Fix unit tests

Then it should be good to go 🤞

@hjoliver
Copy link
Member

I guess we need a stopped icon (not question mark) for stopped flows in gscan.

@kinow
Copy link
Member Author

kinow commented Sep 14, 2020

From RIOT (hope it's OK to copy that over here @dwsutherland / @hjoliver )

image

I will try to update the UI code to handle the shutdown: true as an indicator that the workflow is shutting down, and leave the UI elements in a valid state (whether it's stopped, running, restarted, etc)

@oliver-sanders
Copy link
Member

All sounds/looks good to me.

For follow-on it would be good to sort GScan so that active suites appear above inactive ones (some people have insane numbers of stopped suites).

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.

Looks good.

Screenshot 2020-09-14 at 13 52 44

Can go in once the tests pass.

…ors when a workflow does not have tree data)
@codecov-commenter
Copy link

Codecov Report

Merging #493 into master will decrease coverage by 0.19%.
The diff coverage is 87.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #493      +/-   ##
==========================================
- Coverage   61.99%   61.79%   -0.20%     
==========================================
  Files          41       41              
  Lines         942      945       +3     
  Branches       56       57       +1     
==========================================
  Hits          584      584              
- Misses        340      342       +2     
- Partials       18       19       +1     
Flag Coverage Δ
#unittests 61.79% <87.50%> (-0.20%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/components/cylc/gscan/GScan.vue 57.14% <75.00%> (-0.76%) ⬇️
src/components/cylc/gscan/index.js 92.30% <90.90%> (+0.64%) ⬆️
src/components/cylc/tree/index.js 100.00% <100.00%> (ø)
src/components/cylc/tree/deltas.js 96.00% <0.00%> (-4.00%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e8a3eae...812f772. Read the comment docs.

@kinow
Copy link
Member Author

kinow commented Sep 14, 2020

Going to merge this one as-is, and move the rest of the work for stopped workflows for another issue. That way it gets simpler to review, and also the stopped needs the two PR's that @dwsutherland created yesterday.

@kinow
Copy link
Member Author

kinow commented Sep 14, 2020

Looks good.

Screenshot 2020-09-14 at 13 52 44

Can go in once the tests pass.

Thanks @oliver-sanders !

@kinow kinow merged commit 2eea489 into cylc:master Sep 14, 2020
@kinow kinow deleted the async-scan-ui branch September 14, 2020 22:16
@kinow
Copy link
Member Author

kinow commented Sep 14, 2020

Will also include the changelog for showing offline workflows in the other PR (forgot it, sorry!)

@kinow
Copy link
Member Author

kinow commented Sep 22, 2020

b51564b adds the changelog for this PR retroactively

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