Restore_state helper to restore entity states from the DB on startup#4614
Conversation
|
Some quick comments:
|
|
Wondering if this can be expanded to support persistence in general. For example, I use |
|
@arsaboo Yes, in it's current for that should work for persistence. I simply used the device_tracker as an example, but also have input_selects and input_booleans that would benefit from this. What should we call this component? |
|
@kellerza |
|
Let's be a bit more specific to what it restores. What about |
|
I would love to have the climate target temperature to be restored from DB as well. |
kellerza
changed the title
|
To be continued... (after 18 Dec) |
There was a problem hiding this comment.
I think that this method should live in history so that history can be the API on top of the recorder ?
There was a problem hiding this comment.
history makes sense, was just concerned about it also requiring http, but it will make things cleaner
There was a problem hiding this comment.
Why do you need to query it again? get_states will already return the state at that point in time.
You should however make sure that the state not already exists. We don't want to overwrite legitimate states. We should also make sure that you add some attribute to indicate that this is a restored state, otherwise we won't be able to show a dismiss button in the UI.
cur_state = hass.states.get(entity_id)
if cur_state is not None:
continue
attributes = dict(state.attributes)
attributes[ATTR_RESTORED_STATE] = True
hass.states.set(entity_id, state.state, attributes)There was a problem hiding this comment.
Agreed, will add the attribute. Actually I have not managed to get past line 60 (with the comment), so this second loading is still an artefact from the previous recorder version.
Once I get my unit test's test data to be returned by get_states I'll continue with this part. Had some challenges to mock recorder.models.dayetime.utcnow Since we never import models directly and datetime also a challenge
There was a problem hiding this comment.
Examples of not mocking time of models but cant find the DEBUG output from line 60 in Travis. I do see some SQLAlchemy exceptions, which wasnt so clear on my machine.... obviously still needs some work to add state entries before HASS start and ensure recorder starts up correctly during HASS start
There was a problem hiding this comment.
Besides being unnecessary (see my other comment), this method is pretty much covered by this method
There was a problem hiding this comment.
The only difference is this method ignores unknown states, but reusing the old also cleaner since the possibility of unknown in the DB should be less toward the end of a session
There was a problem hiding this comment.
Let's keep any integration into other components out of the initial release. Let's really focus on the core support.
Because if we do it right, components/platforms don't have to restore anything. They can just not set a state and then restoration will do it instead .
|
Seems like I created a flaky test here. Py34 passing, py35 not... Seem that my HOME_ASSISTANT_START event is not always fired. Any suggestions would be appreciated |
|
What's the statu of this @kellerza ? |
|
I really look forward to use this component. As for the name I think restore_states or state_restore would be more appropriate as it restores states of the entities, not the entities themselves :). |
|
Will look into this again. Was struggling with temperamental unit tests and will give it another shot this weekend. Will squash all commits on rebase, since there are some conflicts. I like |
|
Just adding some positive support for this component. I'm really looking forward to the usability increase this will bring to HA. |
|
Latest update:
Will try to spend more time on these flaky tests tomorrow |
|
Excited about this as well. I'll ask a dumb question as to if I can try this now? What does it take to add this to an existing install? |
|
At the moment this works well for For
I expect for EDIT: Added option 3, which is better for linting etc, but create lots of dependencies for restore_state |
|
If the recorder connection won't be initialized until Home Assistant starts, how will we be able to query for the states? |
|
The recorder init changed so that we can query states before start - #4614 (comment) |
| vol.Optional(CONF_ENTITIES, default=[]): cv.entity_ids, | ||
| vol.Optional(CONF_DOMAINS, default=[]): | ||
| vol.All(cv.ensure_list, [cv.string]) | ||
| }) |
There was a problem hiding this comment.
continuation line missing indentation or outdented
| vol.Optional(CONF_INCLUDE, default={}): vol.Schema({ | ||
| vol.Optional(CONF_ENTITIES, default=[]): cv.entity_ids, | ||
| vol.Optional(CONF_DOMAINS, default=[]): | ||
| vol.All(cv.ensure_list, [cv.string]) |
There was a problem hiding this comment.
continuation line unaligned for hanging indent
| vol.Optional(CONF_DOMAINS, default=[]): | ||
| vol.All(cv.ensure_list, [cv.string]) | ||
| }), | ||
| vol.Optional(CONF_INCLUDE, default={}): vol.Schema({ |
There was a problem hiding this comment.
continuation line unaligned for hanging indent
balloob
left a comment
balloob
left a comment
There was a problem hiding this comment.
So close! I had a few minor comments but the overall structure is awesome.
| rec_runs = recorder.get_model('RecorderRuns') | ||
| with recorder.session_scope() as session: | ||
| res = recorder.query(rec_runs).order_by(rec_runs.end.desc()).first() | ||
| session.expunge(res) |
There was a problem hiding this comment.
first() can return None which will blow up when passed to session.expunge
| "in %s seconds)", err, CONNECT_RETRY_WAIT) | ||
| time.sleep(CONNECT_RETRY_WAIT) | ||
|
|
||
| load_restore_cache(self.hass) |
There was a problem hiding this comment.
We should remove this here and instead just initialize it the first time get_last_state is called. Cache should only be initialized if hass.state == CoreState.starting (CoreState can be imported from homeassistant.core)
There was a problem hiding this comment.
Can be done, I've got a new unit test (should be non-flakey) where I discovered I'll have to yield get_last_state in any case. On startup many components will potentially wait for this event, which hopefully won't affect order of adding to hass. At least by the time this is called "entities" was already updated in entity_component
There was a problem hiding this comment.
FYI For now, we set up all components and platforms in sequence.
| def get_last_state(entity: Entity, check_async_restore_state: bool=True): | ||
| """Helper to restore state.""" | ||
| recorder.get_instance() | ||
| if check_async_restore_state and \ |
There was a problem hiding this comment.
We should remove this check from this method. It is not a concern from the recorder what this is being used for.
| def async_added_to_hass(self): | ||
| """Component added, restore_state using platforms.""" | ||
| state = get_last_state(self) | ||
| if state: |
There was a problem hiding this comment.
Can you convert this to a guard clause? This example is going to be copied by a lot of other component/platform developers and thus needs to be perfect 🥇
if state is None:
return
params = …There was a problem hiding this comment.
Your view on moving this to homeassistant.helpers.restore_state?
Two reasons:
recorderdoes not explicitly have to request the cache, butget_last_statehandles this.- Many components & platforms (possibly every sensor) will import this "helper"
The cache function can then be a _private
There was a problem hiding this comment.
Let's add another helper to this file that wraps the entity logic around get_state.
def extract_info(state):
"""Restore properties from state object."""
return {
is_on: state.state == STATE_ON,
# etc…
}
class Light:
def async_added_to_hass(self):
yield from restore_state.xxxxx(self, extract_info)And in restore state, the helper xxxxx does all the logic that was in light before.
def xxxxx(entity, extract_info):
if not hasattr(entity, 'async_restore_state'):
return
state = get_state(entity.entity_id)
if not state:
return
yield from entity.async_restore_state(**extract_info(state))| @asyncio.coroutine | ||
| def async_added_to_hass(self): | ||
| """Component added, restore_state using platforms.""" | ||
| state = get_last_state(self) |
There was a problem hiding this comment.
get_last_state should only take in an entity_id.
if not hasattr(self, 'async_restore_state'):
return
state = get_last_state(self.entity_id)
kellerza
changed the title
balloob
left a comment
balloob
left a comment
There was a problem hiding this comment.
Wooooohooooooooooo 🎉
One minor signature change required. You can merge after the comment has been addressed 🐬
|
|
||
|
|
||
| @asyncio.coroutine | ||
| def async_get_last_state(entity: Entity): |
There was a problem hiding this comment.
The parameters should be async_get_last_state(hass, entity_id)
| if not hasattr(entity, 'async_restore_state'): | ||
| return | ||
|
|
||
| state = yield from async_get_last_state(entity.entity_id) |
There was a problem hiding this comment.
We should pass in hass too.
Interestingly. Here we pass entity_id to the async_get_last_state method but it expects an entity. This didn't get caught by the tests.
|
|
||
| @asyncio.coroutine | ||
| def async_added_to_hass(self): | ||
| """Component added to hass, no platforms, so restore state here.""" |
There was a problem hiding this comment.
This should be async_get_last_state(self.hass, self.entity_id)
|
Fixed my own comments and added some tests |
14 participants
Description:
Add a helper function to restore previous entity states from the database/
recorderwhen Entities are added to HASS.Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.github.io#<home-assistant.github.io PR number goes here>
Example entry for
configuration.yaml(if applicable):Checklist:
If user exposed functionality or configuration variables are added/changed:
If the code does not interact with devices:
toxrun successfully. Your PR cannot be merged unless tests pass