Skip to content
This repository was archived by the owner on Sep 8, 2024. It is now read-only.

Feature - check if services ready on boot #2642

Closed
wants to merge 14 commits into from

Conversation

krisgesling
Copy link
Contributor

DO NOT MERGE

Description

Some example code following on from issue #2639

This shifts the mycroft.ready message from the Padatious Service to the Enclosure.

It sends a request to each service over the message bus to respond if it's alive. As such it assumes that both the bus and the enclosure services are alive as they're required for successfully transmission of the messages.

In it's current state it will always timeout as there's no handler added to the Skills service. I'm sure there's also a better way to handle checking the speech service, and general refactoring is needed.

How to test

Boot Mycroft - should startup, train all intents, then check each service for 2 mins and timeout.

Remove skills service from list to check and it should emit mycroft.ready as normal.

Questions

  • Is this a useful approach?
  • Does it matter that it relies on the enclosure running?
  • Presuming we have watchdogs running on each service, what do we do if the boot completely times out? Restart the device?

forslund and others added 6 commits July 22, 2020 10:52
- skills
- audio
- speech client
- messagebus service
- enclosure

The main functions now accepts the arguments ready_hook and error_hook
allowing a service or runner script to catch these states and perform
actions accordingly.

This is useful for things like systemd or a desktop launcher.

Fix audio service startup
If no watchdog is provided a dummy function will be called
Also asks each service to report back before
declaring mycroft.ready
@krisgesling krisgesling added the Status: Work in progress PR being actively worked on, not yet ready for review. label Jul 24, 2020
@devops-mycroft devops-mycroft added the CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors) label Jul 24, 2020
@devops-mycroft
Copy link

Voight Kampff Integration Test Succeeded (Results)

forslund and others added 3 commits July 24, 2020 22:12
The new wait_for_loaded() method will wait until services has been
loaded. It has a 3 minute timeout (default)

This improves the audio service readiness indication to not trigger
until the services are loaded.
@pep8speaks
Copy link

pep8speaks commented Jul 27, 2020

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

Line 211:1: W293 blank line contains whitespace

Line 85:80: E501 line too long (99 > 79 characters)

Comment last updated at 2020-07-27 10:41:21 UTC

@devops-mycroft
Copy link

Voight Kampff Integration Test Succeeded (Results)



def on_ready():
alive_status.set()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since these can be overridden the setting in the alive_status here may not be the best way to do this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah good point, maybe I'll shift them to where the hooks are called from.

Does the overall approach make sense though? Should we be moving straight to an external monitoring service or something else?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It sort of makes sense with the other things the enclosure does. I don't have a better suggestion...

@forslund
Copy link
Collaborator

forslund commented Jul 28, 2020

I wonder if a separate class for tracking the status and handling the reporting and callbacks would be a good idea. Something along the lines of: forslund@da94167

Each process creates an ProcessStatus object and it handles callbacks and reporting readiness over messagebus

@j1nx
Copy link

j1nx commented Jul 29, 2020

Interesting! Does this also mean that the ready status is properly reported back into the pairing skill?
https://github.com/MycroftAI/skill-pairing/blob/20.02/__init__.py#L188

Because the pairing skill always speaks the paired dialog, but never the wait for ready dialog.

@forslund
Copy link
Collaborator

It should improve things but there's still the case when default skills aren't installed that needs to be handled. But that's probably a separate issue

@j1nx
Copy link

j1nx commented Jul 29, 2020

Ok, then I think it is best that I create an seperate issue at that repo referencing to this one.

@krisgesling krisgesling removed the Status: Work in progress PR being actively worked on, not yet ready for review. label Jul 31, 2020
@krisgesling
Copy link
Contributor Author

Closing in favour of #2645

Created in a new PR to maintain discussion and it's context here.

@krisgesling krisgesling deleted the feature/services-ready branch March 18, 2021 05:55
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants