Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove registered keywords on skill shutdown #2877

Merged

Conversation

forslund
Copy link
Collaborator

@forslund forslund commented Apr 2, 2021

Description

Currently Mycroft only removes adapt intents when skill is shutdown any keyword or regex registered will remain registered until shutdown of mycroft-core. This PR uses the new drop feature of Adapt to remove a skill's registered keywords and regexes when shutdown.

This will make updating skill vocabularies and regexes without restarting the skill process more intuitive.

Should resolve #2866 and possibly #2294

How to test

Create a simple skill with adapt intents. Add and remove keywords from a .voc-file and assert that they are added / removed as expected.

Contributor license agreement signed?

[ yes ]

@forslund forslund added Type: Enhancement - proposed New proposal for a feature that is not currently a priority on the roadmap. Status: Work in progress PR being actively worked on, not yet ready for review. labels Apr 2, 2021
@devops-mycroft devops-mycroft added the CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors) label Apr 2, 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

@forslund forslund removed the Status: Work in progress PR being actively worked on, not yet ready for review. label Apr 5, 2021
@devops-mycroft
Copy link

Voight Kampff Integration Test Succeeded (Results)

krisgesling and others added 2 commits May 27, 2021 15:36
@forslund forslund force-pushed the feature/clear-adapt-keywords-on-shutdown branch from d753f64 to ad410d4 Compare May 27, 2021 20:25
@codecov-commenter
Copy link

Codecov Report

Merging #2877 (712e33e) into dev (6c0be72) will increase coverage by 0.08%.
The diff coverage is 92.85%.

❗ Current head 712e33e differs from pull request most recent head ad410d4. Consider uploading reports for the commit ad410d4 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##              dev    #2877      +/-   ##
==========================================
+ Coverage   52.48%   52.57%   +0.08%     
==========================================
  Files         123      123              
  Lines       10970    10992      +22     
==========================================
+ Hits         5758     5779      +21     
- Misses       5212     5213       +1     
Impacted Files Coverage Δ
mycroft/skills/intent_services/adapt_service.py 80.57% <92.85%> (+2.79%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6c0be72...ad410d4. Read the comment docs.

@devops-mycroft
Copy link

Voight Kampff Integration Test Succeeded (Results)

Copy link
Contributor

@krisgesling krisgesling left a comment

Choose a reason for hiding this comment

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

Seems to be working great, thanks!

self.engine.register_regex_entity(regex_str)
else:
self.engine.register_entity(
start_concept, end_concept, alias_of=alias_of)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is beyond the scope of this PR but I just went down a small rabbit hole of what start vs end concepts were and I think that these need to be renamed to better match it's usage eg Adapt now seems to use entity_value, entity_type.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's a terrific idea. it's always been a bit (to say the least) un-intuitive names

Copy link
Contributor

Choose a reason for hiding this comment

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

i always need to double check what these mean, i approve the renaming 👍

@krisgesling krisgesling merged commit 6165c2d into MycroftAI:dev May 28, 2021
Roadmap automation moved this from Inbox to Done May 28, 2021
@forslund forslund deleted the feature/clear-adapt-keywords-on-shutdown branch June 4, 2021 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors) Type: Enhancement - proposed New proposal for a feature that is not currently a priority on the roadmap.
Projects
Roadmap
  
Done
Development

Successfully merging this pull request may close these issues.

Adapt keywords / regexes aren't removed together with intents.
5 participants