-
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
Create delta callbacks, control execution order of added, updated, pruned #749
Conversation
960d8cf
to
70296b4
Compare
I finished the core changes, pending to fix the tests. But before that I will think about it over the weekend, and see if there are other ways to solve the #747 bug. We have 2 callbacks for the Tree view. The first callback populates a workflow lookup, used by Tree view (and Graph view, and Table view, in the future). The second callback creates the workflow tree structure, where the nodes of the tree contain pointers to the workflow lookup created by the first callback. The #747 bug happens when we have processed the first callback, applying some pruned deltas that removed nodes from the global lookup. And then the second callback has the same nodes in the updated delta, but now it fails to locate the nodes that have been just removed. This pull request splits the execution of deltas, creating a common interface for each delta callback, where we have, in order:
For each delta, then, we have in pseudo-code:
And when the component is destroyed, The second for is fixing this bug, by applying pruned deltas in reverse order⦠we add & update everything, everywhere. Then we can safely remove the pruned data. This way we always apply every delta, with no risk of trying to access data that was already removed. |
Codecov Report
@@ Coverage Diff @@
## master #749 +/- ##
==========================================
+ Coverage 90.98% 91.01% +0.03%
==========================================
Files 84 87 +3
Lines 1675 1703 +28
Branches 105 105
==========================================
+ Hits 1524 1550 +26
- Misses 121 123 +2
Partials 30 30
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Compilation errors fixed, now just need to fix these 32 unit tests that are broken... errr.. β π΅ |
Only 32! try messing with the core of cylc-flow ... π |
} | ||
this.mergeQueries(baseSubscriber.query.query, subscriber.query.query) | ||
subscription.callbacks = union(subscription.callbacks, subscriber.query.callbacks) | ||
} |
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.
for-loops are easier to debug π¬
[] | ||
[ | ||
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.
I think this is clearer. Before it was using the same Vuex action that is used by GScan. But now it's even clearer that for now the Dashboard and WorkflowsTable are re-using the query & callbacks of GScan.
src/services/workflow.service.js
Outdated
throw new Error('Error recomputing subscription: Query variables do not match.') | ||
} | ||
this.mergeQueries(baseSubscriber.query.query, subscriber.query.query) | ||
subscription.callbacks = union(subscription.callbacks, subscriber.query.callbacks) |
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.
Note to self: manual test, and confirm this is working OK.
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.
Just pending some manual testing, review if I didn't miss any parts of the code to be tested, and then it should be ready for review πͺ but will probably leave the manual test for tomorrow morning. |
8788df1
to
c1022fa
Compare
Tested locally with some unit tests, found one bug, fixed. Tested locally again with Ready for review! π |
72d38a1
to
b76ac13
Compare
Rebased fixing conflicts. |
98460b6
to
e5c3f95
Compare
(Testing this now, next major review priority) |
Hmm, opening a second tree view on this branch causes the tree to freeze π€ |
(although that doesn't seem to be the case in your screen recording above!) |
I've been keeping this PR up to date with master, maybe something changed and caused the freeze. Will try to reproduce in a bit (or in the morning, on a Windows computer trying to get a nvidia computer board to boot π€ ) |
But if you are investigating it, what I would look at are
Then I'd start looking at vue dev utils to see what's the state of the Tree view (literally, look for the Tree component/view, which must contain a TreeItem which is the top of the tree). And also look at the Network tab to confirm the subscription is going OK. Then from there it'd be the IDE to see what could be wrong with the code. |
Working OK for me @hjoliver
(the quick blank state is due to the subscription being reloaded; I'll check why that's happening) |
Looks good. Does it carry on to the next cycle points though, with multiple tabs open? |
I think so. Two trees, CP 180. Some seconds later, CP 182. I found why it's restarting the subscription, but that's a bug in master too. Will create an issue for that one and start preparing a fix and an e2e test to prevent that regression π |
If you would like I can try with the same workflow you are using and see if it works? |
Huh, strange (but good). I've tried with several workflows. Let me just nuke everything and start from scratch again... |
In case it helps, heres [scheduling]
cycling mode = integer
initial cycle point = 1
[[queues]]
[[[default]]]
limit = 1
[[graph]]
R1 = "prep => foo"
P1 = "foo[-P1] => foo => bar"
[runtime]
[[root]]
script="sleep 5"
[[prep]]
[[foo]]
[[bar]]
|
@hjoliver shared his screen showing what this PR looks like.
Now the tree views are both frozen, not being updated. The network tab shows 2 GraphQL subscriptions, We tested on Chrome and Firefox on Linux. |
(Result: the bug was only triggered in production builds, something to do with lodash's |
I can confirm the latest commit fixes it π |
@hjoliver sent me a workflow that shows errors due to missing parents. It happens when a task cannot find the parent family proxy, and also when a job cannot find the parent task. I've created a gist with the deltas collected, the One parent task only appears as updated, skipping the added status. But I'm still investigating it that's the cause of the errors with that workflow. |
added and updated deltas for each callback, commit. Then the pruned in reverse order, commit.
317c280
to
c71ed5d
Compare
(rebased) |
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'm still investigating
Not sure what the status of this is.
The code looks good, I ran some local tests and everything worked fine.
Just pending review.
Thanks! @hjoliver or @dwsutherland can sign it off then (I think they were the first ones to report the issue) |
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.
π
These changes close #747
Used the workflow provided by @hjoliver to reproduce and fix the issue. See screenshot of the UI after fixing the issue, no browser console, no alerts πββοΈ
Requirements check-list
CONTRIBUTING.md
and added my name as a Code Contributor.