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

Utilize the mycroft-messagebus-client module #2863

Merged
merged 1 commit into from
Mar 31, 2021

Conversation

forslund
Copy link
Collaborator

@forslund forslund commented Mar 17, 2021

Description

Thanks to @NeonDaniel the mycroft-messagebus-client is (almost) back in sync with core. Discussions in chat suggested that there might be a good idea to switch to using the common module. It was proposed back in #2431, but perhaps the world is ready for this abstraction now :)

Currently still in WIP due to a pending PR, VK fails due to the git+ urls aren't parsed in setup.py without jumping through some hoops. But can be tested locally using the dev_setup.sh script.

The pros are

  • the client and core won't get out of sync
  • less code in core

The cons:

  • one more dependency

How to test

Make sure mycroft runs as expected

Contributor license agreement signed?

CLA [ yes ]

@forslund forslund added Status: Work in progress PR being actively worked on, not yet ready for review. Status: For discussion Feature proposal in development. Community input and discussion is invited. labels Mar 17, 2021
@devops-mycroft devops-mycroft added the CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors) label Mar 17, 2021
@devops-mycroft
Copy link

Voight Kampff Integration Test Failed (Results).
Mycroft logs are also available: skills.log, audio.log, voice.log, bus.log, enclosure.log

@NeonDaniel
Copy link
Member

I definitely support more modular code. I see this as a big benefit for plugins, etc. to be able to utilize the exact same messagebus package as core.

FWIW, I'm testing similar changes as these against the NeonAI fork of mycroft-core without issue.

@forslund forslund force-pushed the feature/mycroft-bus-client branch from 8b08387 to 33f320d Compare March 19, 2021 08:05
@forslund forslund added Type: Refactoring and other improvements Improvement of code and documentation that does not alter functionality. and removed Status: Work in progress PR being actively worked on, not yet ready for review. labels Mar 19, 2021
@forslund
Copy link
Collaborator Author

The requirements.txt has been updated with the released package.

@devops-mycroft
Copy link

Voight Kampff Integration Test Failed (Results).
Mycroft logs are also available: skills.log, audio.log, voice.log, bus.log, enclosure.log

@forslund forslund force-pushed the feature/mycroft-bus-client branch from 33f320d to 1e02094 Compare March 20, 2021 22:25
@devops-mycroft
Copy link

Voight Kampff Integration Test Succeeded (Results)

This utilizes the shared messagebus client implemented in the
mycroft-messagebus-client.
@forslund forslund force-pushed the feature/mycroft-bus-client branch from 1e02094 to 9acf5b7 Compare March 22, 2021 05:32
@devops-mycroft
Copy link

Voight Kampff Integration Test Succeeded (Results)

@NeonDaniel
Copy link
Member

Anything holding up this PR? The event-based waiting alone was a notable speed up to skill responses in my testing.

@krisgesling
Copy link
Contributor

Nope this all looks good.

I didn't expect such a noticeable speed improvement but the VK tests are a good indicator for this - they are running almost 20% quicker!

@krisgesling krisgesling merged commit 356288a into MycroftAI:dev Mar 31, 2021
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) Status: For discussion Feature proposal in development. Community input and discussion is invited. Type: Refactoring and other improvements Improvement of code and documentation that does not alter functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants