-
-
Notifications
You must be signed in to change notification settings - Fork 52
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
Make MassPlayMediaOnMediaPlayer work directly from the new LLMs in 2024.6 without the need of a separate conversation agent #2434
Conversation
As a result of this change what information is sent to the LLM as a result of the request? Is it still just the prompt and the query? |
Yesterday I was trying to work on removing the need for a separate agent, and decided that it really needs the prompt to be able to return things as we expect them, however, I like the fact that you're supporting both use cases here. I appreciate the clean up too, sometimes you simply can't see the wood for the trees, but it was definitely feeling messy overall. I'll try test the changes later, but I like the direction. I'm also considering whether it makes sense to calling the exposed service rather than the api directly, but haven't made a decision here or there. |
Perhaps I'm too stupid to test this, but I can't get this to work reliably at all. @tronikos Exactly how have you set things up for your testing? |
@OzGav @jozefKruszynski for testing follow these steps:
|
Got it working, had to remove and re-add the integration and disable my open ai integration that has the prompt. I already disabled the open ai integration earlier, but for some reason the prompt was clearly still being used somehow. |
I just want to confirm that if we did this change it is still possible to send the minimal prompt+query to the LLM as I am not interested in sending all my house details to it (nor paying for that). I think based on what Jozef has said this is optional but want to confirm. |
correct |
I'll request some small changes later, but for the most part this looks good to me I think. @tronikos are you one the discord server? |
Yes I'm on the discord server. My username is tronikos. |
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.
Overall, I think the changes make sense, please add the return type annotations as requested below and make sure to run pre-commit run --all-files
so that the linter and other checks all succeed.
I'll test again when I get home this evening |
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 !
(just note the couple of linter fixes that are needed)
b10329f
I fixed the linter but it's in an unrelated file |
ah, probbaly from an earlier merge or a bump of ruff or whatever. Thanks! |
Google Generative AI and OpenAI conversation agents in 2024.6 can call registered intents. The LLMs can already infer artist/track/album from the command and we can avoid the need of setting up a separate conversation agent to pass the query. This also saves an LLM request. I tried it with the examples in https://github.com/music-assistant/hass-music-assistant/blob/main/prompt/prompt.txt and the LLM was able to correctly pass the artist/track/album even for the "play the artist that composed the soundtrack of Inception" example. For the "play a list of 5 classic 80's rock tracks" it called MassPlayMediaOnMediaPlayer with query="list of 5 classic 80's rock tracks".
While I was here I refactored the code a bit to make it more readable and improved error handling.