Improve readiness check for all services #2648

krisgesling · 2020-08-06T12:13:55Z

Description

Add a consistent interface to set and query the state of Mycroft Services improving the ability to check that Mycroft as a whole is ready for use. Full documentation at:
https://mycroft-ai.gitbook.io/docs/mycroft-technologies/mycroft-core#process-status-proposed

This adds a ProcessStatus object to key services providing a common interface for querying it's lifecycle status.
It also shifts the readiness check from the Padatious service to the Enclosure. Though is still triggered by the completion of the first Padatious training run.

Initial issue discussion at: #2639
Previous closed PR's - #2642, #2645

How to test

Run unit tests.

Alternatively break a service preventing it from loading and check that the mycroft.ready message is not emitted (also logs "Mycroft is all loaded and ready to roll!" to the enclosure.log).

Contributor license agreement signed?

CLA (Whether you have signed a CLA - Contributor Licensing Agreement

devops-mycroft · 2020-08-06T12:49:56Z

Voight Kampff Integration Test Succeeded (Results)

devops-mycroft · 2020-08-09T20:04:20Z

Voight Kampff Integration Test Succeeded (Results)

chrisveilleux

On the surface, this seems like an unnecessarily elaborate status logging mechanism. Are there follow-up PRs that leverage the ProcessStatus class more than it is here? If we are going to implement a status service in the future, is this really necessary now? Would this even be needed if we had a status service?

I also wonder if it would be better to react to status change messages on the bus rather than making periodic checks for a particular status.

chrisveilleux · 2020-08-13T22:37:25Z

mycroft/audio/__main__.py

@@ -45,6 +46,8 @@ def main(ready_hook=on_ready, error_hook=on_error, stopping_hook=on_stopping):
        check_for_signal("isSpeaking")
        bus = MessageBusClient()  # Connect to the Mycroft Messagebus
        Configuration.set_config_update_handlers(bus)
+        status = ProcessStatus('audio', bus, on_ready=ready_hook,


I am seeing this pattern of parameters passed a lot of places (ready_hook, error_hook, stopping_hook). Could be replaced by a dataclass/namedtuple/dictionary to simplify the parameterization.

I'm not sure that simplifies it, it just adds another layer.

Edit: And by that I mean that for that another object needs to parametrized and then passed to this object for initialization. Basically layering another object but still needing the same number of parameters somewhere. And since each project shall provide their own hook functions this still needs to be done at some point.

Been trying it out a bit. Grouping the callback methods into a named tuple sort of makes sense and the complexity increase is quite low. I'll add that and we'll see what you think.

chrisveilleux · 2020-08-13T22:43:57Z

mycroft/util/process_utils.py

+    def set_alive(self):
+        """Basic loading is done."""
+        self.state = ProcessState.ALIVE
+        if self.on_alive:
+            self.on_alive()
+
+    def set_ready(self):
+        """All loading is done."""
+        self.state = ProcessState.READY
+        if self.on_ready:
+            self.on_ready()


I would like some more clarity on what is "alive" vs. "ready". What is difference between "basic loading" and all loading. what is an example of why we care about alive vs. ready?

This will differ wildly between the different processes. In skills "basic loading" is defined as priority skills are loaded and the process has started it's main sequence but all skills aren't yet loaded.

In the audio service basic loading is when TTS has been loaded and mycroft can start talking, (servicing the priority skills for example), all loading is when the audioservices are loaded.

chrisveilleux · 2020-08-13T22:45:41Z

mycroft/util/process_utils.py

+        self.bus.on('mycroft.{}.is_alive'.format(self.name), self.check_alive)
+        self.bus.on('mycroft.{}.is_ready'.format(self.name),
+                    self.check_ready)
+        self.bus.on('mycroft.{}.all_loaded'.format(self.name),
+                    self.check_ready)


What is difference between is_ready and all_loaded? Aren't "ready" and "all loaded" the same thing? Also, I don't see "all_loaded" emitted anywhere.

.all_loaded is a backwards compatibility message type (conforming to the call VK currently makes), in case someone / thing is already using it.

I'll add a comment about that.

Forgot about this, but it's added now.

chrisveilleux · 2020-08-13T22:47:09Z

mycroft/util/process_utils.py

@@ -121,3 +122,131 @@ def echo(message):
            # Listen for messages and echo them for logging
            LOG(name).info("BUS: {}".format(message))
    return echo
+


Run this through black and pylint

The code has been pylinted and blacked (-s -l 79)

chrisveilleux · 2020-08-13T22:51:17Z

mycroft/skills/__main__.py

+def on_started():
+    LOG.info('Skills service is starting up.')
+
+
+def on_alive():
+    LOG.info('Skills service is alive.')
+
+
 def on_ready():
-    LOG.info('Skill service is ready.')
+    LOG.info('Skills service is ready.')


 def on_error(e='Unknown'):
-    LOG.info('Skill service failed to launch ({})'.format(repr(e)))
+    LOG.info('Skills service failed to launch ({})'.format(repr(e)))


 def on_stopping():
-    LOG.info('Skill service is shutting down...')
+    LOG.info('Skills service is shutting down...')


Couldn't these log messages be put into the ProcessStatus class and generalized so they can be written out for any service?

They are basically there to be defaults for the status hooks arguments of the process's main function.

Why have defaults? Status hooks should be optional. If there is nothing to do when a paticular status is triggered, pass None to the hook and don't do anything. This is more meaningful that passing stubs all the time, IMO.

The main reason was to have functions with the expected signature available to anyone overriding them so it's easy to see what's expected of the callbacks.

I have no strong opinion of this and can remove them if you'd prefer.

chrisveilleux · 2020-08-13T23:27:45Z

mycroft/util/process_utils.py

+
+    def _register_handlers(self):
+        """Register messagebus handlers for status queries."""
+        self.bus.on('mycroft.{}.is_alive'.format(self.name), self.check_alive)


I don't see this emitted anywhere.

I don't think this is used currently but it makes sure backwards compatibility is ensured with the current implementation of mycroft.skills.is_alive we can at some point deprecate this is we it doesn't get used

chrisveilleux · 2020-08-13T23:28:42Z

mycroft/util/process_utils.py

+        """Respond to all_loaded status request.
+
+        Arguments:
+            message: Optional message to respond to, if omitted no message


I don't see message passed anywhere.

This is currently used by VK and by the mycroft-gui

mycroft/util/process_utils.py

chrisveilleux · 2020-08-13T23:54:17Z

mycroft/skills/__main__.py



-def main(ready_hook=on_ready, error_hook=on_error, stopping_hook=on_stopping,
-         watchdog=None):
+def main(alive_hook=on_alive, started_hook=on_started, ready_hook=on_ready,


Why are the hooks passed as arguments like this? The hook functions exist in the same module, this seems extraneous.

The hooks are meant to be overridden by someone implementing a Mycroft wrapping (for example a systemd service). See more info about the hooks in the PR that originally implemented them: #2601

These are just default values.

mycroft/audio/__main__.py

devops-mycroft · 2020-08-14T06:57:28Z

Voight Kampff Integration Test Succeeded (Results)

forslund · 2020-08-14T07:33:02Z

One use case for the hooks that exists today is systemd integration. (the default functions here are intended to be overridden).

The reason for having defaults is basically to show them being used to developers doing this type of integration.

pep8speaks · 2020-08-17T14:13:10Z

Hello @krisgesling! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-01-28 16:29:33 UTC

devops-mycroft · 2020-08-17T18:41:10Z

Voight Kampff Integration Test Failed (Results)

devops-mycroft · 2020-08-17T19:51:57Z

Voight Kampff Integration Test Succeeded (Results)

devops-mycroft · 2020-09-09T06:58:34Z

Voight Kampff Integration Test Succeeded (Results)

devops-mycroft · 2020-09-24T06:14:16Z

Voight Kampff Integration Test Failed (Results)

devops-mycroft · 2020-09-25T05:29:04Z

Voight Kampff Integration Test Succeeded (Results)

devops-mycroft · 2020-09-25T07:22:47Z

Voight Kampff Integration Test Succeeded (Results)

devops-mycroft · 2020-09-25T15:14:55Z

Voight Kampff Integration Test Failed (Results)

devops-mycroft · 2020-09-27T08:34:19Z

Voight Kampff Integration Test Succeeded (Results)

forslund · 2020-09-28T05:29:47Z

Rebased the code and updated it for the new Padatious intent service. Tackled most of the concerns above.

krisgesling · 2020-11-11T11:15:30Z

Hey, Chris and I spent some time today looking at what this might look like being pulled out as a new service independent of the the services that it's monitoring.

Started a spec doc that would be great to get your thoughts on:
https://docs.google.com/document/d/1REXWaUt1HUrbyaunB09cfQ435krdNwhjVbMVVG3iNKs/edit?usp=sharing
This should be open for anyone to comment on 🤞

@j1nx - I'm sure you'd have some great input too

Changes addressed

krisgesling · 2021-01-27T22:25:04Z

Hey @forslund, are you able to rebase this one?

I think it's all good to merge!

forslund · 2021-01-28T11:06:47Z

Sure thing, will rebase tonight

ProcessStatus tracks the process status and allows callbacks on changes and status queries over the messagebus. StatusCallbackMap is used to setup the callbacks ProcessState is an enum tracking the different states.

Previously Mycroft reported mycroft.ready when Padatious had finished it's first training run. This check has been shifted to the enclosure service, and starts checking once the Padatious training is complete. Currently checks on readiness of audio, speech and skills services.

Change all strings referring to the service to use "Skills service"

forslund · 2021-01-28T16:29:47Z

Rebased

devops-mycroft · 2021-01-28T17:18:21Z

Voight Kampff Integration Test Succeeded (Results)

krisgesling requested a review from chrisveilleux August 6, 2020 12:13

devops-mycroft added the CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors) label Aug 6, 2020

forslund force-pushed the feature/ready-check branch from e7d1883 to 5261d01 Compare August 9, 2020 19:27

krisgesling added the Status: To be reviewed Concept accepted and PR has sufficient information for full review label Aug 11, 2020

chrisveilleux previously requested changes Aug 14, 2020

View reviewed changes

forslund force-pushed the feature/ready-check branch from 5261d01 to 3ebfeb7 Compare August 14, 2020 06:20

forslund force-pushed the feature/ready-check branch from 3ebfeb7 to 48f70aa Compare August 17, 2020 14:13

forslund force-pushed the feature/ready-check branch from 48f70aa to aa9b996 Compare August 17, 2020 18:40

forslund force-pushed the feature/ready-check branch from aa9b996 to 6c60d22 Compare August 17, 2020 19:14

forslund mentioned this pull request Aug 26, 2020

initialize status members before bus handlers #2681

Merged

forslund force-pushed the feature/ready-check branch from 6c60d22 to 1efdaf1 Compare September 9, 2020 06:20

krisgesling added the Type: Enhancement - roadmapped Implementation of a feature that is a priority on the roadmap. label Sep 24, 2020

forslund force-pushed the feature/ready-check branch from 1efdaf1 to 7328c62 Compare September 25, 2020 04:53

forslund force-pushed the feature/ready-check branch from ec940f0 to c98e8a0 Compare September 25, 2020 14:38

forslund force-pushed the feature/ready-check branch from c98e8a0 to 241adfd Compare September 27, 2020 07:58

krisgesling added Status: Accepted PR has been reviewed and accepted. There must be some reason why it isn't being merged. and removed Status: To be reviewed Concept accepted and PR has sufficient information for full review labels Jan 27, 2021

forslund and others added 9 commits January 28, 2021 17:21

Add ProcessStatus class

bea1f00

ProcessStatus tracks the process status and allows callbacks on changes and status queries over the messagebus. StatusCallbackMap is used to setup the callbacks ProcessState is an enum tracking the different states.

Change names for consistency

cc3bf5b

Add test cases for ProcessStatus

eeb44ed

add ProcessStatus to key services

1273425

Remove is_alive and all_loaded registration test

93747a1

Add started and alive hooks to skills process

94d5e34

Always use the plural "Skills" for the service

b595f6c

Change all strings referring to the service to use "Skills service"

Add Todo / comment regarding startup order

5df1524

forslund force-pushed the feature/ready-check branch from 241adfd to 5df1524 Compare January 28, 2021 16:29

krisgesling merged commit cd4dbb3 into MycroftAI:dev Jan 28, 2021

Improve readiness check for all services #2648

Improve readiness check for all services #2648

Uh oh!

Conversation

krisgesling commented Aug 6, 2020

Description

How to test

Contributor license agreement signed?

Uh oh!

devops-mycroft commented Aug 6, 2020

Uh oh!

devops-mycroft commented Aug 9, 2020

Uh oh!

chrisveilleux left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

forslund Aug 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

forslund Aug 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

forslund Sep 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

forslund Aug 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

devops-mycroft commented Aug 14, 2020

Uh oh!

forslund commented Aug 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pep8speaks commented Aug 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Comment last updated at 2021-01-28 16:29:33 UTC

Uh oh!

devops-mycroft commented Aug 17, 2020

Uh oh!

devops-mycroft commented Aug 17, 2020

Uh oh!

devops-mycroft commented Sep 9, 2020

forslund Aug 14, 2020 •

edited

Loading

forslund Aug 14, 2020 •

edited

Loading

forslund Sep 9, 2020 •

edited

Loading

forslund Aug 14, 2020 •

edited

Loading

forslund commented Aug 14, 2020 •

edited

Loading

pep8speaks commented Aug 17, 2020 •

edited

Loading