home-assistant / architecture

Repo to discuss Home Assistant architecture
315 stars 99 forks source link

Template update restrictions with states & states.domain #449

Closed Petro31 closed 3 years ago

Petro31 commented 3 years ago

Context

As you may know, I help tons of people on the forums and discord. In 0.116 we limited templates using {{ states }} or {{ states.domain }} to only update once a minute. This is great for all those people who were using sensor.time, but it's not so great for people who just wanted to update based on the state changes inside states.domain. Here's the most recent issue that I ran into, a overall light dynamic light entity.

light:
  - platform: template
    lights:
      all_lights:
          friendly_name: "All"
          value_template: >-
            {{ state.light | selectattr('state','eq','on') | list | count > 0 }}
          turn_on:
            service: light.turn_on
            entity_id: all
          turn_off:
            service: light.turn_off
            entity_id: all

The gentleman in question only had 15 or 20 lights. He simply wants to have a light that turns and off all lights without needing to adjust a configuration whenever a new light is added. The current band aide to a situation like this is to create an automation to watch states and update the light or come up with some inventive templates.

Proposal

Make expand() work on domains, or add a new function that expands domains.

In the case above, using expand on the light domain would return a generator of light states.

light:
  - platform: template
    lights:
      all_lights:
          friendly_name: "All"
          value_template: >-
            {{ expand('light') | selectattr('state','eq','on') | list | count > 0 }}
          turn_on:
            service: light.turn_on
            entity_id: all
          turn_off:
            service: light.turn_off
            entity_id: all

This would update whenever any new light is added, also it would update exactly when a light is turned on instead of being rate limited to once a minute like state.light would do.

Consequences

The only consequence would be that a user expands the sensor domain and they end up bogging the machine down. Regardless, users will be able to rate limit by just using states.domain, but for those special cases they can use expand to get all the states in a domain.

balloob commented 3 years ago

We have light groups for exactly this reason. Like… is this person adding lights on a daily basis that they need to update their YAML that often, that it warrants a change to how Home Assistant works? I don't believe that this template is a convincing use case.

CC @bdraco who has been doing the recent template work.

amelchio commented 3 years ago

In 0.116 we limited templates using {{ states }} or {{ states.domain }} to only update once a minute.

For full context, this is only a limitation compared to 0.115, right? I believe the template would not update at all with 0.114?

...

Unlike balloob, I think it is a valid use case to avoid manual maintenance of a list of all entities. However, this can already be done; here is an example creating a group.all_lights replacement.

It is more work but I think this approach has some merit because you can then customize the group, for example to exclude outdoor lights.

Petro31 commented 3 years ago

Yes that would work in this situation, but that still requires 2 items for something that could be done with 1. After sitting on this for a bit, I had another idea that could expand the use case here if we went with a new template method. Add glob or just flat out use glob. The glob would only apply to entity_id and this would solve issues dating back to the beginning of HA. I don't think this is an outlandish request and I'd be willing to do the work. So I'd like to ask, what's the reason for resistance? If we add a simple function that globs entity_id's and creates a state object generator, what are the drawbacks? Why wouldn't we want to add something that makes it easier for the user?

balloob commented 3 years ago

The restrictions are there to protect performance. We should not make it easy for users to break their system, especially if there are alternatives.

Petro31 commented 3 years ago

But what's stopping a person from creating a group the exact same way?

script:
  create_every_light_group:
    sequence:
      - service: group.set
        data_template:
          object_id: "every_light"
          entities: >
            {{ states|map(attribute='entity_id') | join(',') }}

Both are opt into. And users who don't have a clue will do both to get what they want.

bdraco commented 3 years ago

This is a difficult one as most users don't need a rate limit on states.DOMAIN. It was only a few users who had a massive number of templates that do. If we were starting from scratch, I don't think it would be a problem to remove the rate limit for states.DOMAIN. states definitely needs it for all cases.

Petro31 commented 3 years ago

That's why I was trying to compromise by adding a glob function or enhance expand. Glob would be the most useful. It would allow users to simplify templates like this (which are all over the forums):

{% for s in states.sensor if 'foo' in s.entity_id %}
   ...
{% endfor %}

to

{{ expand_glob('sensor.*foo*') | ... }}

It'll solve more problems than it causes and it fits into the current paradigm.

bdraco commented 3 years ago

A glob expand is going to have to be processed every time to discover the states that apply. This requires iterating all the states in the state machine which can be an expensive operation on it own when the user has 2000+ states. The current implementation sorts them via sorted() (https://github.com/home-assistant/core/blob/dev/homeassistant/helpers/template.py#L733) as well which makes it more expensive. We could potentially avoid that in a new implementation.

I had put together glob support for groups which would not have the same concern since the group integration would keep track of the entities that matched and expand() would work the same as it does today: https://github.com/home-assistant/core/pull/39718

Petro31 commented 3 years ago

My biggest concern here is that nothing gets done under the guise of "users are lazy and they shouldn't be lazy". We should strive to make the product better any way that we can. So far it seems that both this change and the linked group change both are being dismissed under the assumption that users are lazy. Personally, I can't count how many time's I've implemented code to move away from a hardcoded list, yet here we are forcing this on users.

EDIT: Just a quick example of a similar situation in the software I work on for work. In our software, we have an option to display points with an 'arrow' coming out of it to show the direction. At any given time, we can have upwards of 3 million points in memory and displaying it can be painful. We decided to put a soft cap on the displayed point direction (the arrow) at 1000. But if the user wants to override that, they can in system settings. It's a simple flag. The user doesn't have to jump through hoops backwards to get it working even if it will slow their system to a crawl. And yes, many times with millions of points it leads to OutOfMemory Exceptions. But it was their choice.

bdraco commented 3 years ago

My biggest concern here is that nothing gets done under the guise of "users are lazy and they shouldn't be lazy". We should strive to make the product better any way that we can. So far it seems that both this change and the linked group change both are being dismissed under the assumption that users are lazy. Personally, I can't count how many time's I've implemented code to move away from a hardcoded list, yet here we are forcing this on users.

I think the primary concerns are adding more levers and knobs to adjust behavior and duplicating functionality that increases the long term tech debt.

If we can stay away from that course, we should be able to move a solution forward.

Swamp-Ig commented 3 years ago

A glob expand is going to have to be processed every time to discover the states that apply.

It only needs to be processed when a state is added or removed (uncommon once startup complete), or if a state that is referenced in the last run of the template changes (required regardless).

bdraco commented 3 years ago

A glob expand is going to have to be processed every time to discover the states that apply.

It only needs to be processed when a state is added or removed (uncommon once startup complete), or if a state that is referenced in the last run of the template changes (required regardless).

Right, most of the time it the performance won't be an issue. The case were we keeping running into problems is when the user has 100+ power sensors that are all updating each second (the monitor all circuits case).

I only have about 16 myself so I built an integration to simulate 100 of them: https://github.com/bdraco/scaletest

Swamp-Ig commented 3 years ago

Just a suggestion, but maybe it would be better to apply the automatic rate limit once the template hits more than a particular number of states, rather than some hard-coded trigger based on the stuff fed into it. It's easy enough to see how many get hit, it's all in the states accessed list. As the states accessed list gets larger, throttle the refreshes harder.

If the limit was 10 seconds for 100 states hit I doubt it would really be a big issue. Clock speeds are measured in GHz, so even if we blow up 100 states up by 1000 calculations each for a refresh, that's still only 0.0001% of the calculating ability of the processor being used.

Sorry if this is way off base, I did come up with the algorithm for caching the state list, but pretty much left it alone since then and didn't do any of the detailed testing so I'm way out of the loop.

Petro31 commented 3 years ago

Here are some examples I've recently helped people with on the forums:

glob would help here: 'sensor.node_*_energy'

      {{ [states.sensor.node_3_energy, states.sensor.node_4_energy,
          states.sensor.node_5_energy, states.sensor.node_6_energy,
          states.sensor.node_7_energy] | map(attribute='state') | map('float') | sum }}

with a new glob, the template would be:

{{ new_glob('sensor.node_*_energy') | map(attribute='state') | map('float') | sum }}

this is rate limited and glob would help 'sensor.*_dimmer_energy'

        {% set ns = namespace(states=[], offline=[]) %}
        {% for s in states.sensor %}
          {% if s.object_id.endswith('_dimmer_energy') %}
            {% if s.state in ['unavailable', 'unknown' ] %}
              {% set ns.offline = ns.offline + [ s.entity_id ] %}
            {% else %}
              {% set ns.states = ns.states + [ s.state | float ] %}
            {% endif %}
          {% endif %}
        {% endfor %}
        {% if ns.offline | count > 0 %}
          unavailable
        {% else %}
          {{ ns.states | sum | round(2) }}
        {% endif %}

with a new glob, the template would be

{{ new_glob('sensor.*_dimmer_energy') | map(attribute='state') | map('float') | sum }}

this one is rate limited but who has more than 10 plants, I guess it could be argued that you wouldn't hit the rate limit. But then again, what if this was smoke detectors and you weren't home?

    {% set ns = namespace(count = 0) %}
    {% for plant in states.plant if 'moisture low' in plant.attributes.problem %}
      {% set ns.count = ns.count+1 %}
    {% endfor %}
    {{ns.count}}

with a new glob, the template would be

    {% set ns = namespace(count = 0) %}
    {% for plant in new_glob('plant.*') if 'moisture low' in plant.attributes.problem %}
      {% set ns.count = ns.count+1 %}
    {% endfor %}
    {{ns.count}}

I found these on the forums from people I recently helped. I haven't checked discord yet, that might take a bit of time.

EDIT: Here's one from discord:

The guy has guests that take keyrings and do something with them... not sure what but he wanted a list of them. They are added so frequently that it's a pain in the ass to keep changing the configuration (according to him)

{% ns = namespace(entities=[]) %}
{% for s in states.timer if s.object_id.startswith('keyring') and s.state == 'active' %}
  {% set ns.entities = ns.entities + [ s.entity_id ] %}
{% endfor %}
{{ ns.entities | join(', ') }}
bdraco commented 3 years ago

I think #41225 was too conservative with the rate limit for states.DOMAIN. Looking back at all the profiles I've since received, I think we can safely set it to 1s for domains and keep all states at 1 minute.

I'll run some tests this weekend with the templates I've received and the scale test integration

bdraco commented 3 years ago

Using the scale test integration, even with 16 of these, we spend more time processing (edit: fixed that in https://github.com/home-assistant/core/pull/42005) the rate limit than actually rendering the template at 1s. It should be just fine to lower it.

      all_sensors:
        entity_id: sensor.time
        friendly_name: Unavailable Entities
        unit_of_measurement: items
        value_template: "{{states.sensor|rejectattr('state', 'in', ['unavailable','unknown','none'])|map(attribute='state')|list|join(', ')}}"
bdraco commented 3 years ago

https://github.com/home-assistant/core/pull/42004