Lazy loading of service descriptions by amelchio · Pull Request #11479 · home-assistant/core

amelchio · 2018-01-06T04:43:57Z

Description:

With this PR the loading of services.yaml is postponed until it is needed, namely when listing services in websocket_api and api.

This enables significant code cleanup in platforms but my main motivation was to allow custom_components testing without having to copy services.yaml around.

Checklist:

The code change is tested and works locally.

If the code does not interact with devices:

Local tests with tox run successfully.
Tests have been added to verify that the new code works.

houndci-bot · 2018-01-06T04:44:07Z

+        'fields': description.get('fields', {})
+    }
+
+@bind_hass


expected 2 blank lines, found 1

houndci-bot · 2018-01-06T04:44:07Z


        return service_ent_id
+
+@bind_hass


expected 2 blank lines, found 1

houndci-bot · 2018-01-06T04:44:07Z

+        'fields': description.get('fields', {})
+    }
+
+@bind_hass


expected 2 blank lines, found 1

houndci-bot · 2018-01-06T04:44:08Z


        return service_ent_id
+
+@bind_hass


expected 2 blank lines, found 1

MartinHjelmare

I like the clean up. Downside is that it's less clear that the services have descriptions.

MartinHjelmare · 2018-01-06T11:17:03Z

+
+    if domain == ha.DOMAIN:
+        import homeassistant.components as components
+        file = components.__file__


file is a python builtin. Please use another variable name, eg fil, comp_file or similar.

MartinHjelmare · 2018-01-06T11:21:10Z

+
+    descriptions = DESCRIPTION_CACHE.get(file)
+    if descriptions is None:
+        descriptions = load_yaml_config_file(


This call will do I/O. Since get_description will be called from async_get_all_descriptions, the function that does I/O needs to be scheduled on the executor.

Oh. This was moved from the LIFX async_setup_platform but I guess that was also wrong.

I am a bit confused by the main/executor transitions available. What is the proper way to schedule the function in this situation?

I think the function that schedules the job for the executor, via hass.async_add_job, needs to be a coroutine to be able to yield and wait for the result, ie to have the descriptions ready. Not 💯 though. I'd wait for a core review before changing anything.

You are correct Martin

houndci-bot · 2018-01-06T15:42:18Z

-        return {domain: {key: value.as_dict() for key, value
-                         in self._services[domain].items()}
-                for domain in self._services}
+        return {domain: self._services[domain].copy() for domain in self._services}


line too long (83 > 79 characters)

balloob · 2018-01-06T22:11:35Z

+        descriptions[domain] = {}
+        for service in services[domain]:
+            description = yield from hass.async_add_job(
+                get_description, hass, domain, service)


Since we know that we're going to load all the descriptions, it would help if we preload the catch all services file (/components/services.yaml). That file is pretty heavy if we have to parse it X times.

It should only be read and parsed once, that's what the DESCRIPTION_CACHE is for.

Oh I see now that you cache that file. I'm not sure if that is a great idea, it's big and a lot will be unused.

balloob · 2018-01-06T22:12:09Z

I love this ❤️

It doesn't make sense to keep it in core.

balloob · 2018-01-06T22:25:25Z

Once removed from all components/platforms, should really speed up the startup and tests 👍

balloob · 2018-01-06T22:26:37Z


 _LOGGER = logging.getLogger(__name__)

+DESCRIPTION_CACHE = {}


Don't store the description cache in a global. Instead store it in hass.data, which is a dict for globals during the runtime of a hass instance.

balloob · 2018-01-06T22:27:55Z

+
+    descriptions = DESCRIPTION_CACHE.get(comp_file)
+    if descriptions is None:
+        descriptions = load_yaml_config_file(


this file might not exist

balloob · 2018-01-06T22:29:02Z

+    if descriptions is None:
+        descriptions = load_yaml_config_file(
+            path.join(path.dirname(comp_file), 'services.yaml'))
+        DESCRIPTION_CACHE[comp_file] = descriptions


Would it make sense to keep a cache per file or would a cache per service be better?

houndci-bot · 2018-01-07T00:24:19Z

+    """Return descriptions (i.e. user documentation) for all service calls."""
+    FILE_CACHE = {}
+
+    if not SERVICE_DESCRIPTION_CACHE in hass.data:


test for membership should be 'not in'

balloob · 2018-01-07T05:21:09Z

+    for domain in services:
+        descriptions[domain] = {}
+        for service in services[domain]:
+            descriptions[domain][service] = yield from hass.async_add_job(


Your code would be a lot faster if you test if the key exists in the cache inside the async context (before this line). No need to yield and wait for a thread to run our job.

balloob · 2018-01-07T05:27:36Z

Super excited about this PR ! It looks like we're saving around ~1 minute on a full test run! (8m23s -> 7m23s)

amelchio · 2018-01-07T09:56:36Z

I reworked it once again, this time with more attention to performance. So now we only async_add_job the actual load_yaml call and we do so concurrently (loading all files at once).

Can't say I notice any difference on my dev setup, though. Maybe it will be more significant on slow machines with a cold disk cache, or maybe not.

@balloob Are the timings from your local setup? I still only use Travis and it is so erratic that I cannot tell whether there is any improvement at all.

MartinHjelmare · 2018-01-07T09:42:31Z

-            msg['id'], self.hass.services.async_services()))
+        @asyncio.coroutine
+        def call_service_helper(msg):
+            """Call a service and fire complete message."""


The function name and docstring are stale. This function doesn't call a service.

MartinHjelmare · 2018-01-07T09:56:50Z

+
+    def format_cache_key(domain, service):
+        """Build a cache key."""
+        return "{}.{}".format(domain, service)


I'd just store the format string as a constant instead of returning it inside a function. You'll save one function call like that.

balloob · 2018-01-07T22:44:51Z

Those were timings from Travis. I took the build times from your branch and compared it to the latest dev branch. Not very scientific as container speeds fluctuate.

balloob · 2018-01-07T22:54:32Z

🎉 🐬 🌮 💃 🍻

amelchio · 2018-01-07T22:55:04Z

The device tracker tests failed frequently with this change. I think I finally figured out that it was a couple of existing races that got easier to hit after the YAML loading was removed.

balloob · 2018-01-07T23:02:55Z

Did a test run on my Macbook Pro Retina 2012:

Before:

Results (276.76s):
    3077 passed
       1 failed
         - tests/test_core.py:800 TestConfig.test_is_allowed_path
      52 skipped

After:

Results (220.09s):
    3078 passed
       1 failed
         - tests/test_core.py:797 TestConfig.test_is_allowed_path
      52 skipped

That's a great improvement! 👍

balloob · 2018-01-07T23:03:20Z

(is_allowed_path always fails on Mac because /tmp is a symlink)

Lazy loading of service descriptions

2000bad

amelchio requested a review from a team as a code owner January 6, 2018 04:43

homeassistant added integration: api integration: websocket_api platform: light.lifx cla-signed labels Jan 6, 2018

balloobbot added component core platform labels Jan 6, 2018

houndci-bot reviewed Jan 6, 2018

View reviewed changes

Fix tests

0e52af7

homeassistant added the cla-signed label Jan 6, 2018

MartinHjelmare reviewed Jan 6, 2018

View reviewed changes

amelchio added 2 commits January 6, 2018 15:44

Load YAML in executor

a1d0a89

Return a copy of available services to allow mutations

94555de

houndci-bot reviewed Jan 6, 2018

View reviewed changes

Remove lint

aff3d73

amelchio force-pushed the lazy-load-descriptions branch from 53c8f61 to aff3d73 Compare January 6, 2018 16:27

homeassistant added the cla-signed label Jan 6, 2018

balloob reviewed Jan 6, 2018

View reviewed changes

amelchio added 2 commits January 7, 2018 01:22

Only cache descriptions for known services

ad57ef8

Add zha/services.yaml

959a34a

houndci-bot reviewed Jan 7, 2018

View reviewed changes

amelchio changed the title ~~RFC: Lazy loading of service descriptions~~ WIP: Lazy loading of service descriptions Jan 7, 2018

amelchio added 2 commits January 7, 2018 04:24

Remove unused import os

e1b0a24

Remove unused import os, part 2

a162015

balloob reviewed Jan 7, 2018

View reviewed changes

amelchio added 2 commits January 7, 2018 10:03

Remove unneeded coroutine decorator

2ee3909

Only use executor for loading files

9b2ea2d

amelchio changed the title ~~WIP: Lazy loading of service descriptions~~ Lazy loading of service descriptions Jan 7, 2018

MartinHjelmare reviewed Jan 7, 2018

View reviewed changes

Cleanups suggested in review

817f69a

homeassistant added the cla-signed label Jan 7, 2018

amelchio added 2 commits January 7, 2018 18:51

Increase test coverage

38bccc5

Fix races in existing tests

e310c0b

balloob approved these changes Jan 7, 2018

View reviewed changes

balloob merged commit 8267a21 into home-assistant:dev Jan 7, 2018

MartinHjelmare mentioned this pull request Jan 11, 2018

Error while setting up platform squeezebox (0.60-DEV) #11569

Closed

amelchio mentioned this pull request Jan 11, 2018

Fix new squeezebox service descriptions for lazy loading #11574

Merged

6 tasks

balloob mentioned this pull request Jan 11, 2018

0.61 #11589

Merged

mouth4war mentioned this pull request Jan 15, 2018

AttributeError: 'NoneType' object has no attribute '__file__' #11654

Closed

This was referenced Jan 15, 2018

Move several local services to their right domain #11677

Merged

Limit service description loading to a single thread #11733

Merged

ciotlosm mentioned this pull request Jan 31, 2018

Climate panel is not displayed until the heat sensor detects a temperature change #12095

Closed

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

ghost added integration: lifx and removed platform: light.lifx labels Mar 21, 2019

Uh oh!

Conversation

amelchio commented Jan 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MartinHjelmare left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

balloob commented Jan 6, 2018

Uh oh!

balloob commented Jan 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

balloob commented Jan 7, 2018

Uh oh!

amelchio commented Jan 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

balloob commented Jan 7, 2018

Uh oh!

balloob commented Jan 7, 2018

Uh oh!

amelchio commented Jan 7, 2018

Uh oh!

balloob commented Jan 7, 2018

Uh oh!

balloob commented Jan 7, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

amelchio commented Jan 6, 2018 •

edited

Loading