-
Notifications
You must be signed in to change notification settings - Fork 18
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
expanded workflow status deltas #212
expanded workflow status deltas #212
Conversation
08a1a8b
to
7e53b81
Compare
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.
Only had time to review the code today. Going home earlier due to our evening meeting. Couple comments. One about any
… I think we don't need the has_queues
array. It could be a bool
instead, and just be set to True
if any delta_queue
is not empty? Brain is in the javascript-mode, so I couldn't write Python to test it now, but can try it another day with ☕ feel free to ignore if that doesn't make sense 👍
7e53b81
to
1642d49
Compare
Tests won't pass until cylc/cylc-flow#4206 is in. |
I don't quite get the |
Using the Cylc UI GScan branch to test this PR now. The first thing I noticed was that the only workflows displayed using this branch are the running ones. Quickly debugging it, looks like the GScan filters isn't happy with the new states. I'll update that branch to work with this PR so that it displays the same as on Cylc UI's |
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.
@dwsutherland I had a workflow/run five-running/run19
which I simply rm -rf ~/cylc-run/five-running/run19
. Then I received two deltas:
{"id": "1", "type": "data", "payload": {"data": {"deltas": {"id": "kinow|five-running/run19", "shutdown": false, "added": {"workflow": {"id": "kinow|five-running/run19", "name": "five-running/run19", "status": "uninstalled", "owner": "kinow", "host": "", "port": 0, "__typename": "Workflow"}, "__typename": "Added"}, "updated": {"workflow": {"id": "kinow|five-running/run19", "__typename": "Workflow"}, "__typename": "Updated"}, "__typename": "Deltas"}}}}
{"id": "1", "type": "data", "payload": {"data": {"deltas": {"id": "kinow|five-running/run19", "shutdown": false, "added": {"workflow": {"id": "kinow|five-running/run19", "name": "five-running/run19", "status": "uninstalled", "owner": "kinow", "host": "", "port": 0, "__typename": "Workflow"}, "__typename": "Added"}, "updated": {"workflow": {"id": "kinow|five-running/run19", "__typename": "Workflow"}, "__typename": "Updated"}, "__typename": "Deltas"}}}}
They appear to have the exact same content. Any idea why it would result in two identical deltas for the same query (you can see it's the same query as it has the same UI subscription id 1
).
Also, I was expecting to see the workflow updated to have the state uninstalled
. But instead I have the workflow added with the state uninstalled
. Is that expected?
No point in showing |
So instead of the unistalled state we will get a pruned delta? I just need something yhat I can use to remove the workflow from gscan if it was deleted or renamed. |
No, use it as the prune indicator.. but just don't show it. |
Aaaahh! D'oh moment 🤣 will update that branch tomorrow handling the uninstalled state. Great spot David! 🤦♂️ |
It does make sense (in my head anyway) to say "the workflow is uninstalled" in the same way you'd say "the workflow is running/installed/paused".. It essentially is the pruned delta (what's the difference?), and it is the state of the workflow... (we can put it anyway) |
Once a workflow is uninstalled it doesn't exist any more so there is nothing for the UIS/UI to track. So there shouldn't be an entry in the UIS or UI data store for it any more. So I think we want to send a "pruned" message telling remote data stores that the workflow has been removed. The UI shouldn't have to interpret the updated deltas to find out whether or not it needs to purge its data store: if ((delta.updated && delta.status && delta.status == WorkflowState.UNINSTALLED) || delta.pruned) {
prune(delta.id)
} |
Update the Cylc UI PR to ignore the uninstalled state.
Having a delta would definitely be simpler. Also, since we have pruned tasks, jobs, etc... I think it would be OK to have a workflow being pruned too. But I can implement either way in the UI, so I will leave the decision up to you guys, @hjoliver , and others 👍 |
Ok.. I was originally trying to avoid touching the scheduler end with code it knows nothing about... Will add a boolean field to the pruned deltas ... However, I'm tempted to keep |
1642d49
to
afbb17e
Compare
Looks good! |
afbb17e
to
418e2f0
Compare
Code looks good. Run a few tests:
Here's the result of a test I performed using the following subscription: subscription {
deltas {
added {
workflow(stripNull:true) {
id
status
}
}
updated {
workflow(stripNull:true) {
id
status
}
}
pruned {
workflow
}
}
} Installing/playing/cleaning a workflow resulted in the following deltas: $ cylc install one --no-run-name
added(one, installed)
$ cylc play one
added(one, running) # should be updated(one, running)
updated(one) # empty delta not needed for this sub caused by task pool changes
updated(one, stopped)
$ cylc clean one
updated(one) # should not be an updated delta here
pruned(one) Here's the returned JSON in full: $ cylc install one --no-run-name
{"id": "1", "type": "data", "payload": {"data": {"deltas": {"added": {"workflow": {"id": "oliver|one", "status": "installed"}}, "updated": {}, "pruned": {}}}}}
$ cylc play one
{"id": "1", "type": "data", "payload": {"data": {"deltas": {"added": {"workflow": {"id": "oliver|one", "status": "running"}}, "updated": {}, "pruned": {}}}}}
{"id": "1", "type": "data", "payload": {"data": {"deltas": {"added": {}, "updated": {"workflow": {"id": "oliver|one"}}, "pruned": {}}}}}
{"id": "1", "type": "data", "payload": {"data": {"deltas": {"added": {}, "updated": {}, "pruned": {}}}}}
{"id": "1", "type": "data", "payload": {"data": {"deltas": {"added": {}, "updated": {"workflow": {"id": "oliver|one", "status": "stopped"}}, "pruned": {}}}}}
$ cylc clean one
{"id": "1", "type": "data", "payload": {"data": {"deltas": {"added": {}, "updated": {"workflow": {"id": "oliver|one"}}, "pruned": {"workflow": "oliver|one"}}}}} |
Tested with same result as @oliver-sanders I applied the subscription above in GraphiQL, then did an install, play, pause, play, stop, clean while watching the network response in Firefox Dev Tools (because you can't tell in GraphiQL if the same response comes back multiple times).
@dwsutherland - can the server strip / not send these empty (ID only) deltas? |
Just wondering if |
Worth a shot sticking it a level up on the deltas bit: subscription {
deltas(stripNull:true) {
added {
workflow(stripNull:true) {
id
status
}
}
updated {
workflow(stripNull:true) {
id
status
}
}
pruned {
workflow
}
}
} $ cylc install one --no-run-name
added(installed)
$ cylc play one
added(running) # should be updated(running)
updated() # empty updated delta
null # empty delta
updated(stopped)
$ cylc clean one
updated() # empty updated delta
pruned() $ cylc install one --no-run-name
{"id": "1", "type": "data", "payload": {"data": {"deltas": {"added": {"workflow": {"id": "oliver|one", "status": "installed"}}, "updated": {}, "pruned": {}}}}}
$ cylc play one
{"id": "1", "type": "data", "payload": {"data": {"deltas": {"added": {"workflow": {"id": "oliver|one", "status": "running"}}, "updated": {}, "pruned": {}}}}}
{"id": "1", "type": "data", "payload": {"data": {"deltas": {"added": {}, "updated": {"workflow": {"id": "oliver|one"}}, "pruned": {}}}}}
{"id": "1", "type": "data", "payload": {"data": {"deltas": {"added": {}, "updated": {}, "pruned": {}}}}}
{"id": "1", "type": "data", "payload": {"data": {"deltas": {"added": {}, "updated": {"workflow": {"id": "oliver|one", "status": "stopped"}}, "pruned": {}}}}}
$ cylc clean one
{"id": "1", "type": "data", "payload": {"data": {"deltas": {"added": {}, "updated": {"workflow": {"id": "oliver|one"}}, "pruned": {"workflow": "oliver|one"}}}}} |
It's inherited so you can declare at the top:
However, the reason added, updated, and pruned aren't stripped (yet) is because they aren't protobuf fields (this data structure is a mix of dictionary and protobuf)..
(as mentioned on riot) It should be the topic of a separate PR, this PR is only providing the It may take some work to figure out how to make some graphql fields show up only when other fields are not empty. Additionally, I don't think I can have strip null as an argument to the parent/root |
Presumably we do push after stripping? (There's no point in pushing before stripping!) ... I must be misunderstanding you? |
@dwsutherland so you've explained the empty (ID only) deltas. But I don't think you commented on this?:
|
Started reviewing this one today with the sibling cylc-flow, and the UI gscan deltas branches, but it will take a few more days to complete the review & tests :+ |
Tricky one, because to the scheduler it is The only So I'd suggest leaving it as is and have the UI treat this transition the same as a |
Yes, what I meant was: If there's nothing to push after stripping, can we back out of yielding a result to the web socket? |
I wasn't aware of the added delta on reload (and I doubt the UI is either). We should be able to turn that added into an updated before broadcast somehow? Really don't want to have to make the UI interpret the information it's being provided in order to sync it's internal data store (the UI doesn't know much about Cylc). Going to bump to a new issue to avoid convoluting this one - #221 |
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.
Tested the pruned delta, works fine for me, bumped the other stuff into an issue for debate to allow us to focus on one thing at a time.
Is this in the async generator |
@dwsutherland - a few tests failing here. |
Yes, it will fail until the |
Oh, of course - sorry 🤕 |
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.
No problems here on re-testing. 👍 Let's get this in ...
These changes partially address cylc/cylc-ui#543
Sibling to cylc/cylc-flow#4206
(that one should go in first)
This PR:
installed
/uninstalled
as seen at the UI Server in the form of deltas.Needed for the UI to know when it can remove old/uninstalled workflows.
From Element (Oliver) (note:
held => paused
):To test this I ran the following:
Requirements check-list
CONTRIBUTING.md
and added my name as a Code Contributor.