Closed rthill closed 1 year ago
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.
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.
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)
@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 https://github.com/smarthomeNG/smarthome/issues/27
@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)
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!
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.
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.
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
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?
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
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 ;) )
Note: plugins changed due to lib.network adjustments should all be restartable now (no PR before 1.8)
Any further thoughts / ideas on this topic?
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)
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.
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...
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):
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 ;-)
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...)
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....
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.
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
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.
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
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... )
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
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.