Clean up entity component #11691

balloob · 2018-01-16T08:12:04Z

Description:

First off: I'm sorry, this became quite a lengthy PR. It is meant as a clean up round as preparation for #11533.

Overview of the changes:
The responsibility for managing the lifecycle of an entity was spread out over EntityComponent, EntityPlatform and Entity. This PR brings all the responsibility of managing the lifecycle to EntityPlatform. EntityComponent is now only handling a collection of EntityPlatforms and some helper functions (config, discovery, extract entities from service).

Although I have done my due diligence, this PR might break some components that do weird things and are not covered by tests.

Here are the changes:

Remove entity.async_remove
Add method EntityComponent.async_remove_entity which delegates to correct EntityPlatform.async_remove_entity
Add lifecycle hook Entity.async_will_remove_from_hass for clean up tasks when an entity is removed
Rewrite group to use EntityComponent so lifecycles are getting called
EntityComponent.entities is no longer a dictionary of all entities of that component. Instead it is an iterable of entities. (Most components use EntityComponent.extract_from_service which remains unchanged)
Introduced EntityComponent.get_entity(entity_id) for camera, group, media_player and script. (to work around breaking change in above change)
Raise in EntityPlatform.async_add_entities when entity is None
Remove unused methods Group.start, Group.stop

Breaking Change

EntityComponent.add_entity(entity) and EntityComponent.async_add_entity(entity) have been removed. Use EntityComponent.add_entities([entity]) and EntityComponent.async_add_entities([entity]) instead.

CC @rcloran

Checklist:

If the code does not interact with devices:

Local tests with tox run successfully. Your PR cannot be merged unless tests pass
Tests have been added to verify that the new code works.

MartinHjelmare · 2018-01-16T08:43:04Z

homeassistant/components/group/__init__.py

+        if entity_ids:
+            self.tracking = tuple(ent_id.lower() for ent_id in entity_ids)
+        else:
+            self.tracking = []


Above you create a tuple here you create a list.

MartinHjelmare · 2018-01-16T08:58:51Z

homeassistant/helpers/entity_component.py

+            return
+
+        entity.hass = self.component.hass
+        entity.platform = self


This allows access to the platform from all platform entities but also allows the entities to change attributes of the platform. Powerful but maybe dangerous?

They already have access to hass. But I guess you're right in that it might allow people to directly interact with entities from other platforms.

The reason I wanted to add this is to allow platforms to be able to remove entities which is something @rcloran was looking at in a recent PR.

You could write protect some of the attributes with properties, if we think that's needed.

This is Python, people can work around it. I'm not afraid of them assigning the platform attribute.

Of course, we're all adults. I'm more thinking about mistakes. But I think it's ok like this.

balloob · 2018-01-16T19:45:47Z

We could reinstate Entity.async_remove if we want, but I'm not sure if it's necessary:

def async_remove(self):
    if self.platform is not None:
        self.platform.remove_entity(self.entity_id)
    else:
        self.hass.states.async_remove(self.entity_id)

pvizeli

I would prefere for remove things or handle dynamic platform renew stuff, we should make a mechanic like discovery that allow a component to remove all entity from a platform or only a single entity over our bus without we prefere to store all stuff into hass.data. I like hass.data for handle global things but at the moment hass.data would be use for all things and allow to make bad stuff instead to use cool things like the bus or dispatcher and handling abstraction better.

But I like 99% of that code 😄

pvizeli · 2018-01-17T15:05:39Z

homeassistant/helpers/entity_component.py

        def component_platform_discovered(platform, info):
            """Handle the loading of a platform."""
-            self.hass.async_add_job(
-                self._async_setup_platform(platform, {}, info))
+            yield from self._async_setup_platform(platform, {}, info)


So it look better but I think before it was faster. Ben mean that a coro they wait of a other coro without any reason is slower than only schedule a job.

I think that we're micro-optimizing here and it is costing us meaningful stack traces.

pvizeli · 2018-01-17T15:30:12Z

homeassistant/helpers/entity_component.py

+            return
+
+        entity.hass = self.component.hass
+        entity.platform = self


I think that is not needed or we should not do it for our software designe. We should add the platform to setup_platform(..., platform=None) so we need not store the entities anymore in hass.data for own service handling. At the end, the platform need handling the entities and need access to his platform and not the entity

I don't want to change the method signature of platform setup. That will break all platforms.

One thing we could do (and it feels like a hack), is to make EntityPlatform callable and behave like async_add_devices for async setup.

Current responsibilities:

EntityComponent: responsible for managing platforms, group of all entities in all platforms, discovery, extracting entities from services

EntityPlatform: responsible for managing the entities of a platform and keeping them up to date

Entity: responsible for representing an entity in the state machine

EntityPlatform already knows about it's parent EntityComponent to notify that the group needs updating.

The alternative to the entity not knowing the platform it belongs to is using an event to have the entity signal that something should happen. However, in the case of remove, it feels like we're abusing an event system: it is a call for action instead of something that did happen.

Yes, you are right.

Maybe we find in future a nice way that a platform have access to his platform i.e. for handle services.

pvizeli · 2018-01-17T15:33:55Z

homeassistant/helpers/entity_component.py

+        if hasattr(entity, 'async_will_remove_from_hass'):
+            yield from entity.async_will_remove_from_hass()
+
+        self.component.hass.states.async_remove(entity_id)


I think we should not remove the old async_remove from entity. We can call it on this please instead to make a new hidden function. for async_update and maybe async_added_to_hass is that okay but I prefere (for software designe) a overwritte able exists function on base class that every one can see it and can overwrite it (maybe with pass). The enity.async_remove was a nice overwrite for it self. He add it to state machine and he remove it from statemachine.

The reason I removed Entity.async_remove is because the platform is responsible for managing the entities of a platform. The entity cannot just decide to skip out without notifying the platform.

To help entities be notified of being removed, I've added the async_will_remove_from_hass hook. (similar to async_added_to_hass)

btw, I am not against reinstating Entity.async_remove. It would look like this: #11691 (comment)

Maybe async_remove_from_hass. And we should add this method with pass to entity for the design. I think async_update should be the only function for look at it. This help also on async to can make a flip to other task and speed up the parallel. I don't think we lost some things with make coroutine with pass and it make it easyer to read.

I'm fine with this change with comment above but the #11691 is bad.

I change my mind. I think #11691 is a good solution 👍

pvizeli · 2018-01-20T16:05:52Z

I have leaf only a comment for the design of entity.

Anyway. We need a mechanic like discovery for handling remove stuff in a nice way. A component can discovery platforms and for reload he can remove the platform/entity over the eventbus without store stuff to hass.data.

Later we can make a remove like setup (bootstrap - component) and we can make components they can setup or remove on runtime.

balloob · 2018-01-21T07:28:34Z

I've added Entity.async_remove back and added some tests for it.

balloob · 2018-01-21T07:29:36Z

The problem with a remove a-la discovery is that we don't know where to route it to. I guess we can add a remove_platform(…) as a counter to setup_platform(…). But then again, when would we trigger this?

pvizeli · 2018-01-22T11:17:13Z

We trigger that on EntityComponent like we do now with discovery... So we can call the platform remove of a single entity or all entities.

pvizeli · 2018-01-22T11:24:07Z

homeassistant/helpers/entity.py

    hass = None  # type: Optional[HomeAssistant]

+    # Owning platform instance. Will be set by EntityPlatform
+    platform = None
+


I think we should extend this entity with:

async def async_added_to_hass(self): pass async def async_remove_from_hass(self): pass

I rather have us check if it exists so that we don't need an extra yield? (we can do this in a later PR, will merge now)

I think it make the object more understandable and help to give extra time for loop to handle parallel tasks. Like OS scheduler, more small task are better as long running tasks.

andrey-git · 2018-01-24T18:43:14Z

Marked as a breaking change due to removal of Entity.async_add_entity which might be used by custom components.

balloob · 2018-01-24T22:26:04Z

Good point. Have updated the PR description.

balloob requested a review from a team as a code owner January 16, 2018 08:12

homeassistant added the cla-signed label Jan 16, 2018

MartinHjelmare reviewed Jan 16, 2018

View reviewed changes

homeassistant added the cla-signed label Jan 16, 2018

balloob mentioned this pull request Jan 16, 2018

zha: Add remove service #11683

Merged

9 tasks

andrey-git self-requested a review January 17, 2018 08:57

pvizeli reviewed Jan 17, 2018

View reviewed changes

andrey-git removed their request for review January 17, 2018 19:32

balloob force-pushed the cleanup-entity-component branch from 02a94a1 to 2938dbe Compare January 21, 2018 07:12

homeassistant added the cla-signed label Jan 21, 2018

pvizeli approved these changes Jan 22, 2018

View reviewed changes

pvizeli reviewed Jan 22, 2018

View reviewed changes

balloob added 5 commits January 22, 2018 21:10

Clean up entity component

ef6edb9

Lint

60c260d

List -> Tuple

d6852a8

Add Entity.async_remove back

18b1f32

Unflake setting up group test

dd0fc42

balloob force-pushed the cleanup-entity-component branch from a8eaf11 to dd0fc42 Compare January 23, 2018 05:11

homeassistant added the cla-signed label Jan 23, 2018

balloob merged commit 183e054 into dev Jan 23, 2018

balloob deleted the cleanup-entity-component branch January 23, 2018 06:54

andrey-git added the breaking-change label Jan 24, 2018

balloob mentioned this pull request Jan 26, 2018

0.62 #11921

Merged

This was referenced Jan 28, 2018

Assistants and packages config #11778

Closed

Some of the ZWave entities not showing up in 0.62 #11997

Closed

This was referenced Feb 8, 2018

Weird state reporting ZWave nodes (2 values in the same time) #12239

Closed

0.63 #12267

Merged

home-assistant locked and limited conversation to collaborators May 29, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up entity component #11691

Clean up entity component #11691

balloob commented Jan 16, 2018 •

edited

Loading

MartinHjelmare Jan 16, 2018

MartinHjelmare Jan 16, 2018

balloob Jan 16, 2018

MartinHjelmare Jan 16, 2018

balloob Jan 16, 2018

MartinHjelmare Jan 16, 2018 •

edited

Loading

balloob commented Jan 16, 2018

pvizeli left a comment •

edited

Loading

pvizeli Jan 17, 2018

balloob Jan 18, 2018

pvizeli Jan 17, 2018 •

edited

Loading

balloob Jan 18, 2018

balloob Jan 18, 2018

pvizeli Jan 20, 2018 •

edited

Loading

pvizeli Jan 17, 2018 •

edited

Loading

balloob Jan 18, 2018

balloob Jan 18, 2018

pvizeli Jan 20, 2018 •

edited

Loading

pvizeli Jan 20, 2018

pvizeli commented Jan 20, 2018 •

edited

Loading

balloob commented Jan 21, 2018

balloob commented Jan 21, 2018

pvizeli commented Jan 22, 2018 •

edited

Loading

pvizeli Jan 22, 2018

balloob Jan 22, 2018

pvizeli Jan 23, 2018

andrey-git commented Jan 24, 2018

balloob commented Jan 24, 2018

Clean up entity component #11691

Clean up entity component #11691

Conversation

balloob commented Jan 16, 2018 • edited Loading

Description:

Breaking Change

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MartinHjelmare Jan 16, 2018 • edited Loading

Choose a reason for hiding this comment

balloob commented Jan 16, 2018

pvizeli left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pvizeli Jan 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pvizeli Jan 20, 2018 • edited Loading

Choose a reason for hiding this comment

pvizeli Jan 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pvizeli Jan 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pvizeli commented Jan 20, 2018 • edited Loading

balloob commented Jan 21, 2018

balloob commented Jan 21, 2018

pvizeli commented Jan 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrey-git commented Jan 24, 2018

balloob commented Jan 24, 2018

balloob commented Jan 16, 2018 •

edited

Loading

MartinHjelmare Jan 16, 2018 •

edited

Loading

pvizeli left a comment •

edited

Loading

pvizeli Jan 17, 2018 •

edited

Loading

pvizeli Jan 20, 2018 •

edited

Loading

pvizeli Jan 17, 2018 •

edited

Loading

pvizeli Jan 20, 2018 •

edited

Loading

pvizeli commented Jan 20, 2018 •

edited

Loading

pvizeli commented Jan 22, 2018 •

edited

Loading