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

Clear registered vocab when skill is unloaded #2867

Closed
wants to merge 1 commit into from

Conversation

forslund
Copy link
Collaborator

Description

This adds a search and destroy method moving through the Adapt Trie removing any keywords belonging to the skill when skill is shutdown.

This should fix 50% of the issues described in #2866.

How to test

  • Start Mycroft
  • Enter the word jokes in the CLI and assert you get a suitably bad joke
  • Take the joke skill's Joke.voc vocab and remove the word jokes then save the file
  • Skill will reload
  • Enter the word jokes again and assert that Mycroft don't know what your talking about
  • Enter joke and make sure that works as expected

Contributor license agreement signed?

CLA [ yes ]

@devops-mycroft devops-mycroft added the CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors) label Mar 18, 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

@JarbasAl
Copy link
Contributor

shouldn't this be made against adapt repo instead? feels like the find_type method belongs somewhere in there

if its faster it can be merged as a workaround, but maybe leave a TODO + Issue in adapt repo

@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
Copy link
Collaborator Author

I'm still pondering on how to do it best in adapt. Possibly a search/remove using regex / globbing matches.

Adapt has by tradition no remove code so we'll see how well those patches are received :)

This adds a search and destroy method moving through the Adapt Trie
removing any keywords starting with the skill id.
@forslund
Copy link
Collaborator Author

Opened adapt PR MycroftAI/adapt#122

@devops-mycroft
Copy link

Voight Kampff Integration Test Succeeded (Results)

@forslund
Copy link
Collaborator Author

Going to close this, got some excellent feedback over at the Adapt PR telling me why this is not a good idea and will iterate on it there.

@forslund forslund closed this Mar 19, 2021
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)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants