-
-
Notifications
You must be signed in to change notification settings - Fork 422
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
[voice] Add length limit to TTS handled by cache #3699
Conversation
We can safely assume that long TTS are generated for report (meteo, chatGPT ?), probably not meant to be repeated, and so not cached. Signed-off-by: Gwendal Roulleau <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Looks to me like a very good idea. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general LGTM. Is the limit in characters? Maybe add that to the description.
....openhab.core.voice/src/main/java/org/openhab/core/voice/internal/cache/TTSLRUCacheImpl.java
Outdated
Show resolved
Hide resolved
bundles/org.openhab.core.voice/src/main/resources/OH-INF/config/voice.xml
Outdated
Show resolved
Hide resolved
Apply code review Signed-off-by: Gwendal Roulleau <[email protected]>
Thank you both for your reviews ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks
During the TTS cache development, there was some discussion about adding a boolean to enable / disable cache per request. We didn't keep that because it makes the API complicated.
But time passed and new usages arise, and maybe it now can be discussed, on a new form ?
For example, there is now a ChatGPT binding. It is possible to make some advanced chat/queries with a LLM. Most of the time, the chat response are lengthy. And so one "discussion" with a LLM can clog the LRU cache and removes older entries worth keeping.
Proposal :
We can use the TTS length as a simple way to differentiate TTS sentences that should be cached, automatically.
TTS we should cache are those that can be repeated, and they are nearly always pretty short. examples :
"Ok, I'll do that"
"Hello, what can I do for you ?",
"You've got a message"
"Please close the garage door".
On the other side, one can safely assume that long TTS are generated for report of some kind (meteo, chatGPT ?), probably not meant to be repeated. They are also the ones taking the most space in the cache.
I put a 150 default character limit (configurable). To be discussed if the whole idea worth something.