MycroftAI / mycroft-core

Mycroft Core, the Mycroft Artificial Intelligence platform.
https://mycroft.ai
Apache License 2.0
6.48k stars 1.27k forks source link

Make Signals event-based #3111

Open NeonDaniel opened 2 years ago

NeonDaniel commented 2 years ago

Is your feature request related to a problem? Please describe. The current implementation of signal files relies on querying the filesystem for the presence of a particular file. This means looped checks are used which eat up processing cycles and don't account for files that are deleted and re-created (i.e. isSpeaking). It also means that signal checks must originate from modules with a shared file system which is challenging for containers.

Describe the solution you'd like I implemented a SignalManager in Neon that uses bus events to set/clear/wait for signals. By default, signal files are still written to disk for backwards-compat, but methods to check or wait for a signal use Events instead of repeated FS checks.

Describe alternatives you've considered Most signal use could be removed entirely, but isSpeaking in particular would be difficult to refactor. Also, skills that use signals should remain supported.

Additional context The primary motivation for this was running modules in containers, as implemented in Neon.

krisgesling commented 2 years ago

Yeah totally agree, we want everything to be a common communication type with a clear structure.

To borrow a trope - you can communicate with other components however you like... as long as it's a bus message :stuck_out_tongue:

forslund commented 2 years ago

At some point I know we deprecated the whole signal system and it was slated to be removed completely.

However something like the signal manager would be cool to track state, back then I imagined something similar to the threading.Event but backed by the messagebus to make it cross-module.. The tricky part was to have a sync at startup to make sure a new process knew the current state if it got restarted or started late or whatever. I even wrote a proposal for it but it was not of interest to the team back then. This signal manger seems to be something similar right? But more?

forslund commented 2 years ago

Signal manager be something that lives in the messagebus-client module? Since one of the things we do there is setup standard ways of communicating over the messagebus with helper primitives?

NeonDaniel commented 2 years ago

Signal manager be something that lives in the messagebus-client module? Since one of the things we do there is setup standard ways of communicating over the messagebus with helper primitives?

I put the signal manager with the messagebus service; it seemed like the best place to force one instance per "core" since (1) the service will fail to start if there's already another bus active and (2) I think of the messagebus as defining a discrete "core".

The tricky part was to have a sync at startup to make sure a new process knew the current state if it got restarted or started late or whatever.

I just use Messages to set and query signals, so the processes are stateless and the SignalManager object is the source of truth.

To borrow a trope - you can communicate with other components however you like... as long as it's a bus message :stuck_out_tongue:

This is my mantra whenever I look at how to communicate with a module. My inspiration here was that I refactored all the signals Neon added into bus messages and figured that's just a better implementation for signals in general

forslund commented 2 years ago

Gotcha, sounds simpler than what I proposed :)

But maybe the signal class may be a good fit over there if the SignalManager is a feature of the messagebus service interaction with it should be in the client-module? or does that put it too far away from it's parent?

NeonDaniel commented 2 years ago

Gotcha, sounds simpler than what I proposed :)

But maybe the signal class may be a good fit over there if the SignalManager is a feature of the messagebus service interaction with it should be in the client-module? or does that put it too far away from it's parent?

Hmm, I hadn't thought about using the Signal class outside of the manager; I was focused on backwards-compat with the create_signal and check_for_signal methods. I don't think there's ever a need to interact with the Signals directly unless there's a use case for checking for the last creation time? Even that could be handled via bus events though.

The only reason I created the Signal class was to have a created event and a `cleared event, so either case of waiting (create or clear) could use an event trigger instead of polling