Handle multiple intents with the same name #2921
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
devops-mycroft
added
the
CLA: Yes
forslund
force-pushed
the
bugfix/multiple-intents-with-same-name
branch
from
3a8c9d9
to
aa962b6
Compare
|
Voight Kampff Integration Test Succeeded (Results) |
|
Voight Kampff Integration Test Failed (Results). |
|
Voight Kampff Integration Test Succeeded (Results) |
JarbasAl
pushed a commit
to HelloChatterbox/HolmesIV
that referenced
this pull request
Jul 1, 2021
cherry pick of MycroftAI#2921 Make enable/disable intent handle the new exception The enable/disable intent did not mark an intent as detached, instead it remained in the list of intents after disabling in the IntentServiceInterface to be retrieved when the intent should be re-enabled. This moves detached intents into a list of detached intents to so they won't cause the double enable exception. Add tests for intent collisions Add check for duplicate adapt intents There are two cases, duplicated named intent and duplicated anonymous intent. A named intent will cause a ValueError exception notifying the skill author that there is a collision. An anonymous intent will silently derive a new name and use that instead of the default generated one.
JarbasAl
pushed a commit
to HelloChatterbox/HolmesIV
that referenced
this pull request
Jul 1, 2021
cherry pick of MycroftAI#2921 Make enable/disable intent handle the new exception The enable/disable intent did not mark an intent as detached, instead it remained in the list of intents after disabling in the IntentServiceInterface to be retrieved when the intent should be re-enabled. This moves detached intents into a list of detached intents to so they won't cause the double enable exception. Add tests for intent collisions Add check for duplicate adapt intents There are two cases, duplicated named intent and duplicated anonymous intent. A named intent will cause a ValueError exception notifying the skill author that there is a collision. An anonymous intent will silently derive a new name and use that instead of the default generated one.
JarbasAl
pushed a commit
to HelloChatterbox/HolmesIV
that referenced
this pull request
Jul 1, 2021
cherry pick of MycroftAI#2921 Make enable/disable intent handle the new exception The enable/disable intent did not mark an intent as detached, instead it remained in the list of intents after disabling in the IntentServiceInterface to be retrieved when the intent should be re-enabled. This moves detached intents into a list of detached intents to so they won't cause the double enable exception. Add tests for intent collisions Add check for duplicate adapt intents There are two cases, duplicated named intent and duplicated anonymous intent. A named intent will cause a ValueError exception notifying the skill author that there is a collision. An anonymous intent will silently derive a new name and use that instead of the default generated one.
JarbasAl
pushed a commit
to HelloChatterbox/HolmesIV
that referenced
this pull request
Jul 1, 2021
cherry pick of MycroftAI#2921 Make enable/disable intent handle the new exception The enable/disable intent did not mark an intent as detached, instead it remained in the list of intents after disabling in the IntentServiceInterface to be retrieved when the intent should be re-enabled. This moves detached intents into a list of detached intents to so they won't cause the double enable exception. Add tests for intent collisions Add check for duplicate adapt intents There are two cases, duplicated named intent and duplicated anonymous intent. A named intent will cause a ValueError exception notifying the skill author that there is a collision. An anonymous intent will silently derive a new name and use that instead of the default generated one.
JarbasAl
pushed a commit
to HelloChatterbox/HolmesIV
that referenced
this pull request
Jul 5, 2021
cherry pick of MycroftAI#2921 Make enable/disable intent handle the new exception The enable/disable intent did not mark an intent as detached, instead it remained in the list of intents after disabling in the IntentServiceInterface to be retrieved when the intent should be re-enabled. This moves detached intents into a list of detached intents to so they won't cause the double enable exception. Add tests for intent collisions Add check for duplicate adapt intents There are two cases, duplicated named intent and duplicated anonymous intent. A named intent will cause a ValueError exception notifying the skill author that there is a collision. An anonymous intent will silently derive a new name and use that instead of the default generated one.
JarbasAl
pushed a commit
to HelloChatterbox/HolmesIV
that referenced
this pull request
Jul 5, 2021
cherry pick of MycroftAI#2921 Make enable/disable intent handle the new exception The enable/disable intent did not mark an intent as detached, instead it remained in the list of intents after disabling in the IntentServiceInterface to be retrieved when the intent should be re-enabled. This moves detached intents into a list of detached intents to so they won't cause the double enable exception. Add tests for intent collisions Add check for duplicate adapt intents There are two cases, duplicated named intent and duplicated anonymous intent. A named intent will cause a ValueError exception notifying the skill author that there is a collision. An anonymous intent will silently derive a new name and use that instead of the default generated one.
JarbasAl
pushed a commit
to HelloChatterbox/HolmesIV
that referenced
this pull request
Jul 5, 2021
cherry pick of MycroftAI#2921 Make enable/disable intent handle the new exception The enable/disable intent did not mark an intent as detached, instead it remained in the list of intents after disabling in the IntentServiceInterface to be retrieved when the intent should be re-enabled. This moves detached intents into a list of detached intents to so they won't cause the double enable exception. Add tests for intent collisions Add check for duplicate adapt intents There are two cases, duplicated named intent and duplicated anonymous intent. A named intent will cause a ValueError exception notifying the skill author that there is a collision. An anonymous intent will silently derive a new name and use that instead of the default generated one.
JarbasAl
pushed a commit
to HelloChatterbox/HolmesIV
that referenced
this pull request
Jul 6, 2021
cherry pick of MycroftAI#2921 Make enable/disable intent handle the new exception The enable/disable intent did not mark an intent as detached, instead it remained in the list of intents after disabling in the IntentServiceInterface to be retrieved when the intent should be re-enabled. This moves detached intents into a list of detached intents to so they won't cause the double enable exception. Add tests for intent collisions Add check for duplicate adapt intents There are two cases, duplicated named intent and duplicated anonymous intent. A named intent will cause a ValueError exception notifying the skill author that there is a collision. An anonymous intent will silently derive a new name and use that instead of the default generated one.
JarbasAl
pushed a commit
to HelloChatterbox/HolmesIV
that referenced
this pull request
Jul 6, 2021
cherry pick of MycroftAI#2921 Make enable/disable intent handle the new exception The enable/disable intent did not mark an intent as detached, instead it remained in the list of intents after disabling in the IntentServiceInterface to be retrieved when the intent should be re-enabled. This moves detached intents into a list of detached intents to so they won't cause the double enable exception. Add tests for intent collisions Add check for duplicate adapt intents There are two cases, duplicated named intent and duplicated anonymous intent. A named intent will cause a ValueError exception notifying the skill author that there is a collision. An anonymous intent will silently derive a new name and use that instead of the default generated one.
JarbasAl
pushed a commit
to HelloChatterbox/HolmesIV
that referenced
this pull request
Jul 6, 2021
cherry pick of MycroftAI#2921 Make enable/disable intent handle the new exception The enable/disable intent did not mark an intent as detached, instead it remained in the list of intents after disabling in the IntentServiceInterface to be retrieved when the intent should be re-enabled. This moves detached intents into a list of detached intents to so they won't cause the double enable exception. Add tests for intent collisions Add check for duplicate adapt intents There are two cases, duplicated named intent and duplicated anonymous intent. A named intent will cause a ValueError exception notifying the skill author that there is a collision. An anonymous intent will silently derive a new name and use that instead of the default generated one.
JarbasAl
pushed a commit
to HelloChatterbox/HolmesIV
that referenced
this pull request
Jul 7, 2021
cherry pick of MycroftAI#2921 Make enable/disable intent handle the new exception The enable/disable intent did not mark an intent as detached, instead it remained in the list of intents after disabling in the IntentServiceInterface to be retrieved when the intent should be re-enabled. This moves detached intents into a list of detached intents to so they won't cause the double enable exception. Add tests for intent collisions Add check for duplicate adapt intents There are two cases, duplicated named intent and duplicated anonymous intent. A named intent will cause a ValueError exception notifying the skill author that there is a collision. An anonymous intent will silently derive a new name and use that instead of the default generated one.
JarbasAl
pushed a commit
to HelloChatterbox/HolmesIV
that referenced
this pull request
Jul 7, 2021
cherry pick of MycroftAI#2921 Make enable/disable intent handle the new exception The enable/disable intent did not mark an intent as detached, instead it remained in the list of intents after disabling in the IntentServiceInterface to be retrieved when the intent should be re-enabled. This moves detached intents into a list of detached intents to so they won't cause the double enable exception. Add tests for intent collisions Add check for duplicate adapt intents There are two cases, duplicated named intent and duplicated anonymous intent. A named intent will cause a ValueError exception notifying the skill author that there is a collision. An anonymous intent will silently derive a new name and use that instead of the default generated one.
JarbasAl
pushed a commit
to HelloChatterbox/HolmesIV
that referenced
this pull request
Jul 7, 2021
cherry pick of MycroftAI#2921 Make enable/disable intent handle the new exception The enable/disable intent did not mark an intent as detached, instead it remained in the list of intents after disabling in the IntentServiceInterface to be retrieved when the intent should be re-enabled. This moves detached intents into a list of detached intents to so they won't cause the double enable exception. Add tests for intent collisions Add check for duplicate adapt intents There are two cases, duplicated named intent and duplicated anonymous intent. A named intent will cause a ValueError exception notifying the skill author that there is a collision. An anonymous intent will silently derive a new name and use that instead of the default generated one.
JarbasAl
pushed a commit
to HelloChatterbox/HolmesIV
that referenced
this pull request
Jul 7, 2021
cherry pick of MycroftAI#2921 Make enable/disable intent handle the new exception The enable/disable intent did not mark an intent as detached, instead it remained in the list of intents after disabling in the IntentServiceInterface to be retrieved when the intent should be re-enabled. This moves detached intents into a list of detached intents to so they won't cause the double enable exception. Add tests for intent collisions Add check for duplicate adapt intents There are two cases, duplicated named intent and duplicated anonymous intent. A named intent will cause a ValueError exception notifying the skill author that there is a collision. An anonymous intent will silently derive a new name and use that instead of the default generated one.
JarbasAl
pushed a commit
to HelloChatterbox/HolmesIV
that referenced
this pull request
Jul 7, 2021
cherry pick of MycroftAI#2921 Make enable/disable intent handle the new exception The enable/disable intent did not mark an intent as detached, instead it remained in the list of intents after disabling in the IntentServiceInterface to be retrieved when the intent should be re-enabled. This moves detached intents into a list of detached intents to so they won't cause the double enable exception. Add tests for intent collisions Add check for duplicate adapt intents There are two cases, duplicated named intent and duplicated anonymous intent. A named intent will cause a ValueError exception notifying the skill author that there is a collision. An anonymous intent will silently derive a new name and use that instead of the default generated one.
JarbasAl
pushed a commit
to HelloChatterbox/HolmesIV
that referenced
this pull request
Jul 9, 2021
cherry pick of MycroftAI#2921 Make enable/disable intent handle the new exception The enable/disable intent did not mark an intent as detached, instead it remained in the list of intents after disabling in the IntentServiceInterface to be retrieved when the intent should be re-enabled. This moves detached intents into a list of detached intents to so they won't cause the double enable exception. Add tests for intent collisions Add check for duplicate adapt intents There are two cases, duplicated named intent and duplicated anonymous intent. A named intent will cause a ValueError exception notifying the skill author that there is a collision. An anonymous intent will silently derive a new name and use that instead of the default generated one.
JarbasAl
pushed a commit
to HelloChatterbox/HolmesIV
that referenced
this pull request
Jul 9, 2021
cherry pick of MycroftAI#2921 Make enable/disable intent handle the new exception The enable/disable intent did not mark an intent as detached, instead it remained in the list of intents after disabling in the IntentServiceInterface to be retrieved when the intent should be re-enabled. This moves detached intents into a list of detached intents to so they won't cause the double enable exception. Add tests for intent collisions Add check for duplicate adapt intents There are two cases, duplicated named intent and duplicated anonymous intent. A named intent will cause a ValueError exception notifying the skill author that there is a collision. An anonymous intent will silently derive a new name and use that instead of the default generated one.
JarbasAl
pushed a commit
to HelloChatterbox/HolmesIV
that referenced
this pull request
Jul 9, 2021
cherry pick of MycroftAI#2921 Make enable/disable intent handle the new exception The enable/disable intent did not mark an intent as detached, instead it remained in the list of intents after disabling in the IntentServiceInterface to be retrieved when the intent should be re-enabled. This moves detached intents into a list of detached intents to so they won't cause the double enable exception. Add tests for intent collisions Add check for duplicate adapt intents There are two cases, duplicated named intent and duplicated anonymous intent. A named intent will cause a ValueError exception notifying the skill author that there is a collision. An anonymous intent will silently derive a new name and use that instead of the default generated one.
JarbasAl
pushed a commit
to HelloChatterbox/HolmesIV
that referenced
this pull request
Jul 9, 2021
cherry pick of MycroftAI#2921 Make enable/disable intent handle the new exception The enable/disable intent did not mark an intent as detached, instead it remained in the list of intents after disabling in the IntentServiceInterface to be retrieved when the intent should be re-enabled. This moves detached intents into a list of detached intents to so they won't cause the double enable exception. Add tests for intent collisions Add check for duplicate adapt intents There are two cases, duplicated named intent and duplicated anonymous intent. A named intent will cause a ValueError exception notifying the skill author that there is a collision. An anonymous intent will silently derive a new name and use that instead of the default generated one.
krisgesling
added
the
Status: To be reviewed
|
If an active intent is re-registered, it raises an exception that wasn't previously there which results in a spoken error. I think this could be safely caught in the |
|
Thanks for flagging this. I think the registering should raise the exception but it should be caught and logged by the enable_intent. Does that sound like a good solution? |
There are two cases, duplicated named intent and duplicated anonymous intent. A named intent will cause a ValueError exception notifying the skill author that there is a collision. An anonymous intent will silently derive a new name and use that instead of the default generated one.
The enable/disable intent did not mark an intent as detached, instead it remained in the list of intents after disabling in the IntentServiceInterface to be retrieved when the intent should be re-enabled. This moves detached intents into a list of detached intents to so they won't cause the double enable exception.
MycroftSkill.enable_intent() will now check if the intent is detached before trying to re-enable it.
forslund
force-pushed
the
bugfix/multiple-intents-with-same-name
branch
from
aa962b6
to
0b5e7b8
Compare
JarbasAl
reviewed
Oct 22, 2021
| nbr += 1 | ||
| name = f'{original_name}{nbr}' | ||
| else: | ||
| if name in self.intent_service: |
There was a problem hiding this comment.
i think this should be if name in self.intent_service.registered_intents: to account for intent enabling
caused some issues when playing around with IntentLayers that enable/disable intents
(might have been a race condition, it worked most of the time and doesn't make sense to only fail sometimes)
There was a problem hiding this comment.
Hmm, that would be basically the same thing if I understand your suggestion correctly. The __contians__ checks for names in registered_intents. the line above basically expands to
if name in [i[0] for i in self.intent_service.registered_intents]:What sort of failure did you get?
There was a problem hiding this comment.
It threw the exception about intent name being taken, i thought that contains also checked for disabled intents, my suggestion was meant to not throw the exception if the intent was disabled
There was a problem hiding this comment.
I've updated the code with a quick locking around the critical sections. They're rather clumsy at the moment but hopefully there won't be deadlocks and should handle race conditions here
There was a problem hiding this comment.
Thanks for the clarification (firefox didn't update the page so I missed your quick reply). The detach() method moves the intent from the registered_intents list to the detached_intents list so it shouldn't throw unless the intent is really active at the moment.
Buuuut... it was long ago since I looked at this branch so might miss-remember. Will dig into the code and try to recall what my intention was.
This should avoid some race conditions that may occur if multiple threads tries to enable / disable intents
forslund
force-pushed
the
bugfix/multiple-intents-with-same-name
branch
from
5f707bb
to
8b835d0
Compare
I think for backwards-compatibility these exceptions should be handled (catch and log), maybe with a deprecation notice that future releases will raise an exception if an active intent is activated. |
|
It's now more than a year since the last concern raised here was pushed (I think), is this something the core team is interested in or should I close the PR? |
|
Hey Ake, I think it's a good change - seems like everything is accounted for and it passed all the tests. So if there's no opposition I think we can merge it in. I'll do this on Monday/Tuesday unless I hear otherwise. |
krisgesling
approved these changes
Nov 23, 2022
JarbasAl
pushed a commit
to OpenVoiceOS/ovos-core
that referenced
this pull request
Nov 26, 2022
* Add check for duplicate adapt intents There are two cases, duplicated named intent and duplicated anonymous intent. A named intent will cause a ValueError exception notifying the skill author that there is a collision. An anonymous intent will silently derive a new name and use that instead of the default generated one. * Add tests for intent collisions * Make enable/disable intent handle the new exception The enable/disable intent did not mark an intent as detached, instead it remained in the list of intents after disabling in the IntentServiceInterface to be retrieved when the intent should be re-enabled. This moves detached intents into a list of detached intents to so they won't cause the double enable exception. * Add move logic to find if intent is detached MycroftSkill.enable_intent() will now check if the intent is detached before trying to re-enable it. * Lock updates of intents This should avoid some race conditions that may occur if multiple threads tries to enable / disable intents
JarbasAl
added a commit
to OpenVoiceOS/ovos-core
that referenced
this pull request
Dec 13, 2022
* Handle multiple intents with the same name (MycroftAI#2921) * Add check for duplicate adapt intents There are two cases, duplicated named intent and duplicated anonymous intent. A named intent will cause a ValueError exception notifying the skill author that there is a collision. An anonymous intent will silently derive a new name and use that instead of the default generated one. * Add tests for intent collisions * Make enable/disable intent handle the new exception The enable/disable intent did not mark an intent as detached, instead it remained in the list of intents after disabling in the IntentServiceInterface to be retrieved when the intent should be re-enabled. This moves detached intents into a list of detached intents to so they won't cause the double enable exception. * Add move logic to find if intent is detached MycroftSkill.enable_intent() will now check if the intent is detached before trying to re-enable it. * Lock updates of intents This should avoid some race conditions that may occur if multiple threads tries to enable / disable intents * refactor a bit * standardize usage of intent name in all methods Co-authored-by: Åke <ake.forslund@gmail.com>
23 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This implements Alternative 1 from #2920
ValueErrorif two namedIntentBuilders with the same name is registeredIntentBuilders are used they will be automatically given an unique nameThe handling for enable-disable intent was updated to make sure that the intent was removed from the list of
registered_intents, for re-enabling it's moved to a list ofdetached_intentsHow to test
Add a second
@intent_handler()decorator to a method ensure both can be used to trigger the handler and that the handler is triggered exactly once.Contributor license agreement signed?
CLA [ Yes ]