-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
[ChatGPT] Enhance binding #17320
[ChatGPT] Enhance binding #17320
Conversation
Here is jar for testing. |
I'm a Git noob. I tried to open a new PR with another addon. How can I create a new PR that does not include already existing commits in my fork? I have a new commit in the new branch, but when I try to open a PR, it includes all commits from this PR... |
@Artur-Fedjukevits You should always use feature branches and never work on |
<default>1</default> | ||
<advanced>true</advanced> | ||
</parameter> | ||
<parameter name="type" type="text"> |
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.
I think to remember that you said that this parameter is obsolete and can be removed?
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.
No, I didn't. I was talking about another parameter that has already been removed. I find the 'Top P' parameter useful.
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.
... but I am talking about the "type" parameter here.
bundles/org.openhab.binding.chatgpt/src/main/resources/json/tools.json
Outdated
Show resolved
Hide resolved
bundles/org.openhab.binding.chatgpt/src/main/resources/json/tools.json
Outdated
Show resolved
Hide resolved
Thanks. Yes, it's new to me. Everything in this field is new to me, I’m still learning. :) From now on, I'll do it. |
Signed-off-by: Artur-Fedjukevits <[email protected]>
This reverts commit 454384d. Signed-off-by: Artur-Fedjukevits <[email protected]>
…ols.json Co-authored-by: Kai Kreuzer <[email protected]> Signed-off-by: Artur-Fedjukevits <[email protected]> Signed-off-by: Artur-Fedjukevits <[email protected]>
…ols.json Co-authored-by: Kai Kreuzer <[email protected]> Signed-off-by: Artur-Fedjukevits <[email protected]> Signed-off-by: Artur-Fedjukevits <[email protected]>
1b66593
to
baf154d
Compare
Hi @kaikreuzer. Do you have time for review this PR? |
Still on my list, sorry for taking so long - it is not forgotten! |
Question out of curiosity - but is this based on the Assistants API/Functionality of OpenAI? |
It based on function calling via Chat Completions API |
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.
Thanks - I did a round of code review first, without actively testing the functionality yet. Please find my comments below.
...inding.chatgpt/src/main/java/org/openhab/binding/chatgpt/internal/hli/ChatGPTHLIService.java
Outdated
Show resolved
Hide resolved
bundles/org.openhab.binding.chatgpt/src/main/resources/OH-INF/thing/thing-types.xml
Outdated
Show resolved
Hide resolved
bundles/org.openhab.binding.chatgpt/src/main/resources/OH-INF/thing/thing-types.xml
Outdated
Show resolved
Hide resolved
bundles/org.openhab.binding.chatgpt/src/main/resources/OH-INF/thing/thing-types.xml
Outdated
Show resolved
Hide resolved
<default>1</default> | ||
<advanced>true</advanced> | ||
</parameter> | ||
<parameter name="type" type="text"> |
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.
... but I am talking about the "type" parameter here.
Ah, I myself was looking in the Assistents API (Which also supports Function Calling) as it seemed to be more fitting for controlling a home-automation system Also @kaikreuzer - kudo's on the reviewing, it's so concise and descriptive! Amazing! |
Co-authored-by: Kai Kreuzer <[email protected]> Signed-off-by: Artur-Fedjukevits <[email protected]>
Co-authored-by: Kai Kreuzer <[email protected]> Signed-off-by: Artur-Fedjukevits <[email protected]>
Co-authored-by: Kai Kreuzer <[email protected]> Signed-off-by: Artur-Fedjukevits <[email protected]>
Signed-off-by: Artur-Fedjukevits <[email protected]>
Signed-off-by: Artur-Fedjukevits <[email protected]>
…binding/chatgpt/internal/ChatGPTHandler.java Co-authored-by: Kai Kreuzer <[email protected]> Signed-off-by: Artur-Fedjukevits <[email protected]>
…binding/chatgpt/internal/ChatGPTHandler.java Co-authored-by: Kai Kreuzer <[email protected]> Signed-off-by: Artur-Fedjukevits <[email protected]>
…binding/chatgpt/internal/ChatGPTHandler.java Co-authored-by: Kai Kreuzer <[email protected]> Signed-off-by: Artur-Fedjukevits <[email protected]>
…binding/chatgpt/internal/hli/ChatGPTHLIService.java Co-authored-by: Kai Kreuzer <[email protected]> Signed-off-by: Artur-Fedjukevits <[email protected]>
…binding/chatgpt/internal/hli/ChatGPTHLIService.java Co-authored-by: Kai Kreuzer <[email protected]> Signed-off-by: Artur-Fedjukevits <[email protected]>
…binding/chatgpt/internal/hli/ChatGPTHLIService.java Co-authored-by: Kai Kreuzer <[email protected]> Signed-off-by: Artur-Fedjukevits <[email protected]>
…binding/chatgpt/internal/hli/ChatGPTHLIService.java Co-authored-by: Kai Kreuzer <[email protected]> Signed-off-by: Artur-Fedjukevits <[email protected]>
…binding/chatgpt/internal/hli/ChatGPTHLIService.java Co-authored-by: Kai Kreuzer <[email protected]> Signed-off-by: Artur-Fedjukevits <[email protected]>
…binding/chatgpt/internal/hli/ChatGPTHLIService.java Co-authored-by: Kai Kreuzer <[email protected]> Signed-off-by: Artur-Fedjukevits <[email protected]>
…binding/chatgpt/internal/hli/ChatGPTHLIService.java Co-authored-by: Kai Kreuzer <[email protected]> Signed-off-by: Artur-Fedjukevits <[email protected]>
Signed-off-by: Artur-Fedjukevits <[email protected]>
Thanks @kaikreuzer! I hope I’ve taken all your comments into account and fixed everything. I’m looking forward to your next comments :) |
@Artur-Fedjukevits Many thanks for this awesome PR - I have just started playing around with it a bit and what I have seen so far looks promising. LLMs bring human language interpretation to a whole new level 🚀 |
@Artur-Fedjukevits can you fix the merge conflict for the readme? |
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.
Thanks for addressing my comments. A few small things are still open, see my comments below.
bundles/org.openhab.binding.chatgpt/src/main/resources/OH-INF/thing/thing-types.xml
Outdated
Show resolved
Hide resolved
bundles/org.openhab.binding.chatgpt/src/main/resources/OH-INF/thing/thing-types.xml
Outdated
Show resolved
Hide resolved
bundles/org.openhab.binding.chatgpt/src/main/resources/OH-INF/thing/thing-types.xml
Outdated
Show resolved
Hide resolved
bundles/org.openhab.binding.chatgpt/src/main/resources/OH-INF/thing/thing-types.xml
Show resolved
Hide resolved
bundles/org.openhab.binding.chatgpt/src/main/resources/OH-INF/thing/thing-types.xml
Outdated
Show resolved
Hide resolved
bundles/org.openhab.binding.chatgpt/src/main/resources/OH-INF/thing/thing-types.xml
Outdated
Show resolved
Hide resolved
bundles/org.openhab.binding.chatgpt/src/main/resources/OH-INF/thing/thing-types.xml
Outdated
Show resolved
Hide resolved
bundles/org.openhab.binding.chatgpt/src/main/resources/OH-INF/thing/thing-types.xml
Outdated
Show resolved
Hide resolved
bundles/org.openhab.binding.chatgpt/src/main/resources/OH-INF/thing/thing-types.xml
Outdated
Show resolved
Hide resolved
@@ -70,8 +95,15 @@ String Morning_Message { channel="chatgpt:account:1:morningMessage" } | |||
|
|||
Number Temperature_Forecast_Low | |||
Number Temperature_Forecast_High | |||
Dimmer Kitchen_Dimmer "Kitchen main light" [ "OpenAI" ] | |||
String LivingRoom_AC_Mode "Thermostat mode in the living room" [ "OpenAI" ] {channel="", am="OFF, HEAT, AUTO, COOL, FAN, DRY" } |
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.
String LivingRoom_AC_Mode "Thermostat mode in the living room" [ "OpenAI" ] {channel="", am="OFF, HEAT, AUTO, COOL, FAN, DRY" } | |
String LivingRoom_AC_Mode "Thermostat mode in the living room" [ "OpenAI" ] {am="OFF, HEAT, AUTO, COOL, FAN, DRY" } |
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.
Sorry, I still don't understand what the "am" metadata namespace is about. What you need here are simply command options, which is already possible to set in the Main UI for your items (and it should be possible to add them textually with the "commandDescription" namespace as well).
The nice thing is that these options are already provided from bindings for certain channels, so the users might not even have to define it themselves.
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.
I'm not using the main UI at all; everything is in files, and I have never heard of the 'commandDescription' namespace either. I will then rewrite this part.
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.
Fixed. Should I describe this in the README, or does it go without saying? Something like: "If your item accepts special commands and they are not provided by the binding, you need to set them in the main UI or add the 'commandDescription' namespace to the item."
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.
I would write:
„An Item’s command options are used to determine available commands. If your Item accepts special commands, but the binding does not provide command options, you can use the command description metadata.“
...inding.chatgpt/src/main/java/org/openhab/binding/chatgpt/internal/hli/ChatGPTHLIService.java
Outdated
Show resolved
Hide resolved
…thing/thing-types.xml Co-authored-by: Kai Kreuzer <[email protected]> Signed-off-by: Artur-Fedjukevits <[email protected]>
…thing/thing-types.xml Co-authored-by: Kai Kreuzer <[email protected]> Signed-off-by: Artur-Fedjukevits <[email protected]>
…thing/thing-types.xml Co-authored-by: Kai Kreuzer <[email protected]> Signed-off-by: Artur-Fedjukevits <[email protected]>
Signed-off-by: Artur-Fedjukevits <[email protected]>
Signed-off-by: Artur-Fedjukevits <[email protected]>
Signed-off-by: Artur-Fedjukevits <[email protected]>
Signed-off-by: Artur-Fedjukevits <[email protected]>
fixed |
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.
Thanks @Artur-Fedjukevits for your updates and your patience with the review process. I think the result is great and we are good to merge!
Just one last tiny change request where the code does not yet meet the documentation.
bundles/org.openhab.binding.chatgpt/src/main/resources/OH-INF/thing/thing-types.xml
Outdated
Show resolved
Hide resolved
...binding.chatgpt/src/main/java/org/openhab/binding/chatgpt/internal/ChatGPTConfiguration.java
Outdated
Show resolved
Hide resolved
…thing/thing-types.xml Signed-off-by: Kai Kreuzer <[email protected]>
…binding/chatgpt/internal/ChatGPTConfiguration.java Signed-off-by: Kai Kreuzer <[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.
I've done the small change myself, so that it can be merged and included in tomorrow's milestone.
Nice! Thanks! |
This is a new PR because the old one was broken due to incorrect rebase. I hope I have taken all the comments into account. @lsiepel and @kaikreuzer welcome to the review.