Skip to content
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

Get room hints from areas #21519

Merged
merged 3 commits into from Mar 2, 2019

Conversation

Projects
None yet
4 participants
@Swamp-Ig
Copy link
Contributor

commented Feb 28, 2019

Description:

When syncing devices, return room hints for area names if not configured in the config file.

Pull request in home-assistant.io with documentation (if applicable): home-assistant/home-assistant.io#8772

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.

If user exposed functionality or configuration variables are added/changed:

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.

@ghost ghost added the in progress label Feb 28, 2019

@Swamp-Ig Swamp-Ig referenced this pull request Feb 28, 2019

Merged

Room/Area support documentation. #8772

2 of 2 tasks complete

@Swamp-Ig Swamp-Ig changed the title WIP: Get room hints from areas Get room hints from areas Feb 28, 2019

entity_registry.async_get_registry())

entity_entry = entity_registry.async_get(state.entity_id)
if entity_entry:

This comment has been minimized.

Copy link
@balloob

balloob Feb 28, 2019

Member

Let's restructure this code using guard clauses to be a bit more readable. If we move the for trt in traits code above the room hint figuring out, we can just return device as soon as a room hint is configured or any other if-check is falsey. We can then drop the else.

Now this line can be replaced with:

if not entity_entry:
    return device

This comment has been minimized.

Copy link
@balloob

balloob Feb 28, 2019

Member

same with the other 2 if statements below.

This comment has been minimized.

Copy link
@Swamp-Ig

Swamp-Ig Feb 28, 2019

Author Contributor

Put it into an internal function so that we don't have stray returns in the procedure.

This comment has been minimized.

Copy link
@Swamp-Ig

Swamp-Ig Mar 1, 2019

Author Contributor

See comment as below...

device_entry = device_registry.devices.get(
entity_entry.device_id)
if device_entry and device_entry.area_id:
area_registry = (await self.hass.helpers.

This comment has been minimized.

Copy link
@balloob

balloob Feb 28, 2019

Member

I wonder if we should get all registries at once, to avoid having to yield 3 times.

dev_reg, ent_reg, area_reg = asyncio.gather(
    self.hass.helpers.device_registry.async_get_registry(),
    self.hass.helpers.entity_registry.async_get_registry(),
    self.hass.helpers.area_registry.async_get_registry(),
)

This comment has been minimized.

Copy link
@balloob

balloob Feb 28, 2019

Member

Actually, we should move getting the registries into async_devices_sync so that we only have to get the registries once. Or maybe they should become part of config ?

This comment has been minimized.

Copy link
@balloob

balloob Feb 28, 2019

Member

Let's start with just putting it in async_devices_sync

This comment has been minimized.

Copy link
@Swamp-Ig

Swamp-Ig Feb 28, 2019

Author Contributor

I have pushed an update that changes all this.

I was thinking however that it might be useful to have some helper methods:

in DeviceRegistry:

async def get_by_entity_id(entity_id) -> Optional[DeviceEntry]:

and in AreaRegistry:

async def get_by_entity_id(entity_id) -> Optional[AreaEntry]:
async def get_by_device_id(device_id) -> Optional[AreaEntry]:

I don't know that caching the registries here makes a lot of difference performance wise, they're cached anyhow so it's just a quick look-up. The await is only so that it can load it from storage if needed, and that's only the first call anyhow.

These helper methods would be in a different PR though.

room = entity_config.get(CONF_ROOM_HINT)
if room:
device['roomHint'] = room
async def set_room():

This comment has been minimized.

Copy link
@balloob

balloob Feb 28, 2019

Member

I think that we should pass the registries as parameters to entity.sync_serialize() and only fetch them once inside async_devices_sync

When I talked about guard clauses, I didn't mean to extract it into a new method. You can move the for trt in traits for-loop above the room resolving, and then just return device when you're done processing

This comment has been minimized.

Copy link
@Swamp-Ig

Swamp-Ig Mar 1, 2019

Author Contributor

I didn't mean to extract it into a new method. You can move the for trt in traits for-loop above the room resolving, and then just return device when you're done processing

I didn't like this, it seems messy that you're moving stuff around so you can sneak in a return. It's too much of a goto statement.

This comment has been minimized.

Copy link
@balloob

balloob Mar 1, 2019

Member

I don't agree, it's like the sole purpose of a return: return something from a function when you're done processing. With a guard clause you check if further processing is needed, and return otherwise.

Right now you still have the same returns, except that we also define a new function on each invocation of sync and do an extra yield to the event loop. Without the function, if a room hint is configured, no yielding will be done.

This comment has been minimized.

Copy link
@Swamp-Ig

Swamp-Ig Mar 1, 2019

Author Contributor

OK changed it as requested.

NB: _GoogleEntity gets reconstructed with every invocation, and each call to sync_serialise is just a single call on that object, so there's not a lot of value in moving the registry lookup code anywhere else.

@awarecan
Copy link
Contributor

left a comment

Per the discussion in #21373 (comment), it will be very useful if we can have debug log for the response we sent to google, especially for SYNC command, since Google have a validation tool to verify those response JSON.

if not (entity_entry and entity_entry.device_id):
return

device_entry = dev_reg.devices.get(entity_entry.device_id)

This comment has been minimized.

Copy link
@awarecan

awarecan Feb 28, 2019

Contributor

I will suggest add a device_registry.async_get() method to replace devices.get(), and do the same enhancement in area_registry as well. Directly access devices and areas are not good design.

Moreover, it will be very helpful if we have a helper input is a entity_id, output are the entity_entry, device_entry, and area_entry

I actually have those in my another un-submitted pr.

This comment has been minimized.

Copy link
@balloob

balloob Feb 28, 2019

Member

That is a good point.

I've also been thinking about if we should get the registries squared out earlier and store it on hass, so we don't have to await whenever we use it. Downside is that we delay startup a little bit

This comment has been minimized.

Copy link
@Swamp-Ig

Swamp-Ig Mar 1, 2019

Author Contributor

The startup delay isn't going to be that long, the asynchio thread is free to go do other stuff while it waits for the data to load from disk. I expect the registry loading is going to happen pretty early in the piece anyhow because at the very least the entity registry needs to load before a lot of the components, so it's not like you've really gained anything much startup time wise by pseudo lazy-loading it.

I guess at minimum you have to load the config files and the three registries from disk because basically everything else hangs off these. Would an early asyncio.gather hitting these bits of IO and parsing them be worthwhile? That way at least the loading-parsing steps get done in 'parallel', in whatever order they load from disk. Then you can just store the objects somewhere and not have to worry about awaiting any of their methods. It's probably not worth doing it threaded, although you could, the longest wait is for the IO, the parsing isn't going to be that CPU intensive.

OTOH, awaiting a method that doesn't actually do any IO isn't going to be very expensive so there's no major harm in occasional awaits, only it gets annoying because you have to propagate the async marker to everything that calls them. It does have the upside that you don't need to do a big refactor just to get this stuff sorted earlier.

This comment has been minimized.

Copy link
@balloob

balloob Mar 1, 2019

Member

Some of these files can grow pretty big, there are people with 1500 entities. If we delay start up, we cannot initialize any component until all three are loaded. In the future more data might be added, and we end up delaying startup even more.

This comment has been minimized.

Copy link
@Swamp-Ig

Swamp-Ig Mar 1, 2019

Author Contributor

I just did a test run with the default config, and entity_registry.async_get_registry and device_registry.async_get_registry does get called on start up, it's hit as soon as any platform calls the async_add_entities method in async_setup_platform for the component which first happens with the yr platform, although it does get hit dozens more times.

This hits entity_platform.async_add_entities, so really no components can become available until at least the entity and device registries are loaded from disk. So the upshot is that startup gets delayed no matter what you do so concerns around start up time are a bit neither here nor there.

Of course this is aio enabled, so we can get on and do other work in the mean time, which might be a reasonable enough argument to leave it how it is.

I'm not really venturing an opinion on how it should be, just listing the pros and cons 😁

Anyhow this is getting quite off-topic from this PR. Happy to change it in the future if the registry code changes.

This comment has been minimized.

Copy link
@balloob

balloob Mar 1, 2019

Member

Agree with the off-topic and good point on it being used inside Entity Platform, so we might as well make it available right away.

Swamp-Ig added some commits Feb 26, 2019

@Swamp-Ig Swamp-Ig force-pushed the Swamp-Ig:google-assist-areas branch from 4a6b885 to a76d714 Mar 1, 2019

@balloob

balloob approved these changes Mar 1, 2019

Copy link
Member

left a comment

Great 🎉

@balloob balloob merged commit f61f650 into home-assistant:dev Mar 2, 2019

4 checks passed

Hound No violations found. Woof!
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls First build on google-assist-areas at 92.746%
Details

@ghost ghost removed the in progress label Mar 2, 2019

@Swamp-Ig Swamp-Ig deleted the Swamp-Ig:google-assist-areas branch Mar 2, 2019

@balloob balloob referenced this pull request Mar 20, 2019

Merged

0.90.0 #22216

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.