Event handling for active alarms #121
Conversation
…the responses to those queries.
__init__.py
Outdated
| def handle_active_alarm_query(self, _): | ||
| """Event handler for the skill.alarm.query-active command.""" | ||
| event_data = {"active_alarms": bool(self.settings["alarm"])} | ||
| event = Message("skill.alarm.active-queried", data=event_data) |
There was a problem hiding this comment.
should this be a message.response rather than a new Message?
There was a problem hiding this comment.
What is the definition of "active alarm"? Any alarm at all (like for next week, or next year)? Or just in the next 24 hrs? Or just until midnight tonight? I don't care what the answer is... just that it's well-defined and documented.
There was a problem hiding this comment.
@krisgesling I am purposefully trying to avoid the request/response mechanism. in my reading on event driven architecture, the proper way to respond to a command or request is to emit an event indicating the task was completed.
Are we an event driven architecture or a request/response architecture? I don't think we should be both.
There was a problem hiding this comment.
@MetaGamer The current definition is whether or not any alarms exists that have not expired. I will add more information in the docstring.
There was a problem hiding this comment.
FWIW, the HiveMind spec, which is a kind of superset of the current Mycroft stack, splits the difference by emitting a response containing ACK INSTRUCTION
(doesn't translate directly, because INSTRUCTION is an index, but it accomplishes cleanliness while preserving the two-way bus)
There was a problem hiding this comment.
just gonna leave this here https://github.com/JarbasHiveMind/HiveMind-core/wiki/Mycroft-Messages
the reply/forward methods are essential for hivemind support
There was a problem hiding this comment.
I understand and agree that wait_for_response() is not useful in this scenario (as far as I understand the use case) the using .response() may still be of use so that messages follows a standardized format and indicates it's relation in a clear way. if the event emitted is only sent as a response (and not unprovoked due to other actions)
I do think wait_for_response() has a niche to make the logic flow easier to follow when execution is blocked until an event is sent is a result of a triggering event.
|
Should a |
That PR will not be merged. This is a replacement to that PR. I've had some discussions with the internal team about the Skill API and I have some reservations about its use. |
does that mean MycroftAI/mycroft-core#1822 is being deprecated? |
Is there a roadmap or docs somewhere that discusses the future of this implementation or the desired alternative? Just trying to maintain compatibility with Mycroft where I can. |
|
Slow down folks. One of the reasons the logic in this PR is implemented without the Skill API is to present a potential alternative solution. No decision has been made. We will be working on multiple aspects of skill interaction in our next sprint. Once we've gathered our thoughts on the matter internally, we will be communicating them publicly. |
|
the Skill API was clearly promoted to me as a solution to act as a pointer to other skill methods at the time of writing the homescreen skill instead of polluting the message bus with events that could eventually lead to other skills not be able to use same messages for where they could, so why shouldn't the Skill API be used for things like this ? |
You said categorically that the other PR would not be merged so it looked more like an announcement than a discussion, thanks for the clarification the concern is that we are starting to add this in a bunch of skills and would hate to refactor again, so if a deprecation was planned we would look into alternative approaches like providing these decorators in an external package. A new major release is coming, if this is deprecated can we at least delay until the next major release so skill devs have time to adapt? |
The Skill API is not an alternative to "polluting the message bus". In fact, it emts message bus messages itself. It is, at its core, syntactic sugar around using message bus messages the same way I did in this PR to communicate data between skills. |
It was an announcement that a PR would not be merged, not that the Skill API is going to be deprecated. Two very different things. Please do not conflate them into one. There are other reasons, besides Skill API usage, for this decision.
Everyone needs to understand that Mycroft Core is not an architecturally stable platform yet. We release breaking changes every six months. Some breaking changes will deprecate existing methodologies. Some decisions that seemed like good ideas at one point in the project's life cycle will be reversed. If we are doing our jobs right, the amount of changes to existing architecture will decrease over time. We just aren't there yet. The current plan is to complete the aforementioned skill interaction sprint before 21.08 so that it can be included in that release. This is being done because we anticipate breaking changes to be introduced in this sprint. |
|
Just asking that you don't remove it straight away, leave a proper deprecation warning log during the next 6 months, and after that actually remove it in the next major release If a platform is not stable and simply removes core functionality out of nowhere developers will never feel comfortable developing for it and it will hurt adoption, with the deprecation warning and the 6 months time buffer devs can adapt to those breaking changes EDIT: the implicit assumption here is that a deprecation warning would also point devs to the alternative implementation or mention that it will be removed and not replaced. ie, is the decision that this should not be done at all or that it should be done a different way |
Thanks for the calrification, I'll look forward to that discussion of the Skill API when we get there. :) |
|
Hi all, I wanted to follow up and reiterate that nothing is being deprecated. If we were to deprecate something like message replies, we'd first want to talk with the community about why the change was be proposed, what the alternative is, and how that alternative solves some problem or improves upon the current implementation. Then we'd put adequate deprecation warnings and compatibility layers in, all of this long before actually removing it. The Skill interaction sprint that Chris mentioned is coming up. Leading into that piece of work Michael and Derick have been working on a discussion document outlining a proposal for the behaviour that we want to see from Mycroft Skills. We want to review that document with the community and make sure it reflects the behavioural direction we want to go. Once we have a good understanding of the behaviour we are aiming for, then together we can see what implementation would get us there. |
Description
Implements generic "are there active alarms?" request and response events. Initial use case is the new Home Screen skill knowing when there are active alarms so the alarm indicator icon can be shown or hidden.
Type of PR
If your PR fits more than one category, there is a high chance you should submit more than one PR. Please consider this carefully before opening the PR.
Testing
skill.alarm.query-activecommand over the bus. Listen for theskill.alarm.active-queriedevent in response to the command.skill.alarm.scheduledevent.Documentation
Docstrings updated.