custom-components / pyscript

Pyscript adds rich Python scripting to HASS
Apache License 2.0
892 stars 47 forks source link

Feature Request: Automatic reloading for existing scripts? #74

Closed rytilahti closed 3 years ago

rytilahti commented 4 years ago

While jupyter notebook is a great place for doing the initial development, at some point you want to move your automations out from it. It would be great if (at least the existing scripts) would be automatically reloaded on change, i.e., by using inotify listeners on the existing script files and calling the reload() when a modification is detected.

My personal development setup looks like this: I'm using notebooks to do the initial development, and as soon as I'm confident that things are mostly working, I'll move the code over into a new script. For development, I'm using pycharm which I have configured to do an auto-deployment on the production instance whenever a file gets saved. In such cases, the ability to avoid doing a manual reload service call would simplify the workflow.

craigbarratt commented 4 years ago

Definitely agreed. I always thought having to do a reload service call is annoying. But I wasn't able to figure out a good solution. Checking the mtime every time a function was called is too much overhead (especially since it has to be done in an executor thread). Polling is every worse - 99% of the time it's useless, and then it's too slow when you want it. At one time I was thinking of adding a "developer mode" config parameter that would do mtime checks if set, but it's just not elegant. So I dropped it.

Thanks for bringing up this issue, and proposing an elegant solution. If there is a way to register to receive async notifications of file system changes, that would be terrific. It could then be automated and quite transparent. I'm not familiar with inotify but I'll check into it.

rytilahti commented 4 years ago

Yeah, inotify is the solution, albeit Linux-specific. The listeners will get notified by the kernel as soon as the file gets modified, here's one lib for simple integration: https://github.com/rbarrois/aionotify

craigbarratt commented 4 years ago

Yes, that looks like the right package. But before I use it: 1) I want to make sure if I add it as a requirement for pyscript that it doesn't cause pyscript installations to fail if it's not available or supported on a particular non-linux platform, 2) Assuming it is available and installable on non-linux platforms, I'd need to be sure I can auto-detect whether the os supports inotify, and quietly disable it if it doesn't.

I haven't looked at the code yet, but hopefully it's all vanilla python (so there is no install risk), and it provides mechanism (eg, an exception that I can catch) if it is on an unsupported platform.

dlashua commented 4 years ago

That's what I'm doing in #71. Though, I'm using watchdog which uses inotify behind the scenes.

raman325 commented 3 years ago

FYI there is an integration that could theoretically be used for this purpose: https://www.home-assistant.io/integrations/folder_watcher/

dlashua commented 3 years ago

@raman325 well... that sure makes it easy. I'd never seen this integration before. And now, with pyscript's very smart pyscript.reload handling, this automation is quite easy to setup. Thanks!

raman325 commented 3 years ago

Just learned about it today and thought of this issue. For reference, you will need to add your pyscript directory to the allowlist_external_dirs list in your homeassistant config item. You can then add the folder_watcher integration to watch the folder, and I am using the following automation to trigger a reload (currently reloading everything, it's possible to get down to the file, but I'm not sure the best way to figure out how to selectively reload a whole app:

id: reload_pyscript_on_change_to_folder
alias: Reload pyscript on change to folder
trigger:
  platform: event
  event_type: folder_watcher
condition: '{{ trigger.event.data.folder == "/config/pyscript" }}'
action:
  service: pyscript.reload

Note that I am getting a bunch of warnings that the automation is already running because it's by default set to single mode. I have no idea why multiple automations are getting triggered at the same time, perhaps it's because I am not filtering by event type

dlashua commented 3 years ago

if this integration uses "aionotify" under the hood, I think it has something to do with the way you save files. I use VSCode over SSH and when I save a file, it results in TWO file changes of the same file as detected by aionotify. In Home Assistant, I would modify the automation to look like this:

id: reload_pyscript_on_change_to_folder
alias: Reload pyscript on change to folder
mode: single
max_exceeded: silent
trigger:
  platform: event
  event_type: folder_watcher
condition: '{{ trigger.event.data.folder == "/config/pyscript" }}'
action:
  - delay: 1
  - service: pyscript.reload

This will "debounce" the reload requests by 1 second.

Since pyscript now figures out exactly what needs to be reloaded on it's own, selectively reloading anything in this automation isn't likely needed. The only thing that isn't covered is YAML changes. If you want to reload an app because the YAML configuration for it changed, you'll need to write a YAML watcher (likely in pyscript) the compares the YAML present now to the last seen YAML and if there are any changes, calls pyscript.reload with the global_ctx set to that app in order to force a reload of that app without changes to the app's source files.

raman325 commented 3 years ago

thanks @dlashua this is using watchdog under the hood but it could be the same problem. I will give those adjustments you suggested a shot.

The integration also provides trigger.event.data.file which is the filename of the changed file and trigger.event.data.path which is the full path (folder + file) for the changed file. With some clever templating you could probably handle the YAML use case via an automation outside of pyscript

Have you tested calling the pyscript.reload service from within pyscript? If that is possible, it makes this much easier, I just wasn't sure if that would work

dlashua commented 3 years ago

I currently have automatic reloading setup using a pyscript which called pyscript.reload from within it. So, yes, it works perfectly.

https://gist.github.com/dlashua/f7d88f9a5afdcf7af17ce24266925a0b

raman325 commented 3 years ago

good to know, thanks! I think this can be closed then?

dlashua commented 3 years ago

I think @craigbarratt is considering building reloading directly into pyscript and this tracks that feature request.

craigbarratt commented 3 years ago

Yes, I am planning to support autoload on change. Learning about folder_watcher is helpful. It uses watchdog, which has broader OS support than aioinotify, but I'm guess it has to run in its own thread.

I was thinking of just using aioinotify but then it only works on linux. Perhaps I could use aioinotify on linux and watchdog on other OSes?

dlashua commented 3 years ago

I tested both watchdog and aionotify through userspace pyscripts. Both worked just fine on linux/docker though, yes, watchdog did require a thread. While HASS is not "thread safe" (or, so I think I've read elsewhere) it wasn't much of an issue since I was only getting the notification of change, and calling pyscript.reload. aionotify was easier (mostly due to it not requiring a custom class to handle the change events). But, if you're wanting to support more than just linux, I think you could just use watchdog for everything, even linux, and save yourself the trouble of the multi-pathed codebase. At the very least, write the watchdog portion first, for everything, and then if it causes trouble, add the aionotify path later.

That being said, nearly ALL of the tutorials and documentation I've seen for installing HASS in Windows either uses Docker (which means they are actually running Linux) or a VM (with Linux as the OS). So, for this purpose, I would say there are very few Home Assistant users that are running it under Windows. Even those that run Windows and DEVELOP for Home Assistant, if they use the recommended method, are developing with VSCode using a Dev Container which means that, for Home Assistant purposes, they are running Linux.

On the official installation page, it mentions requiring a "Unix like system".

I think, in reality, the number of non-Linux users will be very small and mostly MacOS.

craigbarratt commented 3 years ago

I decided to use watchdog, even though it uses a thread. I develop on MacOS, so that was the tie breaker for me, plus the fact that it is already used by the builtin folder_watcher component.

Is there any way to detect if the yaml configuration has changed? Watching all the yaml files in or below config sounds a little heavy-handed, especially if HASS doesn't already do something like that...

dlashua commented 3 years ago

Excellent! I'll test this (and the done_callback stuff) later today.

As far as I'm aware, there's not a hook for config changes in Home Assistant because nothing in Home Assistant auto reloads.

Options:

1) manually call pyscript.reload on config change 2) periodically request the yaml and compare to a stored yaml and do something if there is a change 3) at startup and on every reload actually parse the yaml ourselves to look for all !include and friends and point a watchdog at those files

craigbarratt commented 3 years ago

How about it autoloads if any yaml files below config/pyscript change? The idea is you could include those from the main config file. (Can you even split the pyscript config into multiple files?). Or we could just have a single file, eg: config/pyscript/config.yaml.

You don't even need to have the real config in that file; eg, you could just touch config/pyscript/config.yaml to reload the config, although that might be confusing...

dlashua commented 3 years ago

We can do includes within YAML. I do this now. My config looks like this:

pyscript:
  allow_all_imports: true
  apps:
    climateautoaway: {}
    alert: {}
    occupancy_manager: {}
    occupancylock: {}
    calc_setpoint: {}
    calc_conditional_avg: {}
    climatemodesync: {}
    sensors: {}
    hvac_boost: {}
  apps_list: !include_dir_merge_list pyscript_apps_config/
  rooms: !include rooms.yaml
  secrets:
    some_password: !secret some_password

pyscript_apps_config is a directory full of files (the names are not important) that contain a yaml list like this:

- app: climatemodesync
  leader: climate.hvac_down
  follower: climate.downstairs

- app: climatemodesync
  leader: climate.hvac_up
  follower: climate.upstairs

- app: calc_setpoint
  sensor_temp: climate.upstairs.current_temperature
  actual_temp: climate.hvac_up.current_temperature
  desired_temp: climate.hvac_up.temperature
  climate_entity: climate.upstairs
  current_setpoint_entity: climate.upstairs.temperature

- app: calc_setpoint
  sensor_temp: climate.downstairs.current_temperature
  actual_temp: climate.hvac_down.current_temperature
  desired_temp: climate.hvac_down.temperature
  climate_entity: climate.downstairs
  current_setpoint_entity: climate.downstairs.temperature

If /config/pyscript/config.yaml was monitored and auto-reloaded that would work perfectly. In that case pyscript.config should likely point to that data which would be a breaking change.

We could also monitor ALL files under /config/pyscript/ and key them based on the filename:

/config/pyscript/config.yaml -> pyscript.config.config /config/pyscript/scripts/config.yaml -> pyscript.config.scripts.config

It's a little bit ugly, but more flexible. And we could likely come up with better naming.

dlashua commented 3 years ago

Also, having a "required" file for config is a known pattern in Home Assistant. For instance, Native automations can be anywhere we want. But, if you want to use the UI Automation Editor you need...

automation: !include automations.yaml

...in your config.

So, requiring that, for autoreload, your pyscript YAML looks like this:

pyscript:
  config: !include pyscript/config.yaml

Isn't something Home Assistant users are foreign to. It does mean I'll have to change how I do YAML (since pyscript's YAML file watcher probably won't handle a second include in config.yaml and watch that file too). But I can manage.

dlashua commented 3 years ago

I have tested reload functionality in a basic way and everything seems to work perfectly. It'll get a bigger test in the near future as I've now turned off my own custom reload script and rely on it pretty heavily as I make tiny changes.

craigbarratt commented 3 years ago

I've added reload upon any yaml file changes below pyscript. That will re-parse the main config and only reload apps whose config has changed.

I'd rather not impose any structure on the yaml files, so users can layout things however they like. I'll add something in docs for your suggestion of putting the pyscript config in pyscript/config.yaml via an include.

dlashua commented 3 years ago

I've tested this recent change and it works perfectly.

This is the best configuration I've found for working with this new feature:

configuration.yaml:

pyscript: !include pyscript/pyscript.yaml

pyscript/pyscript.yaml

allow_all_imports: true
apps: !include_dir_named apps_config/
rooms: !include rooms.yaml
secrets:
  some_password: !secret some_password
test: abcdef

pyscript/apps_config/NAME_OF_APP.yaml

specific_app: config_goes
in_this: file_here

If you need to include an app called "my_app" that doesn't actually need any config, just create a blank file called "my_app.yaml" in the "pyscript/apps_config" directory.

craigbarratt commented 3 years ago

Sounds great. I do need to add one more feature before closing this issue: if allow_all_imports or hass_is_global change when the config is reloaded, then all scripts/apps should be reloaded, since those parameters could affect all scripts/apps.

craigbarratt commented 3 years ago

This last commit should make auto-reload feature complete

dlashua commented 3 years ago

That's a good idea. I tested this and it works perfectly.