-
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
UIS Delta subscriptions & GraphQL null stripping back-end #118
Conversation
b2de15c
to
b250310
Compare
b250310
to
507f4d8
Compare
Ok fixed the GraphiQL problem... It was an issue with the middleware stripping nulls during Introspection. |
507f4d8
to
9b0ec9e
Compare
9b0ec9e
to
32a4d57
Compare
@dwsutherland any reason for having a separate section for what was removed, but not for what was added and updated? From what I understand, it looks like added and updated are both under ps: workflow |
@kinow - Trying to figure out what part of the code you're referring too 😕 (sorry) |
Sorry, I am talking about the GraphQL output. The query response has Sorry for not being very clear. |
In gif above (in the Issue description) there's a "pruned" section that singles out what's been removed in the latest data push. @kinow is wondering (I think, based on a conversation we had on Friday) if it is also possible to single out what's been added? (then I guess the main body of the data structure would contain just what changed - but has not been added or removed .., .rather than what's now present, which includes what's new and what's changed). |
(Doh, I was typing at the same time as @kinow 😬 ) |
But your explanation was much better than mine. I'm working exactly on this, but still doing JS now, not querying the endpoint yet. So good timing for you to have a look at this @dwsutherland 😬 |
Ah, I see.. I haven't really made that distinction at the workflow level either, but intend to do so (for the sake of our event driven future).. I guess if something's been added and then modified (if that's even possible in one push), they could be in both |
Ah, I guess it would still work. I would just go and:
We can do that later. I'll start using this code for the JS code and send a functional reviewer later (not looking much at the code changes, but more how it works, whether it does what it is supposed to do, etc) 👍 Thanks! |
Maybe do the pruning last, although I would hope/think it wouldn't matter as something shouldn't be pruned and updated in the same push. |
@dwsutherland, the original query used by the tree view (current & infinite) has something like: query {
workflows (ids: ["five"]) {
...
taskProxies (sort: ...) {
...
firstParent { }
task { }
jobs (sort: ...) { }
}
}
} Using GraphiQL, I created the following subscription for deltas. subscription {
deltas (id: "five") {
pruned { familyProxies taskProxies jobs }
workflow {
id
status
taskProxies {
id
name
state
...
firstParent {}
... # same structure as in the query
}
familyProxies { }
}
}
} The subscription is submitted and I am getting the results, but the Q1: Is that intentional? The JS code fails when I add a delta Q2: just to confirm, the sorting ( Q3: in the query, I can send Thanks! |
I need to fix how this field is resolved.. I was in two minds to where the resolver would need to look, but this use case makes it clear that sub-field resolution should use the local data-store (not the other delta data) post delta application...
Should be able to reinstate this for subscriptions
Yes, use I'm working on some changes in the backend to distinguish between |
Thanks for the quick reply @dwsutherland ! It's been super easy to work with the deltas so far (easier than I expected). I've left most of the logic in-place in my branch, so once you have the updates for the code I will check it out and test it while finishing the JS code. |
@kinow - Also, I didn't implemented nested resolving because it's not consistently possible and I thought that we were aiming for flat.. i.e.:
What if It's technically possible, but would require an argument of each level of nest to specify whether to resolve from the delta info or the data-store. |
In the case of And now we are splitting things to |
Hmmm, tricky. I can handle the flat structure on the client side. I will have to think a little. We don't have a flat structure in the initial query. It is not easy to fetch tasks or jobs in the original workflow data. These are under So I think the options I have now are iterate in JS (which could have performance issues) or try to use a flat structure for the tree component. So instead of using 🤔 |
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.
Did a functional review, testing if the deltas in this branch worked with the UI. +1, after a few iterations with @dwsutherland , we got it all sorted. Thanks a lot for the help and patience David! Will leave the code review for @hjoliver & @oliver-sanders.
aa20fee
to
0bed373
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.
Looks good, I've not done heavy testing but I believe @kinow has.
One question.
if field.name == WORKFLOW: | ||
self.data[w_id][field.name].Clear() | ||
else: | ||
self.data[w_id][field.name].clear() |
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.
What's going on here, is there a difference between Clear
and clear
?
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.
One's a Protobuf element (i.e. PbWorkflow
), so uses Clear
, the other(s) a dictionary.
From RIOT Re-synced branches this morning after approving it some time ago. There were now Adding Looks like the Having both |
@dwsutherland another comment/question, not related to the issue above. I decided to have a quick look at the Graph component this morning, checking if the delta subscription would work. I don't know much about the Graph component & JS cytoscape, but guessed it was still worth to use the momentum to try and learn and review this PR at the same time. With this query collapsed deltas query with edgessubscription ($workflowId: ID) {
deltas (workflows: [$workflowId], stripNull: true) {
added {
edges {
id
source
target
suicide
cond
}
}
updated {
edges {
id
source
target
suicide
cond
}
}
pruned {
edges
}
}
} I get the initial burst of data, and eventually added and pruned data (I guess for However, every now and then I also get this empty response. {
"deltas": {
"added": {},
"updated": {},
"pruned": {}
}
} Is that correct? I was expecting to get data iff there was some data in the server to be sent to the client. |
Fixed (in |
I get the initial burst:
And then added and pruned thereafter... I could probably strip |
@dwsutherland synced this branch to pick up latest fix for the That appears to have fixed the initial data burst. But the JS code failed to render the tree again. A bit of debugging the queries again, it looks like another interesting issue. Here's what I am getting in Now @dwsutherland it looks like when |
This works:
It's because the jobs |
Tested, and indeed works! Thanks @dwsutherland ! |
Ok original should work now:
Ignored stripping of sub-field if it's return type is in |
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.
A few minor issues after enabling strip null in the UI. Working again (thanks to @dwsutherland !)
Aaahhh, that's what the |
0bed373
to
411da1a
Compare
It looks up the relationship from the data-store without the arg now, but the arg will still determine whether to try retrieve the node from the delta-store or data-store. |
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.
Synced today, and tested with the UI. Everything worked fine, re-approved! 🎉
@dwsutherland I have both branches synced (did that after our Riot meeting some days ago). And have also finished the code and tests in the WUI. So today and tomorrow are days that I am planning to spend a lot of time looking at the screen while it runs During development I spent a looooong time with After some adding some try/catch with a And it's strange that apparently I am receiving a Job from the GraphQL endpoint before the cyclepoint and tasks? See screenshot below. The cycle point for the year 2772 is missing. The JS code does some business logic when adding nodes to the tree, like cycle points, tasks, families, etc. However, what you are seeing in the browser console is the output of the For cycle points, all that I do when adding one is check Here's the GraphQL deltas that I received when the exception occurred. GraphQL deltas response...{
"id": "kinow|families2",
"shutdown": false,
"added": {
"jobs": [
{
"id": "kinow|families2|27720611T1203+12|qux|1",
"firstParent": {
"id": "kinow|families2|27720611T1203+12|qux",
"__typename": "TaskProxy"
},
"batchSysName": "background",
"host": "localhost",
"state": "ready",
"submitNum": 1,
"__typename": "Job"
}
],
"__typename": "Added"
},
"updated": {
"taskProxies": [
{
"id": "kinow|families2|27700611T1203+12|qux",
"state": "succeeded",
"isHeld": false,
"latestMessage": "succeeded",
"firstParent": {
"id": "kinow|families2|27700611T1203+12|FAM6",
"name": "FAM6",
"cyclePoint": "27700611T1203+12",
"state": "succeeded",
"__typename": "FamilyProxy"
},
"task": {
"meanElapsedTime": 20,
"name": "qux",
"__typename": "Task"
},
"__typename": "TaskProxy"
},
{
"id": "kinow|families2|27720611T1203+12|qux",
"state": "ready",
"isHeld": false,
"latestMessage": "",
"firstParent": {
"id": "kinow|families2|27720611T1203+12|FAM6",
"name": "FAM6",
"cyclePoint": "27720611T1203+12",
"state": "ready",
"__typename": "FamilyProxy"
},
"task": {
"meanElapsedTime": 20,
"name": "qux",
"__typename": "Task"
},
"__typename": "TaskProxy"
}
],
"jobs": [
{
"id": "kinow|families2|27700611T1203+12|qux|1",
"firstParent": {
"id": "kinow|families2|27700611T1203+12|qux",
"__typename": "TaskProxy"
},
"finishedTime": "2020-06-11T15:31:36+12:00",
"state": "succeeded",
"__typename": "Job"
}
],
"familyProxies": [
{
"id": "kinow|families2|27720611T1203+12|FAM5",
"state": "ready",
"firstParent": {
"id": "kinow|families2|27720611T1203+12|root",
"name": "root",
"cyclePoint": "27720611T1203+12",
"state": "ready",
"__typename": "FamilyProxy"
},
"__typename": "FamilyProxy"
},
{
"id": "kinow|families2|27720611T1203+12|FAM6",
"state": "ready",
"firstParent": {
"id": "kinow|families2|27720611T1203+12|FAM5",
"name": "FAM5",
"cyclePoint": "27720611T1203+12",
"state": "ready",
"__typename": "FamilyProxy"
},
"__typename": "FamilyProxy"
},
{
"id": "kinow|families2|27700611T1203+12|FAM6",
"state": "succeeded",
"firstParent": {
"id": "kinow|families2|27700611T1203+12|FAM5",
"name": "FAM5",
"cyclePoint": "27700611T1203+12",
"state": "succeeded",
"__typename": "FamilyProxy"
},
"__typename": "FamilyProxy"
},
{
"id": "kinow|families2|27700611T1203+12|FAM5",
"state": "succeeded",
"firstParent": {
"id": "kinow|families2|27700611T1203+12|root",
"name": "root",
"cyclePoint": "27700611T1203+12",
"state": "succeeded",
"__typename": "FamilyProxy"
},
"__typename": "FamilyProxy"
}
],
"__typename": "Updated"
},
"pruned": {
"__typename": "Pruned"
},
"__typename": "Deltas"
} Pruned is empty, so we ignore that. Added has one entry, the job. Now the interesting part. The updated deltas include the families and task proxies. I will start writing some debugging code to capture 100 GraphQL messages, and store so that next time this exception occurs I can take a look at previous messages and confirm I didn't get a Does it make sense? Do you think this scenario is possible by some rare chance? Thanks! p.s.1: deltas.added.cyclePoint is for
p.s.2: once families2 is passing with no runtime issues/errors, I still need to test the deltas in the WUI with complex 😨 |
Funny, couldn't reproduce it at home, but I noticed my Let me try to reproduce it again next Monday (@dwsutherland don't bother looking into this, as I know you are busy with migration, and it could be something in my environment/branch). Notes to self
Executed the complex workflow today too, and after 10 minutes got no errors in the browser console (only warnings like "[Violation] 'message' handler took 197ms'", which indicates there are ApolloClient functions taking too long to execute; I believe these are functions Also executed five and families2, but found no error. When I started the tests, I had cylc-uiserver with latest commit I had also prepared a diff that might be helpful when inspecting deltas: diff --git a/src/components/cylc/tree/deltas.js b/src/components/cylc/tree/deltas.js
index 1176836..f8d90b8 100644
--- a/src/components/cylc/tree/deltas.js
+++ b/src/components/cylc/tree/deltas.js
@@ -195,6 +195,8 @@ function handleDeltas (deltas, tree) {
}
}
+const TEMP_DELTAS = []
+
/**
* @param {null|{
* id: string,
@@ -228,6 +230,7 @@ export function applyDeltas (deltas, tree) {
tree.clear()
return
}
+ TEMP_DELTAS.push(deltas)
if (tree.isEmpty()) {
// When the tree is null, we have two possible scenarios:
// 1. This means that we will receive our initial data burst in deltas.added.workflow
@@ -246,6 +249,12 @@ export function applyDeltas (deltas, tree) {
} catch (error) {
// eslint-disable-next-line no-console
console.error('Error applying initial data burst for deltas', error)
+ // eslint-disable-next-line no-console
+ console.log('Printing latest 10 deltas...')
+ TEMP_DELTAS
+ .slice(-10)
+ // eslint-disable-next-line no-console
+ .map(delta => console.log(delta))
throw error
}
} else {
@@ -258,6 +267,12 @@ export function applyDeltas (deltas, tree) {
} catch (error) {
// eslint-disable-next-line no-console
console.error('Error applying deltas', error)
+ // eslint-disable-next-line no-console
+ console.log('Printing latest 10 deltas...')
+ TEMP_DELTAS
+ .slice(-10)
+ // eslint-disable-next-line no-console
+ .map(delta => console.log(delta))
throw error
}
} That simply prints the latest 10 deltas (I think, haven't tested the code yet). On Monday I will use this code again from my NIWA laptop, and will report back what were the latest deltas, what was the difference in my |
My branches at work are up to date too. The workflow
However, I haven't been able to reproduce this error today after running the workflow in two browsers for ~30 minutes. |
Spoke too soon, just had that again! Let me apply this patch, and try to capture a sequence of deltas to see if that proves helpful. |
Error occurred after another 5 minutes running the workflow The 10 deltas in order to the last one, which caused the error: https://gist.github.com/kinow/374dee8fd5f08620c193e43fe3ab5938 Investigating now, as it could be something in the JS code 👍 |
I repeated this test, now printing also everything that was pruned. I had a theory that maybe the parent wasn't there because I pruned it. For the job without parent, for its cycle point, nothing was pruned. Meaning it wasn't added, and instead the WUI received the data only in the |
Query used: https://gist.github.com/kinow/a1f99b40d0deb3766e13b0584a84773c Variables has only the |
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 think it might be something to do with job deltas being event driven and task pool updates collected at the end of a main-loop iteration(?) hmmm... not sure.. How does a job start and/or message back without the taskProxies being created in the data-store.. doesn't seem possible .. Probably a follow on PR would be fine though |
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.
Depends on: cylc/cylc-flow#3500
(They should be merge together, as there is one change that is breaking to the
BaseResolvers
)Doesn't break Web UI.
Features:
all
) and applied to the local data-store (instead of the same multi-topic ones).Requirements check-list
CONTRIBUTING.md
and added my name as a Code Contributor.