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

Add a way to subscribe to all intents with a prefix #13

Open
koenvervloesem opened this issue Jul 17, 2020 · 10 comments
Open

Add a way to subscribe to all intents with a prefix #13

koenvervloesem opened this issue Jul 17, 2020 · 10 comments
Labels
enhancement New feature or request question Further information is requested

Comments

@koenvervloesem
Copy link
Member

koenvervloesem commented Jul 17, 2020

Something like this could be interesting for some use case:

@app.on_intent("megaApp", prefix = True)

This would then subscribe to intents megaApp/GetTime, megaApp/GetDate and so on.

Suggested by @JonahKr

Does anyone have a use case for this?

@koenvervloesem koenvervloesem added enhancement New feature or request question Further information is requested labels Jul 17, 2020
@JonahKr
Copy link
Contributor

JonahKr commented Jul 17, 2020

One use case i came up with is that it enables a easy way to handle multiple skills via one entrypoint and run a specific function/module only based on the name.
Additionally it prevents naming errors.

@maxbachmann
Copy link
Member

maxbachmann commented Aug 26, 2020

Shouldn't this already be possible using the standard mqtt topics with this decorator aswell (I did not test it)?
I would expect

@app.on_intent("megaApp/+")

to receive all messages one namespace below megaApp and

@app.on_intent("megaApp/#")

to receive all messages below megaApp.

At least for me

@app.on_intent("megaApp", prefix = True)

is not really clearer, since at least for me it is unclear what prefix means here. It could mean I receive megaApp/+, megaApp/# or even something like megaApp* (this last one might have been useful in snips where the topic was <username>:<intentname>, which can not be filtered with the standard mqtt features)

@koenvervloesem
Copy link
Member Author

koenvervloesem commented Aug 26, 2020

This doesn't work at the moment (I just tested it), because the intent names are really interpreted as intent names in _subscribe_callbacks, and then converted to MQTT topics:

    topics: List[str] = [
        NluIntent.topic(intent_name=intent_name) for intent_name in intent_names
    ]

So if we want to support this syntax, we would have to unravel the underlying MQTT topic already in the decorator, taking into account the wildcards, and then the callbacks should be added differently. Or the NluIntent.topic method of rhasspy-hermes could be changed to take into account the prefix.

I'm not convinced yet about having MQTT topic wildcards in the intent_names argument of on_intent. The API of on_intent is expressly designed to abstract away the MQTT stuff. People shouldn't have to know MQTT topic syntax to be able to use it. But maybe we should make an exception here. They would have to know the syntax of megaApp/megaIntent for prefixes anyway because they have to type this into the sentences.ini of the app too. And the use of the decorator with non-wildcards stays the same.

@maxbachmann
Copy link
Member

Another option would be to completely abstract this namespace away everywhere. So this namespace would simply always be the skill name.
-> in the sentences.ini the user would write the normal intents and the skill would automatically add <skill_name>/ in front of each intent (pretty much like snips automatically added <user_name>: automatically) before sending the file over to rhasspy for training
and then in the code you could use

@app.on_intent("megaIntent")

to listen for hermes/intent/megaApp/megaIntent and

@app.on_intent()

to listen for hermes/intent/megaApp/#

@koenvervloesem
Copy link
Member Author

I like this idea! An app shouldn't listen on intents of other apps anyway.

@JonahKr I suppose this still fits your use case?

@JonahKr
Copy link
Contributor

JonahKr commented Aug 26, 2020

I think so. Yes.
But how would different intents look like then within rhasspy?
something like:
Megaapp/miniapp?
And if so, would that even work with the current rhasspy version? I sadly cannot test it atm.

@koenvervloesem
Copy link
Member Author

Yes, this already works: rhasspy/rhasspy-hermes#12 (comment)

The "real" intent name that Rhasspy sees would then indeed be megaApp/megaIntent, while in the app itself it's shown as megaIntent.

@koenvervloesem
Copy link
Member Author

So the only thing holding me back now to implement this change is this: what if someone wants to write an app to react to some "general" intents that have been installed separately, and thus without the app name as a prefix? For instance, on the forum there were some discussions about supplying general intents, like yes/no, common media player intents and so on. Do we want to support this in Rhasspy Hermes App? Or does it make sense to only support intents that are supplied together with the app in an intents file?

@JonahKr
Copy link
Contributor

JonahKr commented Aug 28, 2020

Wouldn't this still be possible by subscribing to wildcards? Something like #/yes_intent?
Of course the level of subscription would have to be adjusted in the rhasspy hermes app backend🤔

@maxbachmann
Copy link
Member

Hm maybe make the decorator

on_intent(intent="#", namespace=skill_name)

and then have a logic along the lines

intent = f"{namespace}/{intent}" if namespace else intent

this way the simple default behaviour is

@app.on_intent("megaIntent")

to listen for hermes/intent/megaApp/megaIntent

@app.on_intent()

to listen for hermes/intent/megaApp/#

and something like

@app.on_intent("yes_no", namespace=None)

or

@app.on_intent("yes_no", namespace="common")

to listen to other namespaces

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants