rhasspy / rhasspy-hermes-app

Helper library to create voice apps for Rhasspy in Python using the Hermes protocol
https://rhasspy-hermes-app.readthedocs.io
MIT License
16 stars 10 forks source link

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

Open koenvervloesem opened 4 years ago

koenvervloesem commented 4 years ago

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?

JonahKr commented 4 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

Yes, this already works: https://github.com/rhasspy/rhasspy-hermes/issues/12#issuecomment-637648461

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

koenvervloesem commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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