-
-
Notifications
You must be signed in to change notification settings - Fork 33.1k
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
Weather forecast llm tool #137314
base: dev
Are you sure you want to change the base?
Weather forecast llm tool #137314
Changes from all commits
0733459
e7cb0e4
4bceec7
dcc3439
f94de77
d8249d6
25acaca
3587854
127d7bb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,10 +24,17 @@ | |
from homeassistant.components.homeassistant import async_should_expose | ||
from homeassistant.components.intent import async_device_supports_timers | ||
from homeassistant.components.script import DOMAIN as SCRIPT_DOMAIN | ||
from homeassistant.components.weather import INTENT_GET_WEATHER | ||
from homeassistant.components.weather import ( | ||
DOMAIN as WEATHER_DOMAIN, | ||
INTENT_GET_WEATHER, | ||
SERVICE_GET_FORECASTS, | ||
) | ||
from homeassistant.const import ( | ||
ATTR_DOMAIN, | ||
ATTR_ENTITY_ID, | ||
ATTR_NAME, | ||
ATTR_SERVICE, | ||
ENTITY_MATCH_ALL, | ||
EVENT_HOMEASSISTANT_CLOSE, | ||
EVENT_SERVICE_REMOVED, | ||
) | ||
|
@@ -443,6 +450,9 @@ | |
for intent_handler in intent_handlers | ||
] | ||
|
||
if exposed_domains and WEATHER_DOMAIN in exposed_domains: | ||
tools.append(WeatherForecastTool()) | ||
|
||
if exposed_entities: | ||
if exposed_entities[CALENDAR_DOMAIN]: | ||
names = [] | ||
|
@@ -745,7 +755,7 @@ | |
"""Init the class.""" | ||
self._domain = domain | ||
self._action = action | ||
self.name = f"{domain}.{action}" | ||
self.name = f"{domain}_{action}" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I found out that some models are not happy with dots in the tool name There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we move this to its own PR |
||
self.description, self.parameters = _get_cached_action_parameters( | ||
hass, domain, action | ||
) | ||
|
@@ -884,3 +894,48 @@ | |
] | ||
|
||
return {"success": True, "result": events} | ||
|
||
|
||
class WeatherForecastTool(Tool): | ||
"""LLM Tool wrapper for weather forecast action.""" | ||
|
||
name = f"{WEATHER_DOMAIN}_{SERVICE_GET_FORECASTS}" | ||
description = "Get weather forecasts" | ||
parameters = vol.Schema( | ||
{ | ||
vol.Required("type"): vol.In(("daily", "hourly", "twice_daily")), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why would we include twice daily? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To match the service capabilities. I can remove it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah, we shouldn't match the services as-is. I've noticed with the event tool that LLms are very easily confused. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I also wonder if we should follow similar ranges like we do for events. |
||
vol.Optional(ATTR_NAME, description="Weather entity name"): cv.string, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I noticed that for calendar, just limiting this to the actual allowed weather values gives a lot better results. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This makes sense, smart homes don't usually have too many weather entities. I will do it. |
||
} | ||
) | ||
|
||
async def async_call( | ||
self, hass: HomeAssistant, tool_input: ToolInput, llm_context: LLMContext | ||
) -> JsonObjectType: | ||
"""Get the forecast.""" | ||
data = self.parameters(tool_input.tool_args) | ||
if ATTR_NAME in data: | ||
result = intent.async_match_targets( | ||
hass, | ||
intent.MatchTargetsConstraints( | ||
name=data[ATTR_NAME], | ||
domains=[WEATHER_DOMAIN], | ||
assistant=llm_context.assistant, | ||
), | ||
) | ||
if not result.is_match: | ||
return {"success": False, "error": "Weather entity not found"} | ||
data.pop(ATTR_NAME) | ||
data[ATTR_ENTITY_ID] = [state.entity_id for state in result.states] | ||
else: | ||
data[ATTR_ENTITY_ID] = ENTITY_MATCH_ALL | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think that we should do this. It's unpredictable. |
||
|
||
service_result = await hass.services.async_call( | ||
WEATHER_DOMAIN, | ||
SERVICE_GET_FORECASTS, | ||
data, | ||
context=llm_context.context, | ||
blocking=True, | ||
return_response=True, | ||
) | ||
|
||
return {"success": True, "result": service_result} |
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.
Make sure that Weather is no longer included in
_get_exposed_entities
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.
The weather entities report the current weather, and the tool reports the forecast, I think we need both
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.
We could change that though, include "current weather" in the tool as well. That way we don't always include all that info.