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

Feature request reload plugins #73

Closed
rthill opened this issue Jul 11, 2016 · 25 comments
Closed

Feature request reload plugins #73

rthill opened this issue Jul 11, 2016 · 25 comments
Labels
core Issue relates to the core of SmartomeNG feature request
Milestone

Comments

@rthill
Copy link
Contributor

rthill commented Jul 11, 2016

I wonder if someone was thinking about implementing a reload plugins action?
Currently the CLI module allows reloading logics. Wouldn't it be interesting having the same for plugins?

This feature would make it easier to update plugins and keep smarthomeNG core running.

@cstrassburg cstrassburg added this to the Version 1.3 milestone Jul 11, 2016
@bmxp
Copy link
Member

bmxp commented Jul 11, 2016

Yes, it would be a good idea to make plugins loadable on the fly. The Backend module allows for reload and trigger of logics as well.
It would also be nice to create items on the fly and delete them as well. Since plugins might need to be able to create items on the fly as well, this step should be first prior to reloading plugins.

@msinn
Copy link
Member

msinn commented Jul 12, 2016

For the time being, the Backend plugin implements the reload of logics in the same way as the CLI plugin does. It reloads only the Python code. It does not reload scheduling information from etc/logics.conf.

Before starting to make plugins reloadable, I think its a good idea to do a complete implementation of the logic reload including the reread of the configuration.

@ohinckel
Copy link
Member

do a complete implementation of the logic reload including the reread of the configuration.

I have this feature already implemented in my local custom branch (reloading of logics, reloading of plugins - including reloading the configuration). But this implementation depends on some other features like putting configuration into different sources (files, database, ...).

Since we also want to support different sources for configuration I could start to create one PR for this feature (support more different source formats, which will have the current supported one implemented).

In the second step I could create a new PR including the reloading implementation.

After all we could
a) easily extend to other configuration formats (like reading from a database or file in JSON format) and
b) be able to reload plugins and logics (providing functionality in CLI and backend)

@cstrassburg cstrassburg self-assigned this Jan 10, 2017
@msinn msinn modified the milestones: Version 1.4, Version 1.3 Aug 6, 2017
@bmxp
Copy link
Member

bmxp commented Aug 22, 2017

@ohinckel Maybe you should discuss this with @msinn since a major step will be the dynamic plug and unplug of plugins. This way we could also care about #27

@msinn
Copy link
Member

msinn commented Aug 22, 2017

@bmxp I had reloading of plugins on my list for a long time. But like for features I was planning, @cstrassburg said he was working on it, so I stopped planing for those features.

But since @cstrassburg has little/no time to spare, I restarted planning dynamic unload an load of plugins.

I am even thinking in the direction of #27 (Checking plugin parameters against a metadata definition)

@bmxp
Copy link
Member

bmxp commented Oct 10, 2017

Just to have an update on this issue: Within 1.4 dev as of oct. 2017 the feature of creating new logics, editing and saving is implemented by @msinn into the core and the backend modified by @psilo909 provides an interface to do so. Thanks for your excellent work so far!

@msinn
Copy link
Member

msinn commented Oct 10, 2017

Besides implementing the logics-api in the backend plugin I updated the cli plugin too. It uses the logics-api and has a new command to display extensive information about a logic.

@msinn msinn modified the milestones: Version 1.4, Version 1.5 Oct 16, 2017
@msinn msinn modified the milestones: Version 1.5, Version 1.6 Apr 22, 2018
@msinn
Copy link
Member

msinn commented May 5, 2018

It is somewhat complicated and the plugins are mostly not written to be reloadable.

I am not sure, we manage a reload ability for version 1.6, but we are working on it.

@msinn msinn modified the milestones: Version 1.6, Version 1.7 Jun 24, 2018
@msinn msinn modified the milestones: Version 1.7, Version 1.8 Mar 10, 2019
@msinn msinn added the core Issue relates to the core of SmartomeNG label Mar 26, 2019
@bmxp
Copy link
Member

bmxp commented Jan 31, 2020

As I recall of a recent gitter communication mostly plugins that utilize threads are a problem for a restart. There is already an entry restartable in every plugin.yaml.
If a stop/restart button would be implemented in Admin Interface, every Plugin author could at least check if his plugins really is able to act accordingly.
There are plugins however that use threads in a third party library (e.g. telegram) It depends somewhat on the timeout settings of those third party threads if a stop works. Maybe be can elaborate this further soon

@msinn msinn modified the milestones: Version 1.8, Version 1.9 Feb 4, 2020
@msinn msinn modified the milestones: Version 1.9, Version 1.10 Dec 24, 2020
@Morg42
Copy link
Member

Morg42 commented Jan 1, 2021

Would it be an acceptable way to extend the stop() methods to do a more thorough cleanup, and perhaps set a flag self._stopped, so calling run() again could re-invoke init()?

Or would a separate method restart() with possible cleanup and re-initialization be a better way?

@msinn
Copy link
Member

msinn commented Jan 3, 2021

Would it be an acceptable way to extend the stop() methods to do a more thorough cleanup

What do you mean by that?

and perhaps set a flag self._stopped

That already exist -> self.alive

calling run() again could re-invoke init()?

Not a good idea. Initialization is what should be done at load time.
There are some plugins that can be stopped and restarted. Those Plugins open connections on run() and close connections on stop(). Important is to open the connections on run() and not on init()

On the road to reloadable plugins, the first step would be stoppable plugins. Those plugins should be able to

  • open connections on run()
  • close connections on stop()
  • to noch change any items while not running (which implies that those plugins should not modify items on startup until run() is called and self.alive is set tu True

@Morg42
Copy link
Member

Morg42 commented Jan 3, 2021

Would it be an acceptable way to extend the stop() methods to do a more thorough cleanup

What do you mean by that?

Many plugins I have seen just these days don't care for "cleaning up", that is, closing connection, deleting schedulers, anything. All in all, when Python quits, it's all done, right? (...)

So - as you said yourself in your last message - the first step should be to rework the stop() methods to be more thorough cleaning up the plugin environment.

and perhaps set a flag self._stopped

That already exist -> self.alive

self.alive would't tell if the plugin already had been running or was just initialized. But with a proper init/run/stop triple this shouldn't be necessary

calling run() again could re-invoke init()?

Not a good idea. Initialization is what should be done at load time.

Jein (is there something adequate in English? :D )

If I assume that restarting plugins (sooner or later) also means reloading item configuration "on the fly", then it might not be enough to re-run parse_item() for all items; also old item definitions and associations need to be removed beforehand. Also, maybe a plugin reload needs to load an (updated) command set or device definitions.
These jobs are classically done in init() and would be skipped if the plugin was only restarted using stop() and run(). Not being able to change plugin configuration or update item definitions pretty much obviates the need for restarting plugins, in my view.

So this needs a bit of thought. Split init into "pre-item-load" and "post-item-load", where post-item-load initialization need to be reversible (cleaning up 'registered' items -> also in shng's update_item() caller), or you just dump (unload) the whole plugin and load (import?) it again. This could be done without changing plugins, because it would need to be done by shng. Not sure if this is possible.

There are some plugins that can be stopped and restarted. Those Plugins open connections on run() and close connections on stop(). Important is to open the connections on run() and not on init()

Agreed. Bad habit. Maybe we need to check this as a kind of quality control on accepting new plugins into develop? I know that at the moment only a few people can write to the shng repos (which should stay that way), but if there was a checklist to go along with the plugin, the author could beforehand check if all "required" marks are checked.

Seeing that many plugin authors do not have much experience in programming, even less doing so to certain formal standards, this could be a way to start getting "better" code, meaning conforming to certain standard like "open connections in run, not before", "don't change items if self.alive == False", "close all connections / files on stop()" and so on.

(Incidentally, these were also you last words... good to see we agree ;) )

@Morg42
Copy link
Member

Morg42 commented Jan 3, 2021

Note: plugins changed due to lib.network adjustments should all be restartable now (no PR before 1.8)

@msinn msinn modified the milestones: Version 1.10, Version 1.11 Nov 30, 2021
@Morg42
Copy link
Member

Morg42 commented Feb 17, 2022

Any further thoughts / ideas on this topic?

  • Is there a feasible / planned way to un-register item callbacks (update_item)?
  • can we undo / redo plugin configuration (esp. items)?

I would be willing to start and try something in this direction, if we can agree on a way to go, on a kind of structure (see my comments above, pre-/post-item-init)

@bmxp
Copy link
Member

bmxp commented Sep 9, 2022

We should consider to implement a way to start and stop plugins soon in a predefined way. With the new implementation of lib.network there are plugins in the wild that try to connect to devices that are frequently not in use like e.g. multimedia devices, tv sets, computers etc.

@Morg42
Copy link
Member

Morg42 commented Sep 9, 2022

Most of this is implemented (or prepared for implementation) in #477. So far, this didn't get much discussion.

Admittedly, I've not yet progressed far with describing this outside the code...

@jentz1986
Copy link

I am wondering if there is a list of problems to be solved to come to a state where everyone agrees "this feature is implemented". From what I can see in the discussion here it feels like "All plugins must be changed" and "we have to wait for dynamic item reloading", "besides: what about threads in plugins". This is meant by no means offensive, what I try to transport is: all these statements try to 'boil the ocean'. What I miss is the identification of the next 'baby-step' - and the request: "If we had this, then it would allow us to learn something from it for the next iteration."

For me a roadmap could potentially look as follows (incomplete, and definitely partially matching some of the thoughts mentioned before):

  • document the intended lifecycle of a plugin "as if reloading would already have been implemented" e.g. 'run' is called not only after SHNG is started, but as well as part of the start during a re-start (cave: future feature!). Outcome: Plugin Developers can check their work against the documentation and consider "restart-effects" now.
  • Add list of aforementioned Quality-checks to the Plugin Development Guide Outcome: Plugin Developers can check their work against the documentation - and be pointed there in case of "what do I have to do?"
  • consider all 'reloadable' plugins to be able to clean their resources on 'stop' and to adhere to the lifecycle above
    1. init (=load items)
    2. run (=start interaction)
    3. on_* (=perform actions)
    4. stop (=clean up everything)
    5. run (=start interaction)
    6. stop (=clean up everything)
    7. del (=Python is shutting down))
  • Ignore the list of gaps in the plugins for the moment, but make the plugin folks aware!
  • identify actions for the dynamic first launch of a plugin during runtime and add them to the core implementation for 1.10(?)
    • load the plugin module, along with their requirements (should be fulfilled already, aren't they)
    • run 'init'
    • run 'run'
  • identify actions for the dynamic shutdown of plugins and their clean-up and add them to the core implementation for 1.11(?)
    • 'stop' the plugin
    • 'del' the instance
  • enable an optional method 'reload_items_and_logics' for the SmartPlugin class, that would allow the plugin to reload the items' and logics' data for 1.12(?) to allow plugin-developers to react on changes of items and logics.
  • the dynamic items configuration can be applied now in two ways:
    • via complete shutdown + dynamic launch
    • via 'reload_items_and_logics'
  • unsolved challenge: how to deal with a changed codebase - reloading modules, potentially answer 2 might help here: (https://stackoverflow.com/questions/45405600/how-to-reload-all-imported-modules)
  • Identify actions that need to be added to the lifecycle for dynamic loading of remote plugins and add them to a "pro-mode" of the UI in core for 1.13(?)
    • get plugin files from origin (git-repos)
    • get plugin requirements (via pip)
    • point users to "now you have to enable the plugin" --> dynamic first launch of a plugin
  • unsolved challenge: How to deal with incompatible / contradicting requirements?

Is there any better way of clustering this? I am getting used to the Scaled Agile Framework with Epic->Feature->PBI->Task in my professional context, which helps to divide big problems into doable actions for developers. Please do not understand this as an "I know better"-statement. I want to make my thought process transparent and I offer my support, if you provide a starting point for me ;-)

@Morg42
Copy link
Member

Morg42 commented Oct 15, 2022

Wow, lots of inputs ;)

I am wondering if there is a list of problems to be solved to come to a state where everyone agrees "this feature is implemented".

When speaking of "make plugins reloadable" - no, not yet, as not all conditions and features are decided on.

We are more or less agreed that before someone works on plugins to be restartable, one of the most important features will be dynamic item editing/reloading, so restartable plugins should be able to reload/reparse items.

Lots of work in this context already has been done (I'm aware of #410, some work based on this issue #73, and #477), and waits to be assessed (which probably depends on me writing the docs beforehand... )

From what I can see in the discussion here it feels like "All plugins must be changed"

No, at least my plan is to adjust class smartplugin with maximum backward compatibility, and in a way that ignoring (not adjusting code to) new features, it still runs as before but cannot profit from new features. Again, docs...

and "we have to wait for dynamic item reloading"

See above. Both are - to a degree - interdependent because of structural changes to the modular setup.

"besides: what about threads in plugins".

Implemented by some plugins and working, with minor inadequacies mostly on shutdown (which are taken care of by Python)

This is meant by no means offensive, what I try to transport is: all these statements try to 'boil the ocean'.

No, not really. If you dare, try to follow the proposed changes in #477, I'm welcoming any feedback (aside from "write the f... docs" :)

For me a roadmap could potentially look as follows (incomplete, and definitely partially matching some of the thoughts mentioned before):

This is my work-to-be-done, I guess - much of the code is already written but lacks documentation (description of work as well as description of goals, which would make up most of your roadmap).

  • unsolved challenge: how to deal with a changed codebase - reloading modules

If by modules you mean plugins, this is a problem I had already implemented in my md plugin project, which has been scrapped; still, the "way to go" is understood and implementable. However, I'd strongly advise against reloading arbitrary modules, only enabling to manually reload (recompile) single plugins on demand, mainly for developers.

This is not meant to say "you're wrong", but I guess we look at the same problem from different angles, this is mine (as limited as my plannable available time for coding (or writing docs) at the moment...)

@bmxp
Copy link
Member

bmxp commented Oct 15, 2022

Would you please be so kind to include a kind of suspend() and activate() function within your thoughts?
There are plugins that need a suspend sometimes, just think of receivers that are switch controlled and are offline suddenly. One would like to not have a couple of errors within the logs.
When you rework the plugin please have this in mind....

@jentz1986
Copy link

Suspend() + activate(), isnt this the same as stop() + run()? At least in my plugins I had used the methods in this way, for exactly that purpose - a device that goes offline due to a hard power-off.

@bmxp
Copy link
Member

bmxp commented Oct 15, 2022

I think it is not. suspend() means the plugin will neither take action nor respond but still has the ability to listen to the changes for items etc. Its just not querying actively.

stop and run normally will also end the plugins thread I think which is not wanted

@Morg42
Copy link
Member

Morg42 commented Oct 15, 2022

If the plugin neither acts on item changes nor queries the associated device, what does it do in the meantime?

I understand stop() not to "clean up", but to "stop interaction", and del() will have to clean up, so run/stop can be cycled ... "carelessly", without having to bother with cleanup/init.

@bmxp
Copy link
Member

bmxp commented Oct 15, 2022

Imagine knx as readonly. It listens but does not take action.
Or russound: It listens to the receiver but gives no commands and thus does not raise errors, but will detect when items change that should probably be sent after plugin is not suspended any more

@Morg42
Copy link
Member

Morg42 commented Oct 15, 2022

Hm... so for non-present network devices (media player e.g.), start/stop would be the way to go as @jentz1986 mentioned; and you want an additional "mode"?

Should "probably be sent" be deterministic or based on - which - criteria? How long should item changes be stored/held back, if suspend takes a day or a week?

(trying to image a use case... )

@Morg42
Copy link
Member

Morg42 commented Feb 23, 2023

Start/stop(/restart) is implemented, adding and removing items to/from plugins and parsing/unparsing items, as far as the core/smartplugin class is concerned.
Next step for the core team would be removal of items from (running) shng (see #520) and adding items to shng at runtime.
Next step for developers is implementating this in their plugins, see lib/model/user_doc.rst for documentation.

Will open new issue to adjust plugins and track progress. @Morg42

Will also try to implement a suspend/resume function if precise instructions (what may/may not happen during suspend) are supplied - recommend opening a new issue for this @bmxp

@Morg42 Morg42 closed this as completed Feb 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Issue relates to the core of SmartomeNG feature request
Projects
None yet
Development

No branches or pull requests

7 participants