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

[ChatGPT] Enhance binding #17320

Merged
merged 70 commits into from
Nov 9, 2024
Merged

[ChatGPT] Enhance binding #17320

merged 70 commits into from
Nov 9, 2024

Conversation

Artur-Fedjukevits
Copy link
Contributor

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.

@Artur-Fedjukevits
Copy link
Contributor Author

Here is jar for testing.

@lsiepel lsiepel added the enhancement An enhancement or new feature for an existing add-on label Aug 29, 2024
@Artur-Fedjukevits Artur-Fedjukevits requested a review from a team as a code owner September 6, 2024 16:30
@Artur-Fedjukevits
Copy link
Contributor Author

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...

@kaikreuzer
Copy link
Member

@Artur-Fedjukevits You should always use feature branches and never work on main.
You can have a branch per PR, so that you can work on them independently. Check out something like https://www.split.io/blog/understanding-the-feature-branching-strategy-in-git/ if this is new to you. 😄

<default>1</default>
<advanced>true</advanced>
</parameter>
<parameter name="type" type="text">
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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.

@Artur-Fedjukevits
Copy link
Contributor Author

@Artur-Fedjukevits You should always use feature branches and never work on main. You can have a branch per PR, so that you can work on them independently. Check out something like https://www.split.io/blog/understanding-the-feature-branching-strategy-in-git/ if this is new to you. 😄

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]>
@Artur-Fedjukevits
Copy link
Contributor Author

Hi @kaikreuzer. Do you have time for review this PR?

@kaikreuzer
Copy link
Member

Still on my list, sorry for taking so long - it is not forgotten!

@ThaDaVos
Copy link

Question out of curiosity - but is this based on the Assistants API/Functionality of OpenAI?
https://platform.openai.com/docs/assistants/overview

@Artur-Fedjukevits
Copy link
Contributor Author

Artur-Fedjukevits commented Oct 13, 2024

Question out of curiosity - but is this based on the Assistants API/Functionality of OpenAI? https://platform.openai.com/docs/assistants/overview

It based on function calling via Chat Completions API

Copy link
Member

@kaikreuzer kaikreuzer left a 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.

bundles/org.openhab.binding.chatgpt/README.md Outdated Show resolved Hide resolved
bundles/org.openhab.binding.chatgpt/README.md Outdated Show resolved Hide resolved
bundles/org.openhab.binding.chatgpt/README.md Outdated Show resolved Hide resolved
bundles/org.openhab.binding.chatgpt/README.md Outdated Show resolved Hide resolved
bundles/org.openhab.binding.chatgpt/README.md Outdated Show resolved Hide resolved
<default>1</default>
<advanced>true</advanced>
</parameter>
<parameter name="type" type="text">
Copy link
Member

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.

@ThaDaVos
Copy link

ThaDaVos commented Oct 16, 2024

Question out of curiosity - but is this based on the Assistants API/Functionality of OpenAI? https://platform.openai.com/docs/assistants/overview

It based on function calling via Chat Completions API

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!

Artur-Fedjukevits and others added 8 commits October 21, 2024 12:42
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]>
Artur-Fedjukevits and others added 8 commits October 21, 2024 17:15
…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]>
@Artur-Fedjukevits
Copy link
Contributor Author

Artur-Fedjukevits commented Oct 21, 2024

Thanks @kaikreuzer!

I hope I’ve taken all your comments into account and fixed everything. I’m looking forward to your next comments :)
I'm sorry for making so many mistakes :(

@florian-h05
Copy link
Contributor

@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 🚀
Just FYI: This binding also works with Ollama's REST API, so I can play around with it offline on my laptop with a Llama 3.2 3B.

@lsiepel
Copy link
Contributor

lsiepel commented Nov 1, 2024

@Artur-Fedjukevits can you fix the merge conflict for the readme?

Copy link
Member

@kaikreuzer kaikreuzer left a 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.

@@ -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" }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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" }

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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."

Copy link
Contributor

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.“

Artur-Fedjukevits and others added 8 commits November 3, 2024 19:47
…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]>
@Artur-Fedjukevits
Copy link
Contributor Author

@Artur-Fedjukevits can you fix the merge conflict for the readme?

fixed

Copy link
Member

@kaikreuzer kaikreuzer left a 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.

…binding/chatgpt/internal/ChatGPTConfiguration.java

Signed-off-by: Kai Kreuzer <[email protected]>
Copy link
Member

@kaikreuzer kaikreuzer left a 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.

@Artur-Fedjukevits
Copy link
Contributor Author

Nice! Thanks!

@kaikreuzer kaikreuzer merged commit 5929ef8 into openhab:main Nov 9, 2024
5 checks passed
@kaikreuzer kaikreuzer added this to the 4.3 milestone Nov 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement or new feature for an existing add-on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants