jmcollin78 / versatile_thermostat

A full featured Thermostat for Home Assistant: presets, window, motion, presence and overpowering management
MIT License
327 stars 34 forks source link

Expose the keep_alive_sec attribute in HASS Developer Tools - States #381

Closed pdcastro closed 8 months ago

pdcastro commented 9 months ago

This PR improves debugging diagnostics for the switch keep-alive feature for issues such as #379.

That’s achieved through one of the three commits. The other two commits improve type hints.

jmcollin78 commented 9 months ago

Hello @pdcastro ,

I will off for one week and my VSCode is totally broken since a try to switch to Python 12. No more PyLance, no more ability to run unit test. So it will be difficult for me to validate this rapidly. I understand this is not a bug but more for debug purpose : the keep-alive was not into the attributes. So maybe it can waits a few days ?

pdcastro commented 9 months ago

@jmcollin78, no hurry, this PR can wait. I can confirm that I have not found any bugs. This PR just improves debugging, logging, and type checking. I can also confirm that all unit tests pass.

Talking of Python 3.12, this PR uses type hint syntax that is compatible with both Python 3.11 and 3.12, even though Python 3.12 introduced a nice new Type Parameter Syntax. I assume that we still need to support Python 3.11, because:

$ docker run -it ghcr.io/home-assistant/home-assistant:2024.2.1 python --version
Python 3.12.1
pdcastro commented 9 months ago

Still on the subject of Python 3.12, today I updated my Home Assistant to version 2024.2.1 and in the Settings area I saw this popup:

support for python

But I would still use the old type hint syntax (compatible with both 3.11 and 3.12) in integrations like VTherm at least until HASS 2024.4 is released.

jmcollin78 commented 9 months ago

Does you Python dev environment works normally after switching to Python 1.12 ?

I mean, I change the .devcontainer file by added this line: "image": "mcr.microsoft.com/vscode/devcontainers/python:1-3.12",

and all is now broken.

If you don't have done it yet, don't do it. I think it is not stable.

pdcastro commented 9 months ago

Does you Python dev environment works normally after switching to Python 1.12 ?

Yes (Python 3.12, devcontainer image python:1-3.12). Specifically, the unit tests (pytest) work fine and home assistant works fine when started with the ./scripts/starts_ha.sh script:

vscode ➜ /workspaces/versatile_thermostat (expose-keepalive-attribute) $ cat /etc/os-release 
PRETTY_NAME="Debian GNU/Linux 12 (bookworm)"
...
vscode ➜ /workspaces/versatile_thermostat (expose-keepalive-attribute) $ python --version
Python 3.12.2
vscode ➜ /workspaces/versatile_thermostat (expose-keepalive-attribute) $ pytest
Test session starts (platform: linux, Python 3.12.2, pytest 8.0.0, pytest-sugar 0.9.7)
...
 tests/test_auto_fan_mode.py ✓✓✓                                                                                                                                                                      3% ▍         
...
 tests/test_window.py ✓✓✓✓✓✓✓✓✓✓✓✓✓                                                                                                                                                                 100% ██████████

Results (35.15s):
      96 passed
       1 skipped
vscode ➜ /workspaces/versatile_thermostat (expose-keepalive-attribute) $ ./scripts/starts_ha.sh 
...
2024-02-16 13:38:21.800 WARNING (SyncWorker_0) [homeassistant.loader] We found a custom integration versatile_thermostat which has not been tested by Home Assistant. This component might cause stability problems, be sure to disable it if you experience issues with Home Assistant
2024-02-16 13:38:24.534 INFO (MainThread) [homeassistant.setup] Setup of domain logger took 0.0 seconds
...
2024-02-16 13:38:24.627 INFO (SyncWorker_1) [homeassistant.util.package] Attempting install of janus==1.0.0
...
2024-02-16 13:38:25.593 INFO (SyncWorker_3) [homeassistant.util.package] Attempting install of Pillow==10.2.0
...
2024-02-16 13:44:58.434 INFO (MainThread) [homeassistant.setup] Setting up versatile_thermostat
2024-02-16 13:44:58.435 INFO (MainThread) [custom_components.versatile_thermostat] Initializing versatile_thermostat integration with config: {...}

As per timestamps above, HASS took more than 5 minutes to start for the first time after rebuilding the devcontainer (VSCode palette command “Dev Containers: Rebuild Container”), as it was installing dependencies at runtime. After that though, restarting HASS takes only a few seconds.

I'm using VSCode version 1.86.2, Docker Desktop 4.27.2, on a Mac laptop.

Are you getting any specific error messages?

pdcastro commented 9 months ago

Pylance also works for me with "image": "mcr.microsoft.com/vscode/devcontainers/python:1-3.12"

pylance-3 12
jmcollin78 commented 9 months ago

ok thank you for your feedback. So I guess I have exactly the same configuration as yours now. I have tried different base image (like FROM mcr.microsoft.com/devcontainers/python:3.12) and 1-3.12.

The errors are:

I will try with the last commit you do fixing the ffmeg (local Dockerfile) to see if it is better.

EDIT1: with the dockerfile and base image mcr.microsoft.com/devcontainers/python:1-3.11 all seems fine (and the ffmep error disappear 🎉 ).

Vscode Unit tests plugin is broken also:

INTERNALERROR>     ^^^^^^^^^^^^^^^^^
INTERNALERROR>   File "/home/vscode/.vscode-server/extensions/ms-python.python-2024.0.1/pythonFiles/vscode_pytest/__init__.py", line 234, in pytest_report_teststatus
INTERNALERROR>     execution_post(
INTERNALERROR>   File "/home/vscode/.vscode-server/extensions/ms-python.python-2024.0.1/pythonFiles/vscode_pytest/__init__.py", line 679, in execution_post
INTERNALERROR>     send_post_request(payload)
INTERNALERROR>   File "/home/vscode/.vscode-server/extensions/ms-python.python-2024.0.1/pythonFiles/vscode_pytest/__init__.py", line 751, in send_post_request
INTERNALERROR>     raise VSCodePytestError(error_msg)
INTERNALERROR> vscode_pytest.VSCodePytestError: Error attempting to connect to extension communication socket[vscode-pytest]: A test tried to use socket.socket.
Error attempting to connect to extension communication socket[vscode-pytest]: A test tried to use socket.socket.
If you are on a Windows machine, this error may be occurring if any of your tests clear environment variables as they are required to communicate with the extension. Please reference https://docs.pytest.org/en/stable/how-to/monkeypatch.html#monkeypatching-environment-variablesfor the correct way to clear environment variables during testing.

Googling the error, tells me that is seems to be a vscode_pytest issue or configuration. Not clear yet. What is your current Python extension in VSCOde ? Mine is this one : Capture d’écran 2024-02-16 à 16 38 57

Will try now with 3.12

pdcastro commented 9 months ago

”What is your current Python extension in VSCOde ? Mine is this one :”

Same version as in your screenshot – v2024.0.1.

I had always used the terminal command line to run tests, but now that you mentioned running them through the VSCode UI, the following has worked for me:

run_tests

results

I think I’m lucky today, it’s the first time I tried to do this and it just worked! I should buy a lottery ticket. 😄

jmcollin78 commented 9 months ago

![results](https://private-user-images.githubusercontent.com/15091591/305510898-994a3a06-1327-465b-a279-fb01278182cb.jpg?jwt=e I think I’m lucky today, it’s the first time I tried to do this and it just worked! I should buy a lottery ticket. 😄

This is exactly how it was working before. Maybe it is not related to Python or pytest plugin but more on my Mac.

EDIT : I ve got it working with this thread : https://github.com/microsoft/vscode-python/issues/22383

jmcollin78 commented 8 months ago

Hello @pdcastro,

This is not forgotten. I was on the major 6.0 release. The bad news is that it generated conflicts with your development. But I want to see what you have done before trying to merge. I'm not a big fan of generic typing which is a kind of obfuscation for me. Typing should help the reader and not confuse it.

pdcastro commented 8 months ago

This is not forgotten. I was on the major 6.0 release. The bad news is that it generated conflicts with your development. But I want to see what you have done before trying to merge. I'm not a big fan of generic typing which is a kind of obfuscation for me. Typing should help the reader and not confuse it.

Well, I was already forgetting about it! 😄 I’ve now rebased on the main branch and fixed the merge conflict. All tests pass.

As you know, typing (including generic typing) enables “intellisense” that allows things like:

The generic type introduced in this PR:

T = TypeVar("T", bound=UnderlyingEntity)

class BaseThermostat(..., Generic[T]):
...
        self._underlyings: list[T] = []

... means that:

This allows intellisense to figure out that, for example, self._underlyings is a list of instances of UnderlyingValve (and not merely instances of UnderlyingEntity), in usage such as:

thermostat_valve.py

        for under in self._underlyings:
            under.set_valve_open_percent()

Where set_valve_open_percent() is only defined for UnderlyingValve, and not UnderlyingEntity. Before this PR, without the generic typing, intellisense would place a red underline under set_valve_open_percent() in the line above, and when you hovered the mouse cursor over it, it would pop up an error message: _”Cannot access member “set_valve_openpercent” for type “UnderlyingEntity”. After this PR, with the generic typing, the red underline and pop error disappear.

The backwards-compatible generic type syntax for Python 3.11 and older is indeed verbose and may seem obscure if one is not used to it. Python 3.12 makes it nicer. For example:

All recent Python versions (backwards compatible syntax):

from typing import TypeVar, Generic

T = TypeVar("T", bound=UnderlyingEntity)

class BaseThermostat(..., Generic[T]):
...
        self._underlyings: list[T] = []

Python 3.12 and later (also requires the most recent versions of pylint and pyright):

class BaseThermostat[T: UnderlyingEntity](...):
...
        self._underlyings: list[T] = []

So if it is any consolation, when Home Assistant drops support for Python 3.11, which may happen in the next 12 months, the new syntax can be adopted and it will be easier on the eye. 🙂

One final note: The three commits in this PR are reasonably self contained, so you may find it easier to review them separately: Go to the Commits tab at the top of the PR page and click on each of the three commits. (I suppose I could have created separate PRs.) And sorry if I am saying things you already knew! I have some awareness of how annoying this can be. 😄

jmcollin78 commented 8 months ago

This is not forgotten. I was on the major 6.0 release. The bad news is that it generated conflicts with your development. But I want to see what you have done before trying to merge. I'm not a big fan of generic typing which is a kind of obfuscation for me. Typing should help the reader and not confuse it.

Well, I was already forgetting about it! 😄 I’ve now rebased on the main branch and fixed the merge conflict. All tests pass.

As you know, typing (including generic typing) enables “intellisense” that allows things like:

  • Displaying the actual type of a variable when the developer hovers the mouse cursor over the variable.
  • Displaying a list of parameter types and return types for a function call.
  • Auto-complete when typing.
  • Highlighting potential mistakes like typos, assignments of incompatible objects, calling inexistent methods, etc.
  • Enabling tools like pylint and pyright to produce warning / error reports.

The generic type introduced in this PR:

T = TypeVar("T", bound=UnderlyingEntity)

class BaseThermostat(..., Generic[T]):
...
        self._underlyings: list[T] = []

... means that:

  • T is a type that is a subclass of UnderlyingEntity (the bound= part).
  • BaseThermostat makes use of T.
  • T may be redefined to compatible types (like UnderlyingSwitch, etc) by subclasses of BaseThermostat.

This allows intellisense to figure out that, for example, self._underlyings is a list of instances of UnderlyingValve (and not merely instances of UnderlyingEntity), in usage such as:

thermostat_valve.py

        for under in self._underlyings:
            under.set_valve_open_percent()

Where set_valve_open_percent() is only defined for UnderlyingValve, and not UnderlyingEntity. Before this PR, without the generic typing, intellisense would place a red underline under set_valve_open_percent() in the line above, and when you hovered the mouse cursor over it, it would pop up an error message: _”Cannot access member “set_valve_openpercent” for type “UnderlyingEntity”. After this PR, with the generic typing, the red underline and pop error disappear.

The backwards-compatible generic type syntax for Python 3.11 and older is indeed verbose and may seem obscure if one is not used to it. Python 3.12 makes it nicer. For example:

All recent Python versions (backwards compatible syntax):

from typing import TypeVar, Generic

T = TypeVar("T", bound=UnderlyingEntity)

class BaseThermostat(..., Generic[T]):
...
        self._underlyings: list[T] = []

Python 3.12 and later (also requires the most recent versions of pylint and pyright):

class BaseThermostat[T: UnderlyingEntity](...):
...
        self._underlyings: list[T] = []

So if it is any consolation, when Home Assistant drops support for Python 3.11, which may happen in the next 12 months, the new syntax can be adopted and it will be easier on the eye. 🙂

One final note: The three commits in this PR are reasonably self contained, so you may find it easier to review them separately: Go to the Commits tab at the top of the PR page and click on each of the three commits. (I suppose I could have created separate PRs.) And sorry if I am saying things you already knew! I have some awareness of how annoying this can be. 😄

Thank you for all explanations ! I have learn something today thanks to you. Because I'm not a Python native developper I didn't know this.