Skip to content

Make WUnderground async #12385

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.

Sign up for GitHub

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

Merged
balloob merged 7 commits into from
Feb 16, 2018
Merged

Make WUnderground async #12385

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
cecdc4a
🐎 Async WUnderground
OttoWinter Feb 13, 2018
c546d8e
∞ Them lines be too long
OttoWinter Feb 13, 2018
371767d
Fix pylint warnings
OttoWinter Feb 13, 2018
52000ca
Changes according to comments
OttoWinter Feb 14, 2018
988e907
Remove STATE_UNKNOWN
OttoWinter Feb 14, 2018
20bcc44
🔬 Fix tests
OttoWinter Feb 14, 2018
2b37fcf
Improve tests
OttoWinter Feb 16, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 27 additions & 18 deletions homeassistant/components/sensor/wunderground.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,21 +4,24 @@
For more details about this platform, please refer to the documentation at
https://home-assistant.io/components/sensor.wunderground/
"""
import asyncio
from datetime import timedelta
import logging

import aiohttp
import async_timeout
import re
import requests
import voluptuous as vol

from homeassistant.helpers.typing import HomeAssistantType
from homeassistant.helpers.typing import HomeAssistantType, ConfigType
from homeassistant.components.sensor import PLATFORM_SCHEMA, ENTITY_ID_FORMAT
from homeassistant.const import (
CONF_MONITORED_CONDITIONS, CONF_API_KEY, CONF_LATITUDE, CONF_LONGITUDE,
TEMP_FAHRENHEIT, TEMP_CELSIUS, LENGTH_INCHES, LENGTH_KILOMETERS,
LENGTH_MILES, LENGTH_FEET, STATE_UNKNOWN, ATTR_ATTRIBUTION)
from homeassistant.exceptions import PlatformNotReady
from homeassistant.helpers.entity import Entity, generate_entity_id
from homeassistant.helpers.entity import Entity, async_generate_entity_id
from homeassistant.helpers.aiohttp_client import async_get_clientsession
from homeassistant.util import Throttle
import homeassistant.helpers.config_validation as cv

Expand Down Expand Up @@ -627,7 +630,9 @@ def _get_attributes(rest):
})


def setup_platform(hass, config, add_devices, discovery_info=None):
@asyncio.coroutine
def async_setup_platform(hass: HomeAssistantType, config: ConfigType,
async_add_devices, discovery_info=None):
"""Set up the WUnderground sensor."""
latitude = config.get(CONF_LATITUDE, hass.config.latitude)
longitude = config.get(CONF_LONGITUDE, hass.config.longitude)
Expand All @@ -639,11 +644,11 @@ def setup_platform(hass, config, add_devices, discovery_info=None):
for variable in config[CONF_MONITORED_CONDITIONS]:
sensors.append(WUndergroundSensor(hass, rest, variable))

rest.update()
yield from rest.async_update()

@balloob balloob Feb 16, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you do this update before initializing the sensors, you will not do unnecessary work. It also means that you don't have to pass True to async_add_devices because the sensors have access to data the moment it is initialized.

@OttoWinter OttoWinter Feb 16, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you do this update before initializing the sensors, you will not do unnecessary work.

With the current design of this platform (which isn't ideal, imho), when a sensor entity is created it tells the data holder that its "feature" should be monitored, so doing WUndergroundData#async_update() before the sensors have added their features would result in an empty query. We could however instead just pass the features into the data holder directly and not through the sensors, but I tried to limit this PR for now as this platform has many improvements that could be made (using the dispatcher to not rely on polling, ...)

It also means that you don't have to pass True to async_add_devices

The reason I'm passing True to async_add_devices is that the only way for sensors to get new data is through their own WUndergroundSensor#async_update. They don't get notified by WUndergroundData that a request has been made. Instead, they periodically poll data from the data holder, which also takes care of throttling the request. Therefore the only way to have some data in the entities before async_update is automatically called is by passing True to async_add_devices. Again, this could be fixed by completely reworking this platform.

@balloob balloob Feb 16, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay, fine fine 👍

@OttoWinter OttoWinter Feb 16, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry if this was perceived as being a bit rude - that was not my intention. I wanted to explain why I did this so that you, and anybody attempting to change this part of the codebase in the future, can follow what's going on. It's always better to ask when some code snippet "smells wrong" 👍.

Anyway, the underlying issue of this component being weird with it using polling still persists. But I think changing that would make the most sense during a transition to the weather component (btw, I'm not using this platform too much, so I wouldn't be the one who would like to do that 🤓).

if not rest.data:
raise PlatformNotReady

add_devices(sensors)
async_add_devices(sensors)

return True

@MartinHjelmare MartinHjelmare Feb 14, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this. Nothing is checking this return value.

@OttoWinter OttoWinter Feb 14, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. Side note: Does this comment apply to the whole codebase? Was wondering about that in #12274


Expand All @@ -663,7 +668,7 @@ def __init__(self, hass: HomeAssistantType, rest, condition):
self._entity_picture = None
self._unit_of_measurement = self._cfg_expand("unit_of_measurement")
self.rest.request_feature(SENSOR_TYPES[condition].feature)
self.entity_id = generate_entity_id(
self.entity_id = async_generate_entity_id(

@OttoWinter OttoWinter Feb 14, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All other platforms using async_generate_entity_id this way, but should we really be using an event loop callback from within __init__? Would async_added_to_hass also work or is the entity_id required before that method is called?

@MartinHjelmare MartinHjelmare Feb 14, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not? It's not doing any I/O, so should be ok.

@OttoWinter OttoWinter Feb 14, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order to call an async_* function in Home Assistant we need to guarantee that the caller is inside the event loop. If we can't guarantee that, we usually wrap the async call inside a hass.add_job().

So, because of our direct call to async_generate_entity_id() we'd also need to guarantee that __init__ is executed from within the event loop. Can we guarantee that though? In this case, it's only used in asyn_setup_platform (therefore inside the event loop), but I mean the object could also be initialized from outside the event loop.

@MartinHjelmare MartinHjelmare Feb 14, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Entities are supposed to be initialized in setup_platform/async_setup_platform. Also the caller is responsible to properly handle the objects it calls. So it's fine like this.

@balloob balloob Feb 16, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

async_added_to_hass is executed after entity_id is assigned. This is fine since the platform uses async_setup_platform

ENTITY_ID_FORMAT, "pws_" + condition, hass=hass)

def _cfg_expand(self, what, default=None):
Expand Down Expand Up @@ -727,9 +732,10 @@ def unit_of_measurement(self):
"""Return the units of measurement."""
return self._unit_of_measurement

def update(self):
@asyncio.coroutine
def async_update(self):
"""Update current conditions."""
self.rest.update()
yield from self.rest.async_update()

if not self.rest.data:
# no data, return
Expand Down Expand Up @@ -764,28 +770,31 @@ def request_feature(self, feature):

def _build_url(self, baseurl=_RESOURCE):
url = baseurl.format(
self._api_key, "/".join(self._features), self._lang)
self._api_key, '/'.join(sorted(self._features)), self._lang)

@OttoWinter OttoWinter Feb 13, 2018 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self._features needs to be sorted in order to make the generated URL stable for tests - i.e. so that the generated URL doesn't depend on the order of execution.

if self._pws_id:
url = url + 'pws:{}'.format(self._pws_id)
else:
url = url + '{},{}'.format(self._latitude, self._longitude)

return url + '.json'

@asyncio.coroutine
@Throttle(MIN_TIME_BETWEEN_UPDATES)
def update(self):
def async_update(self):
"""Get the latest data from WUnderground."""
try:
result = requests.get(self._build_url(), timeout=10).json()
websession = async_get_clientsession(self._hass)

@MartinHjelmare MartinHjelmare Feb 14, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the session is persistent, so you should not do this in the update method but in async_setup_platform and pass it to the instance and store the session as an instance attribute.

@OttoWinter OttoWinter Feb 14, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. Fixed.

with async_timeout.timeout(10, loop=self._hass.loop):
response = yield from websession.get(self._build_url())
result = yield from response.json()
if "error" in result['response']:
raise ValueError(result['response']["error"]
["description"])
else:
self.data = result
return True
self.data = result
return True

@MartinHjelmare MartinHjelmare Feb 14, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing is checking this return value.

except ValueError as err:
_LOGGER.error("Check WUnderground API %s", err.args)
self.data = None
except requests.RequestException as err:
except (asyncio.TimeoutError, aiohttp.ClientError) as err:
_LOGGER.error("Error fetching WUnderground data: %s", repr(err))
self.data = None
self.data = None
return False

@MartinHjelmare MartinHjelmare Feb 14, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.

Loading