Async version of Yr.no #4158

kellerza · 2016-10-31T21:09:24Z

Description:
Async version of yr.no. Continue work of #3597

The logic of the YrSensor / YrData changed:

In the past, Each sensor(YrSensor) called the datastore (YrData), which would update the data if the last result expired.
With the new logic, Yr.data is scheduled to execute on _nexttime and pushes the new data updates to the sensors (YrSensor)

Example entry for configuration.yaml (if applicable):

sensor:
  - platform: yr
    monitored_conditions:
      - temperature
      - symbol
      - precipitation
      - windSpeed

Checklist:

If the code communicates with devices, web services, or third-party tools:

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

mention-bot · 2016-10-31T21:09:25Z

@kellerza, thanks for your PR! By analyzing the history of the files in this pull request, we identified @balloob, @Danielhiversen and @fabaff to be potential reviewers.

kellerza · 2016-10-31T21:10:34Z

tests/test_util/aiohttp.py

@@ -57,7 +57,7 @@ def call_count(self):
        return len(self.mock_calls)

    @asyncio.coroutine
-    def match_request(self, method, url, *, auth=None):
+    def match_request(self, method, url, *, auth=None, **kwargs):


This allows the timeout parameter in the get request. (and possible future arguments)

I don't want to use kwargs if we know exactly what keywords to expect. By adding uncertainty we add tech debt.

kellerza · 2016-10-31T21:11:17Z

tests/test_util/aiohttp.py

@@ -80,7 +80,7 @@ def __init__(self, method, url, status, response):

    def match_request(self, method, url):
        """Test if response answers request."""
-        return method == self.method and url == self.url
+        return method == self.method and url.startswith(self.url)


Non-async requests mock handled "startswith" type functionality with the old tests.

That doesn't seem to be the case, see the source. The difference however is that the query string was tested separately.

kellerza · 2016-10-31T21:12:48Z

homeassistant/components/sensor/yr.py

-        self.update()
+        self.hass = hass
+        async_track_time_change(self.hass, self.async_update, second=0,
+                                minute=0, hour=3)


This request schedules a once per day update in case there was a connection error etc in the async_update method. It should effectively "restart" the updating in case anything went wrong.

With this approach you are going to have a new connection run in parallel each day. This will only work if you keep track of the unsub functions that async_track_point_in_utc_time will return.

It's also bad style to just put a restart everything piece in your code. Doesn't show much faith in your own abilities 😛

👍 Not sure what I was thinking...

balloob · 2016-11-01T01:21:25Z

homeassistant/components/sensor/yr.py

-        self.update()
+        self.hass = hass
+        async_track_time_change(self.hass, self.async_update, second=0,
+                                minute=0, hour=3)


With this approach you are going to have a new connection run in parallel each day. This will only work if you keep track of the unsub functions that async_track_point_in_utc_time will return.

It's also bad style to just put a restart everything piece in your code. Doesn't show much faith in your own abilities 😛

balloob · 2016-11-01T01:21:42Z

homeassistant/components/sensor/yr.py

+            if resp.status != 200:
+                return
+            _text = (yield from resp.text())
+        except asyncio.TimeoutError:
            return


Any time you return you should reschedule another connection.

balloob · 2016-11-01T01:22:12Z

homeassistant/components/sensor/yr.py

-        data = response.text
+        finally:
+            self.hass.loop.create_task(resp.release())
+            # ? yield from resp.release()


create_task is fine, no need to wait for it to release to continue processing this response.

balloob · 2016-11-01T01:22:36Z

homeassistant/components/sensor/yr.py

-        except requests.RequestException:
+            if resp.status != 200:
+                return
+            _text = (yield from resp.text())


Prefixing a local variable with _ usually indicates that it is not going to be used.

balloob · 2016-11-01T01:23:27Z

homeassistant/components/sensor/yr.py

-        # Check if new will be available
-        if self._nextrun is not None and dt_util.utcnow() <= self._nextrun:
-            return
+        resp = yield from self.hass.websession.get(self._url, timeout=30)


This is not how aiohttp does timeouts. See their Getting Started

I probably have read timeouts here then and not connection setup timeout http://aiohttp.readthedocs.io/en/stable/client.html#timeouts

balloob · 2016-11-01T01:27:29Z

homeassistant/components/sensor/yr.py

+            # pylint: disable=protected-access
+            if new_state != dev._state:
+                dev._state = new_state
+                yield from dev.async_update_ha_state()


Instead of yielding on a per sensor basis, you can use gather.

tasks = [] for dev in y: tasks.append(dev.async_update_ha_state()) yield from asyncio.gather(*tasks, loop=hass.loop)

balloob · 2016-11-01T02:39:43Z

tests/components/sensor/test_yr.py

-        now = datetime(2016, 6, 9, 1, tzinfo=dt_util.UTC)
+def _setup_platform(hass, config):
+    """Setup a yr platform."""
+    hass.allow_pool = True


You would not have to allow pool if you convert the setup to be async_setup_platform

balloob · 2016-11-01T02:41:15Z

tests/test_util/aiohttp.py

@@ -57,7 +57,7 @@ def call_count(self):
        return len(self.mock_calls)

    @asyncio.coroutine
-    def match_request(self, method, url, *, auth=None):
+    def match_request(self, method, url, *, auth=None, **kwargs):


I don't want to use kwargs if we know exactly what keywords to expect. By adding uncertainty we add tech debt.

balloob · 2016-11-01T02:47:18Z

tests/test_util/aiohttp.py

@@ -80,7 +80,7 @@ def __init__(self, method, url, status, response):

    def match_request(self, method, url):
        """Test if response answers request."""
-        return method == self.method and url == self.url
+        return method == self.method and url.startswith(self.url)


That doesn't seem to be the case, see the source. The difference however is that the query string was tested separately.

balloob · 2016-11-01T02:48:00Z

tests/components/sensor/test_yr.py

+    state = hass.states.get('sensor.yr_symbol')
+
+    assert state.state == '3'
+    assert state.state.isnumeric()


This assert makes no sense, we know the state is '3' so we know it will be numeric.

kellerza · 2016-11-01T20:44:39Z

tests/components/sensor/test_yr.py

 @asyncio.coroutine
 def test_default_setup(hass, aioclient_mock):
    """Test the default setup."""
    aioclient_mock.get('http://api.yr.no/weatherapi/locationforecast/1.9/',
                       text=load_fixture('yr.no.json'))
    config = {'platform': 'yr',
              'elevation': 0}
-    yield from hass.loop.run_in_executor(None, _setup_platform(hass, config))
+    hass.allow_pool = True


Could not get rid of this yet...

balloob · 2016-11-02T03:59:45Z

homeassistant/components/sensor/yr.py

-    add_devices(dev)
+    weather = YrData(hass, coordinates, dev)
+    tasks = [async_add_devices(dev), weather.async_update()]
+    yield from asyncio.gather(*tasks, loop=hass.loop)


Instead of this work around you can just write

yield from asyncio.gather(async_add_devices(dev), weather.async_update(), loop=hass.loop)

The * operator in a function call will spread out an iterable over the positional arguments

However, in this case it is actually important that async_add_devices runs first, because it sets the hass attribute and entity_id which is needed to call async_update_ha_state. However, without weather.async_update, you will write unknown to the state machine.

Do note that this way you will first get a bunch of unknown states that will resolve once weather.async_update comes through.

I guess the ultimate solution would be to make an async friendly throttle.

For now I will yield in two statements (1) to make sure entities are created and then (2) updated

Not quite following the "throttling" comment. Delay/wait makes sense, but guess yielding gives that. This should also fix the sometimes failing Travis tests, 2which seems to pass consistently on my dev machine

balloob · 2016-11-02T04:01:31Z

homeassistant/components/sensor/yr.py

+                model = model[0]
+            next_run = dt_util.parse_datetime(model['@nextrun'])
+        except (ExpatError, IndexError) as err:
+            return try_again(err)


Why do you sometimes return something, but not in case of a success?

It is simply to exit from the function in case of aborting the update, the return value is not important. try_again also returns nothing (None?). Should I rather split this statement over two lines?

Yes, split it over two lines. If people should not use the return value, it should have none.

balloob · 2016-11-02T04:01:59Z

tests/test_util/aiohttp.py

@@ -15,7 +16,7 @@ def __init__(self):
        self.mock_calls = []

    def request(self, method, url, *,
-                auth=None,
+                auth=None,  # pylint: disable=unused-variable


Pretty sure this one is disabled in our pylintrc

I will make sure it is there, this raises a warning on my recent dev branch

unused-argument is present in pylintrc, but not unused-variable. I decided against adding unused-variable since it seems too wide.

According to the documentation unused-argument should be raised here, so not sure why I'm getting unused-variable here

balloob · 2016-11-02T04:02:12Z

tests/test_util/aiohttp.py

@@ -57,7 +58,8 @@ def call_count(self):
        return len(self.mock_calls)

    @asyncio.coroutine
-    def match_request(self, method, url, *, auth=None):
+    def match_request(self, method, url, *, auth=None, timeout=None): \


Do you still need to add timeout now that you are no longer using it?

Will remove. It is only applicable for long streaming type requests I guess, for most of our uses the default should be ok

balloob · 2016-11-02T04:03:07Z

tests/test_util/aiohttp.py

+        self._url = url
+        try:
+            self._url_parts = urlparse(url.lower())
+        except AttributeError:


Don't catch this, just blow up. It's usually pretty bad to swallow errors because it means that we won't notice that we mess up until way later and it will be difficult to find the cause.

This mimick requests_mock. The reason for this is that you can pass in a regex object for url, but will rather ask for permission with a test than ask for forgiveness

balloob · 2016-11-02T04:04:34Z

tests/test_util/aiohttp.py

+
+        # Reuse Mock request's match function
+        # pylint: disable=protected-access, attribute-defined-outside-init
+        from requests_mock.adapter import _Matcher


I was considering extracting this class eventually as a standalone lib - so we can't depend on other mockers.

Ok, will revert to blatant plagiarism :-P

Makes sense if you want to contribute it back to aiohttp

balloob · 2016-11-03T02:34:09Z

Woohoo 🐬

Danielhiversen · 2016-11-19T15:20:52Z

@kellerza , @balloob
This pr introduces a bug in the yr sensor. It is no longer updated as often as it should.
It seems that you have misunderstood what the next_run parameter means.

The next_run says when new xml data are available. But each xml file can contain data for several time periods.

Example:
http://api.met.no/weatherapi/locationforecast/1.9/?lat=63;lon=11;msl=0
For each hour the update function should be called, to update the new weater data.
But the xml data should not be downloaded that often, and is given by the next_run parameter.

balloob · 2016-11-19T17:51:17Z

Judging from the code it used to always only update after next_run.

kellerza · 2016-11-19T18:01:59Z

Pretty sure it did not update more often in the past, but looking at the XML data it seems like it should update more often though... I can work on a patch tomorrow evening.

EDIT: Looking at the Entity's update this does seem to need some work

Danielhiversen · 2016-11-19T18:11:11Z

@balloob :
You are referering to the update function in the "class YrData", and that should only update when new xml data is available.

But each senor should check if more recently data are available in the YrData class:
https://github.com/home-assistant/home-assistant/pull/4158/files#diff-08b07de97392e5a044466a28e35157c9L123

kellerza · 2016-11-19T18:11:57Z

Indeed, as noted in my edit above.

balloob · 2016-11-19T23:32:13Z

i'm sorry, you're right @Danielhiversen

Danielhiversen · 2016-11-20T16:26:26Z

I am also getting this error:

16-11-20 17:00:00 ERROR (MainThread) [homeassistant.core] Error doing job: Exception in callback async_track_point_in_utc_time..point_in_time_listener(<Event time_c....501945+01:00>) at /home/dahoiv/home-assistant/homeassistant/helpers/event.py:108
Traceback (most recent call last):
File "/usr/lib/python3.5/asyncio/events.py", line 125, in _run
self._callback(*self._args)
File "/home/dahoiv/home-assistant/homeassistant/helpers/event.py", line 124, in point_in_time_listener
hass.async_run_job(action, now)
File "/home/dahoiv/home-assistant/homeassistant/core.py", line 251, in async_run_job
self.async_add_job(target, *args)
File "/home/dahoiv/home-assistant/homeassistant/core.py", line 231, in async_add_job
task = self.loop.create_task(target(*args))
TypeError: async_update() takes 1 positional argument but 2 were given

I think it comes from here: https://github.com/home-assistant/home-assistant/pull/4158/files#diff-08b07de97392e5a044466a28e35157c9R147
And async_update in yr.py will be called with the "now" parameter.

So for the moment, the yr sensor will never update

kellerza commented Oct 31, 2016

View reviewed changes

kellerza added async Hacktoberfest labels Oct 31, 2016

balloob reviewed Nov 1, 2016

View reviewed changes

robbiet480 removed the Hacktoberfest label Nov 1, 2016

kellerza force-pushed the yr branch 3 times, most recently from dba3cf2 to c7d5c1e Compare November 1, 2016 20:43

kellerza commented Nov 1, 2016

View reviewed changes

balloob reviewed Nov 2, 2016

View reviewed changes

kellerza added 4 commits November 2, 2016 14:44

initial

d30909d

feedback

ffe0866

More feedback. Still need to fix match_url

07f5e7a

url_match

1c999d3

kellerza force-pushed the yr branch from 181b3b6 to 1c999d3 Compare November 2, 2016 17:44

split_lines

aba09cb

balloob merged commit f3595f7 into home-assistant:dev Nov 3, 2016

kellerza deleted the yr branch November 3, 2016 04:03

Danielhiversen mentioned this pull request Nov 19, 2016

yr sensor #4467

Closed

Danielhiversen mentioned this pull request Nov 21, 2016

YR weather sensors does not auto-update #4494

Closed

home-assistant locked and limited conversation to collaborators Mar 17, 2017

Async version of Yr.no #4158

Async version of Yr.no #4158

Conversation

kellerza commented Oct 31, 2016

mention-bot commented Oct 31, 2016

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

kellerza Oct 31, 2016 • edited Loading

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

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

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

balloob Nov 2, 2016 • edited Loading

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

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

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

balloob commented Nov 3, 2016

Danielhiversen commented Nov 19, 2016 • edited Loading

balloob commented Nov 19, 2016

kellerza commented Nov 19, 2016 • edited Loading

Danielhiversen commented Nov 19, 2016 • edited Loading

kellerza commented Nov 19, 2016

balloob commented Nov 19, 2016 • edited Loading

Danielhiversen commented Nov 20, 2016 • edited Loading

kellerza Oct 31, 2016 •

edited

Loading

balloob Nov 2, 2016 •

edited

Loading

Danielhiversen commented Nov 19, 2016 •

edited

Loading

kellerza commented Nov 19, 2016 •

edited

Loading

Danielhiversen commented Nov 19, 2016 •

edited

Loading

balloob commented Nov 19, 2016 •

edited

Loading

Danielhiversen commented Nov 20, 2016 •

edited

Loading