-
-
Notifications
You must be signed in to change notification settings - Fork 428
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
Add dynamic creation of semantic tags #3519
Conversation
6dc968e
to
c5b9bab
Compare
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/openhab-4-0-wishlist/142388/455 |
Does this break HABot? That's where the semantic model came from in the first place. |
This merely lets you add more tags, not delete/alter existing ones. I don't see how it would break anything, although to be sure it would need to be tested by someone familiar with habot. |
My concern with HABot is it actually uses the semantic model to parse arbitrary text to understand the intent. Each semantic tag has a specific meaning to HABot. My worry is that arbitrarily adding them could potentially confuse HABot's human language processor if the new tags are not properly integrated into the ontology. |
While I haven't looked closely into HABot:
I can understand that adding new tags to openhab-core requires a serious consideration because it will impact everyone. That's why it's so difficult so far. That's the beauty of this. What you add, only affects you, and you can easily undo it by restarting openhab. It is not persistent. You'll have to run a rule on OH start up every time to add your set of tags. |
Only if adding the tags actually works and doesn't break it. It would even be OK if the added tags are simply ignored by HABot. But based on past experiences working with ontologies (granted that was a decade ago), it's almost never as simple as just adding a new tag and away we go.
I don't think it's right to add a feature that can render a preexisting feature inoperable with the only mitigation being "well, you should have been more careful". And while breaking HABot is of zero concern to you, it's not to me and everyone else who uses HABot (see https://community.openhab.org/t/self-hosted-notifications-with-nextcloud-talk/144090 for just one use case). "I want it really badly" isn't justification for breaking things IMO. @ghys, I hate to tag you but you are the resident expert on the ontology and developer of HABot. Are my concerns out of line and there really is no issue here, or does this have the potential to break HABot as I fear? |
OTOH, using "it'll break something" can't be the reason for not going forward. It seems that HABot is not maintained anymore, so it should not be the reason to stop development in other areas. That aside: I don't care at all, I don't use semantic tags. |
While I understand your concern regarding its potential impact to habot, I don't understand why it shouldn't be added because of it. This doesn't do anything unless deliberately used. I am sure there are many other things that fall in the similar category. One that instantly came to mind is On the contrary, those who want to add additional tags want to do so because they care about organisation and structure. This is not to say that those who don't, don't care about organisation. |
I'm not saying not to develop this. I would like to be able to add new semantic tags too. What I am saying is let's make sure that we understand the impacts to other parts of OH and mitigate those impacts as much as possible. "I don't use it so who cares" is a pretty hostile attitude towards the end users. That's in fact the exact stated reason for why a number of OH users abandoned HomeAssistant in favor of OH. Things break without warning because the developers don't care how it impacts the end users. I'd hate to see us heading in the same direction. And if this does break HABot and if HABot is truly abandoned than let's just remove it (which would be a shame because one of the most magical things with OH is the ability to tell it to do stuff using natural language) rather than setting up a bunch of land mines for end users to find which blows it up. And this one will be found right away because those who use HABot are definitely going to want to use these new custom defined tags too. Again, to summarize, I'm not saying "no don't do it!" I'm not even saying "no don't do it if it breaks HABot!" I am saying lets spend maybe just a couple of minutes or so to find out how it impacts HABot and if it does, how to mitigate any problems it may cause so end users don't just wake up one day and find HABot doesn't work because they added a custom semantic tag. I really don't think that's too much to ask. |
Thanks for clarifying. Unfortunately I am not familiar with habot. The last time I "played" with it was uhm... when I first came across openhab in 2017-18 maybe. Waiting to hear from @ghys but would you also be able to test? |
@rkoshak You should know that I always try to keep things as backward compatible as possible and to have smooth migration path when this is not possible. "Thing that break without warning" is really something that should be avoided and I don't think it is common in openHAB to have that. We have made a big step forward in avoiding such situations with #3330, especially for UI users. What I wanted to express is: I don't use HABot and I don't use semantic tags. I have no preference for "not extending tags in favor of HABot" or "extending tags and dropping HABot". Regarding the maintenance state: HABot can't be build on current Apple machines since more than two years because of outdated NodeJS (which also brings a ton of outdated dependencies). |
@rkoshak I was just thinking, is there a similar concern each time a new tag is added, or removed from the official list of tags in core? If so, is there a process in place to verify / test / check against habot? Perhaps we can utilise the same process. |
Yes as long as I don't have to build. I don't have a dev environment and I understand it's a heavy lift to do so. But the test should be relatively simple I would think:
Obviously watch the logs for errors. I think that would show if ti breaks and whether the new tag is available to HABot (which is less of a concern to me than steps 1-5 failing for some reason).
I know you do and greatly appreciate that. It just seemed in this particular case that breaking HABot didn't rise to a level where it was even considered. I am happy that isn't the case.
I'd really hate to lose it but if it's slowly dying out maybe now's the time to just remove it.
That's a really good question and I don't know the answer to it. I'll see if I can dig up an old PR that added a new tag. It's been a good while since the last tag was added but I seem to recall that it was more than just adding the tag. |
TestingThanks for wanting to test.
Then proceed with your tests that you've outlined. In order to add a tag, use jsscripting or jruby. I'm not familiar with how to do this in jsscripting, so here's a jruby version:
Existing parent can be It won't let you use an invalid parent tag nor an invalid tag name, etc. The only way to remove the custom tags is by making sure your script doesn't run (e.g. comment them out) then restart openhab.
I think HABot is really cool. I just really haven't had the "time" to play with it / use it. It would be a shame to kill it. It probably just needs some TLC? |
Given the name I'm guess that you are working with the actual Java Class here? In that case it's pretty straight forward. I just need to use
Test results: The timing of the restart was a little off but it makes sense because we didn't clear the whole cache. But I saw a bunch of errors from Zigbee and my rules which I usually see in cases where there was a cache clearing so I restarted again to get a clean start before running the steps.
This failed with:
I changed the parent tag to something more generic, "Property" and got the same error. I tried the simpler call and this too failed with the NoClassDefFoundError. So I'm stuck at step 2 until I get more direction. The good news is the |
@rkoshak Thanks for testing. Please try this updated jar (also just .zip ext added, not actually zipped) The cache + tmp needs to be cleared again after replacing the jar. |
OK, let the testing resume! I installed the new jar file pre instructions.
In summary, merely creating the custom tags does not break HABot. However, it appears that more needs to be done to make HABot aware of custom Property tags. I didn't test Equipment nor Location yet but given that the custom Point worked, I expect those will work too. I'll test that though as well as testing what happens if I use something like "Light" as the parent Point tag. |
Signed-off-by: Jimmy Tanagra <[email protected]>
Neither "Foobar" nor "foobar" worked but "foo bar" did work. So it uses camelcase to determine where spaces should go? This is kind of exciting! It looks like it might work after all, but we will have to document pretty closely how it works when using camelcase like that.
Yes, all the built in tags work. But you are right, the tag is "LivingRoom" but in the query I have to use "living room". Interestingly it doesn't seem to use the Item label in the query. The Item I have tagged with "LivingRoom" is labeled "Family Room" and HABot can't find it. But that's a concern for a different issue. It just cost me a bit of time thinking it wasn't working when it was and my query was wrong. I created a single word "Dog" tag with "Property" as the parent and applied that to my test Item and "show the dog in the living room" worked. So maybe it is working after all and the problem was I used a multiword tag and didn't know it. Question, I've made a couple of assumptions:
1 is understandable at the current state of the PR. If my assumption is wrong, as it appears to be, 2 needs to throw an exception (similar to how it does when trying to use a tag that doesn't start with a capital) or at least a warning log statement if it's really just ignoring the change. If my assumption for 2 was correct, after changing the "Dog" tag to inherit from "Light" I was unable to query for and get it to show up along side all the other Items tagged with "Light". However, looking at the Item seems to indicate that it never changed: The class still shows Property_Dog, not Light_Dog. Creating a new "Cat" tag that inherits from Light shows: And indeed queries for lights now show the test Item as expected. Tl;dr, it seems all the problems I've encountered are my own and not the code. This is awesome! |
At its current incarnation, you cannot re-add an existing tag. It checks against existing tags (built-in or otherwise) and will simply return null and do nothing. It is documented that it returns null in this case, but I see that a warning logger is probably needed as well. CamelCase tagname generates Camel Case label, IF you haven't specified a label when calling add, i.e. just using two arguments to add. This is also documented. |
@rkoshak I go back and forth about this, but I prefer not to issue a warning log. Instead let the caller handle it. The method will return null to indicate this, so if the caller wants to issue warning they can do so themselves. This gives people the freedom to choose the behaviour rather than forcing it on them.
|
Just to be clear though, by documented I mean the behavior needs to be described in the OH docs, not just in the code. End users are not going to go read comments in the code nor refer to the JavaDocs for the most part. Because this is a completely new capability from the end user's perspective, nothing about creating new semantic tags appears anywhere in the user guide. It probably also makes sense that the code that adds/removes?/modifies? semantic tags be exposed as an Action if we are thinking this is something a user may want to do from a rule. I'm thinking that is probably not the best idea and tags should only really be manipulated through the REST API/UI/Karaf Console, but I don't always see all the use cases.
As long as the caller isn't a rule I think I'm OK with that, so long as it's clearly documented at the level of the caller (e.g. if at the REST API it should be clear in the swagger docs what indicates success, ignored, or problem). |
I agree fully with this. The request to let users expand the semantic model seems to come every other year or so, including when the feature was discussed at the time when it was merely in the plans (eclipse-archived/smarthome#1093) and had yet to be implemented - until now I guess. If HABot hasn't received much attention, this isn't because it's not maintained anymore, it's rather because "it works" and I moved on to other things - but I would be sad if it broke for no good reason. As the author I'll admit I rarely use the dedicated HABot UI these days but I still regularly use the text box on the main UI home page, mainly to recall controls from keywords ("kettle", "kitchen lights"...) that I haven't properly put on a page so they're accessible easily, and would be 3 to 4 clicks away if browsing through the semantics cards on the other tabs of the home page. HABot is kind of test-driven, there are extensive tests to exercise the bundled skills in every language HABot supports - you can see the output about 3/4 of the way in the build logs (https://ci.openhab.org/job/openHAB-WebUI/611/consoleText)
If those didn't pass, for any reason, then the web UI builds would fail. I would expect any author of a change that has an impact in this area to own up to the regression they introduced and strive to fix it themselves ;) |
And I'm grateful to @rkoshak for doing all this testing! Having some more Properties added could indeed cause HABot to behave slightly differently (because its training would be different) and could have an impact on the tests, this would be solved with some more training material. |
I've still a bunch of tests to do but it does appear that my initial assessment was caused by using "FooBar" as the Property tag and querying HABot for "foobar" (single word). Once I got that working HABot was able to see and return the Item and control it based on the "foo bar" tag. And it worked with "dog" and "cat" too as single word tags. I made "cat" have "light" as the parent and HABot even recognized "cat" as a light which was really cool. It seems like HABot might be treating these new tags as synonyms perhaps? It would be interesting to custom build a whole hierarchy and see how far it goes. If I add a tag that has "cat" as a parent, will that tag be recognized as a "cat" and a "light"? |
It's hard to say, but there is some code to feed any "attributes" available (meaning, tags & synonyms including those by the user) to the trainer to improve the recognition of those entities. If OpenNLP is involved at all (remember, it isn't if you just state the label of an item in which case it will merely return a card with the item's members if it's a Group), then any deviation in the training might lead to different results. The outcome of these tests can be relied upon because the set of training is predetermined, but in practice it isn't because of the code above. HABot is no ChatGPT, of course, but the basic principle remains the same: the more you train it, the better it becomes. |
@jimtng Did you check if web still builds with this changes? I can't test that because as I said above: HABot can't be build on Apple Silicon processors. @rkoshak @ghys If the build is ok, is there anything that prevents this from being reviewed and merged? |
I'm good. There are still some tests I want to run just to find the limits of what works and what doesn't but I've been able to confirm:
There should be no surprises for the end users unless they really go deep and create some big new hierarchy of tags (I still don't know what will happen in that case). So as far as I'm concerned it's all good and I'm looking forward to adding some custom tags. :-D |
I'm not sure how to do this, but I've built the entire openhab-core repo and openhab-webui repo on linux on the same machine but I didn't combine them into a running instance. Does that count, or is there a special step to verify? I've never done a full distro build. |
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
That should work. |
Why do we need a script/rule to extend semantics? Until now, everything was doable through REST APIs. Why not in this case? The choice looks strange and unusable. |
This API is needed as the primary mechanism to create the tags. Using a script is the easiest method to invoke this API. REST interface to create the tag can be added later on + its corresponding MainUI user interface. Hence the two additional PRs linked to this to lay the ground work towards that: Once those two are merged, then we could add the support to create the tags from the UI. |
What we need is a REST API to add/remove semantics tags. They need to be stored somewhere to survive a OH restart. |
Yes, I understand and agree. There needs to be a semantic tag provider. Removing semantic tags without restarting openhab would be great too. |
) Supersedes #1850. Closes #1822. Depends on openhab/openhab-core#3559 (already merged now). Adding custom semantic tags is now possible: openhab/openhab-core#3519. This PR loads the Semantic tags when MainUI is loaded the first time and stores them in Vuex. This allows the removal of the hard-coded Semantic tags and the translations from the assets and therefore makes the initial JS smaller. -- Signed-off-by: Florian Hotze <[email protected]>
Signed-off-by: Jimmy Tanagra <[email protected]> GitOrigin-RevId: 4f2af88
This pull request has been mentioned on openHAB Community. There might be relevant details there: |
Add the ability to dynamically add new semantic tags
For example, to add a new tag called
MainBedroom
withBedroom
as the parent:SemanticTags.add("MainBedroom", Bedroom.class)
orSemanticTags.add("MainBedroom", "Bedroom")
The new tags can then be used in items and rules.
A text file in the following format can be easily loaded and parsed with a simple rule on startup in any scripting language.
or go full YAML... the syntax is up to the rule writer.
Currently the MainUI can recognise and display the added tags:
However, because MainUI uses a static list of tags (loaded from a file), it doesn't list the new tags in the tag selection dialog. More work is needed to provide a REST end point which the MainUI can then query rather than having to duplicate the list of tags in a static file like it currently does.