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

"Event is not JSON serializable" occurs when using templating in Bayesian sensor #33929

Closed
grantalewis opened this issue Apr 9, 2020 · 12 comments

Comments

@grantalewis
Copy link

grantalewis commented Apr 9, 2020

The problem

Beginning in 0.108.0, when using templating in a Bayesian sensor, I'm seeing "Event is not JSON serializable" errors in the log. If I delete the observations that use templating from the sensor, the error goes away. These errors do not appear in 0.107.7.

Environment

  • Home Assistant Core release with the issue: All versions since 0.108.0.
  • Last working Home Assistant Core release (if known): 1.107.7
  • Operating environment (Home Assistant/Supervised/Docker/venv): Docker, Ubuntu 64 4.15.0-88
  • Integration causing this issue: Unsure
  • Link to integration documentation on our website: https://www.home-assistant.io/integrations/bayesian/

Problem-relevant configuration.yaml

binary_sensor:
  - platform: 'bayesian'
    name: 'son_likely_awake'
    prior: 0.62
    probability_threshold: 0.5
    observations:
  # The following observations do not produce problems
      - entity_id: 'light.son_bedroom_son_bedroom_fan_light'
        prob_given_true: 0.27
        platform: 'state'
        to_state: 'on'
      - entity_id: 'light.game_room_game_room_recessed'
        prob_given_true: 0.4
        platform: 'state'
        to_state: 'on'
      - entity_id: 'switch.houseisawake'
        prob_given_true: 0.87
        platform: 'state'
        to_state: 'on'
      - entity_id: 'media_player.game_room_xbox_one'
        prob_given_true: 0.3
        platform: 'state'
        to_state: 'on'
  # The entries below, if included, cause the log errors to appear
      - platform: template
        value_template: >
          {{ states('sensor.time') < ( states.input_datetime.son_bed_time.attributes.timestamp ) | timestamp_custom('%H:%M', False) and is_state('input_boolean.son_bedtime_alert','on') }}
        prob_given_true: 0.75
      - platform: template
        value_template: >
          {{ states('sensor.time') > ( states.input_datetime.son_alarm_time.attributes.timestamp ) | timestamp_custom('%H:%M', False) and is_state('input_boolean.enable_wake_son','on') }}
        prob_given_true: 0.75
      - platform: template
        value_template: >
          {{ (now().isoweekday()==6 and now() < now().replace(hour=10).replace(minute=00).replace(second=0).replace(microsecond=0)) }}
        prob_given_true: 0.07
      - platform: template
        value_template: >
          {{ (now().isoweekday()==7 and now() < now().replace(hour=8).replace(minute=30).replace(second=0).replace(microsecond=0) and is_state('switch.pres_son','on')) }}
        prob_given_true: 0.07

Traceback/Error logs

Additional information

@dshokouhi dshokouhi added this to the 0.108.2 milestone Apr 9, 2020
@balloob
Copy link
Member

balloob commented Apr 10, 2020

@jlmcgehee21 I think that this might be caused by your PR #30962. Would you be able to take a look? It happens if data is stored in device state attributes that is not JSON serializable.

@balloob balloob modified the milestones: 0.108.2, 0.108.3 Apr 10, 2020
@jlmcgehee21
Copy link
Contributor

@balloob — yes I will address this. Would a top-level test that searches through all modules and asserts that state attributes are serializable be desirable? I’m thinking in this manner because (as a non core-contributor) I didn’t know this was a spec for state attributes, but a failing test would have alerted me to this fact.

@balloob
Copy link
Member

balloob commented Apr 10, 2020

You can't make a top level test because you need to know how to set up every integration. Instead, each integration should crash if state attributes cannot be written. But we do have tests for templates right?

@jlmcgehee21
Copy link
Contributor

Not sure I quite understand what you’re getting at. It should of course be easy to add a test that is scoped to the Bayesian Binary Sensor. I’m just thinking about the next dev who makes a component and is not aware that the functionality of other components may be coupled to the serializability of their module’s state attributes.

@sophof
Copy link
Contributor

sophof commented Apr 11, 2020

I don't think there's currently any tests for the templates in the bayesian sensor. I added them in #29122, but that request was not merged. We could add those tests after this issue is resolved in a separate PR?

I can't seem to create a link to the lines of code, but it's line 153 to 216 in test_binary_sensor.py

@sophof
Copy link
Contributor

sophof commented Apr 11, 2020

With the heavy refactoring of the code I can't be sure why certain changes have been done (some of them seem counter-intuitive to me), but it appears this doesn't do what the original coder expects:

ATTR_OBSERVATIONS: list(self.current_observations.values()),

It returns a list of dicts since it's an orderdedDict, I assume it is supposed to return the current prob_given_true values?
Changing it to seems to fix it:

            ATTR_OBSERVATIONS: list(
                {
                    obs.get("prob_given_true")
                    for obs in self.current_observations.values()
                    if obs is not None
                }
            ),

I am however quite unsure of the code's logic at this point so hesitant to provide this as fix.

@lbschenkel
Copy link
Contributor

Just chiming in: this issue is still a blocker for many users. If you use templating, HASS becomes unusable because it'll keep logging this many times per second and the web interface never loads. This is still happening in 0.108.7.

Only solution is to do a rollback to 0.107.7 via hassos-cli core update --version=0.107.7.

@sophof
Copy link
Contributor

sophof commented Apr 22, 2020

Another temporary solution is by changing the code by hand using the fix I provided above. Am waiting on @jlmcgehee21 since I believe he is the one who wrote the new code for the sensor. I'm not sure if my change is what is expected, so I'm afraid it'll break other things if I provide a pull request.

If I go to the code right now either I don't understand fully what is going on, or it is very inefficient. I'm working on the assumption of the first for the moment.

@grantalewis
Copy link
Author

Another temporary solution is by changing the code by hand using the fix I provided above.

Unfortunately this isn't an option for many users. As @lbschenkel says, I'm stuck on 107.7.

@sophof
Copy link
Contributor

sophof commented Apr 25, 2020

Pull request referenced by @jlmcgehee21 fixes it for me :)
I assume it'll be included soon since it is merged? (it doesn't seem to be included in 108.9)

[edit] ooh the observations are listed much more readable now in the web-interface, that wasn't always the case was it?

@Giannie
Copy link
Contributor

Giannie commented Apr 25, 2020

Pull request referenced by @jlmcgehee21 fixes it for me :)
I assume it'll be included soon since it is merged? (it doesn't seem to be included in 108.9)

[edit] ooh the observations are listed much more readable now in the web-interface, that wasn't always the case was it?

It looks like it’s scheduled to be included in 0.109 on Wednesday.

@jlmcgehee21
Copy link
Contributor

[edit] ooh the observations are listed much more readable now in the web-interface, that wasn't always the case was it?

@sophof, yes that was the intent, but that pesky Template object got me 🙃

@balloob balloob closed this as completed Apr 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants