[voice] LLMItemSerializer: Get semantic relationships from metadata & Include semantic children of non-semantic groups - #5758
Conversation
…ta, only falling back to group membership Signed-off-by: Florian Hotze <dev@florianhotze.com>
…c groups between semantic and non-semantic items Signed-off-by: Florian Hotze <dev@florianhotze.com>
Signed-off-by: Florian Hotze <dev@florianhotze.com>
florian-h05
changed the title
wborn
left a comment
wborn
left a comment
There was a problem hiding this comment.
This PR was reviewed by AI first; a manual maintainer review will still follow.
The overall direction makes sense. Using the semantics metadata as the source for semantic relationships avoids duplicating the hierarchy rules in LLMItemSerializer, and including semantic Items below non-semantic Groups is useful for the LLM context.
There are still a few issues that should be addressed:
- Root semantic Items that are only members of non-semantic Groups are no longer emitted in the semantic section. They should remain semantic roots and additionally be included below the non-semantic Group.
- The new recursive traversal of non-semantic Groups has no protection against recursive group membership, which can result in unbounded recursion.
org.openhab.core.voicenow depends on constants fromorg.openhab.core.semantics.internal. The semantics metadata contract used across bundles should be exposed from a non-internal API rather than coupling the voice bundle to an implementation class.
The added tests cover the main metadata-based hierarchy and an Item that occurs in both sections, but test cases for a root semantic Item inside a non-semantic Group and recursive group membership would help cover the two functional cases above.
| if (parentName != null) { | ||
| for (String parentName : parentNames) { | ||
| parentToChildren.computeIfAbsent(parentName, k -> new ArrayList<>()).add(child); | ||
| childNames.add(child.getName()); |
There was a problem hiding this comment.
childNames now combines two different kinds of relationships: semantic parent relationships from the metadata and membership in non-semantic Groups. This causes a semantic Item to stop being a semantic root merely because it belongs to a non-semantic Group.
For example, if a semantic Point has no hasLocation/isPointOf relationship but is a member of gSensors, it is added to childNames. It is therefore omitted from rootPoints and only appears below gSensors in the non-semantic section, where its semantic type and properties are no longer represented.
The existing test covers an Item that has both a semantic parent and a non-semantic parent, so it still remains reachable through the semantic tree.
Could we track semantic-parent relationships separately from non-semantic Group relationships so that root detection for semantic Items only considers semantic parents? It would also be good to add a test for a root semantic Item that belongs to a non-semantic Group.
There was a problem hiding this comment.
Will split that 👍
| List<NonSemanticItemNode> childNodes = new ArrayList<>(); | ||
|
|
||
| for (Item child : children) { | ||
| childNodes.add(buildNonSemanticNode(child, parentToChildren, locale)); |
There was a problem hiding this comment.
This recursive traversal needs protection against recursive Group membership.
Recursive Groups can exist in the Item model, and other core code such as SemanticsMetadataProvider and EnrichedItemDTOMapper explicitly tracks ancestors to avoid a StackOverflowError. With a hierarchy such as A -> B -> C -> B, where A is a root Group, this method will keep recursively building B -> C -> B -> ....
Could we carry an ancestor/visited set through buildNonSemanticNode, skip a child that is already an ancestor, and add a corresponding regression test?
There was a problem hiding this comment.
Is this an issue? The SemanticsMetadataProvider would not allows this recursive structure to be created in the first place. The semantics namespace is also protected from creating anything directly, it can only be done through item tags.
I see, it is because non-semantic nodes are in this as well.
There was a problem hiding this comment.
Will address this 👍
| */ | ||
| package org.openhab.core.voice.text.interpreter.llm; | ||
|
|
||
| import static org.openhab.core.semantics.internal.SemanticsMetadataProvider.NAMESPACE; |
There was a problem hiding this comment.
Could we avoid depending on SemanticsMetadataProvider here? It is in org.openhab.core.semantics.internal, so its constants are implementation details of the semantics bundle rather than API for other bundles.
Since the metadata namespace and relation names are now intentionally consumed by another bundle, they should probably be exposed from a suitable class in the public org.openhab.core.semantics package, or the relationship lookup itself could be exposed through the semantics API. SemanticsMetadataProvider and LLMItemSerializer could then both use that public contract.
Signed-off-by: Florian Hotze <dev@florianhotze.com>
Signed-off-by: Florian Hotze <dev@florianhotze.com>
3 participants
SemanticsMetadataProvideris single source of truth for building the model).