-
Notifications
You must be signed in to change notification settings - Fork 93
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
Event driven data-store deltas #3938
Conversation
1536fa8
to
f0bdaa5
Compare
eb372b6
to
254193b
Compare
Ok, ready for review.. However, there are a couple things that I might change: One
Reason(s) not to:
Thoughts? Two |
254193b
to
5e836bf
Compare
5e836bf
to
f21ed78
Compare
I've been playing with this branch using However, I'm seeing this error from the UI Server with Cylc UI:
|
Seems to be a genuine error. It still happens after recreating my environments. It occurs after starting the first flow on the back end. |
I'll have a look. But, on the surface, it looks like the UI Server hasn't picked up the protobuf generated module changes.. But you mentioned you recreated your virtual envs.. So, not sure yet |
I did |
Also checked the correct |
No but Bruno approved it already (on holiday!) and it's a simple change, so I'll merge it now. (That's a PR from me, today, BTW). |
(cylc-ui change merged) |
Update on this problem for others: double check that you've done |
(branch conflicted now) |
f21ed78
to
8f9942e
Compare
8f9942e
to
92fd21d
Compare
My feeling is leave it as-is for now. This works, and we could consider moving them into the task proxy object later. However, it may not be worth bothering because we want to move to a proper event-loop driven system before too long: objects raise events (with associated data) and a central event loop processes all events, dispatching them to systems (like the datastore) that have registered interest in them. (Something like that). |
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.
LGTM 👍, no problems found, so approving this before I bail out for the xmas break. Thanks @dwsutherland 💐
aebe2e7
to
0f5d7ab
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, runs fine, happy to have Tui showing succeeded tasks again 👏.
tp_id, PbTaskProxy(id=tp_id)) | ||
tp_delta.stamp = f'{tp_id}@{update_time}' | ||
ext_trigger = tp_delta.external_triggers[trig] | ||
ext_trigger.message = message |
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.
As with outputs the message
field is updated unnecessarily.
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.
This one's a bit different, as I don't set the message
in on task/node creation.. Reason being; I think it's user defined on the fly (via command).
itask.state.reset(TASK_STATUS_WAITING) | ||
if itask.state.reset(TASK_STATUS_WAITING): | ||
self.data_store_mgr.delta_task_state(itask) |
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.
There are a lot of these delta_task_state
so a high risk of missing one, would it make sense to put the delta_task_state
bit inside TaskProxy.reset
to ensure consistency?
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 wrestled with this too..
On the one hand it would be simpler to pass the data_store_mgr object into ever TaskProxy
, and then the proxies' Outputs
, TaskState
, and then to Prerequisites
object and so on... You can see it being passed around a lot, like public bicycle but worse..
But on the other hand, the data_store_mgr doesn't need to know about all the task proxies (i.e. runahead and other not ready), and/or all their events...
At least this way we can be intentional about it... but I'm open..
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.
Ok, might open a follow-on issue as this isn't entirely a data-store issue. Recently hit a bug where the DB wasn't being updated for a particular state change because the full procedure wasn't being followed, so I'm a bit worried about potential bugs now we have the task proxy, data store, outputs and data base to keep in sync.
(Long-term I would really like task events to be pushed to a centralised "event queue" with multiple consumers able to subscribe to the event stream.)
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 now I guess we could write an callback method in the task_pool and pass that to each task_proxy to avoid having to pass a handful of managers down to every object?
TaskPool already has access to suite_db_mgr
, task_events_mgr
& data_store_mgr
.
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.
Bumped - #4037
Prerequisites only seem to be listed for active tasks, presumably a SoD consequence, will need to find a way to generate the output for |
Does this close #3921? The information was already there, but in a different form (updated on event) .. So let's say yes. |
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.
UI working fine with the changes, great work!
@dwsutherland, I noticed while playing with the new code in GraphiQL that I was getting empty deltas, and empty task proxies too. Should them have been stripped/removed from the response instead? Easy to filter in the UI, but looks unnecessary to send {}
. And you can see that the number of {}
's corresponds to the number of task proxies (that's five
, the same bar
/foo
task proxies it's always displaying)
No other issues found 👍 I haven't tested it with cylc/cylc-ui#530, as it's addressing that issue partially. @oliver-sanders did a great review. I had a cursory look at the code, and found no other issues +1 pending only @dwsutherland to address @oliver-sanders ' review.
Correct, although I'd point out that the old system only had prerequisites for waiting tasks that happened to have corresponding task proxies spawned ahead already. |
1c8a612
to
19cee8f
Compare
19cee8f
to
8b18fe7
Compare
Chucked the prerequisites thing into another issue - #4036 |
These changes close #3746, closes #3921, closes #4030
Partially satisfies cylc/cylc-ui#530
At present the data-store deltas are only partially event driven, including:
A few points on current state and intentions:
job_pool.py
) is separate from the data-store manager because it is event driven, but after converting the data-store to an event driven approach, this is no longer necessary.Some updates and these deltas should still be batched for application and publishing (for now) to ensure relational consistency, summary updates are less frequent, and UI/UIS is not overwhelmed. i.e. if you have a family of 100 tasks, and each individual state change constitutes a recalculation of the family state and a publishing of these, then you could end up with hundreds if not thousands of group-state/summary and state deltas to calculate and publish in a few seconds (but batched this would only come to a handful if that).
To do:
job_pool.py
) into the data-store manager.Most the new delta event triggers will be from within the
task_pool.py
andtask_events_mgr.py
Possible future work:
Requirements check-list
CONTRIBUTING.md
and added my name as a Code Contributor.