make TWCManager.py a module and make MQTTControl, WebIPCControl, and MQTTStatus optional

[ngardiner]

make TWCManager.py a module and make MQTTControl, WebIPCControl, and MQTTStatus optional

[ngardiner]

I'd happily merge a PR that would check for the existence of the various modules before loading them and, if they don't exist, skipping the loading of that module (or perhaps just trapping an exception if they don't exist?). I think it's a good idea.

[AndySchroder]

Was planning to trap the exception when importing the MQTTControl, WebIPCControl, and MQTTStatus modules, and then after that, wrap calls to the modules in if statements that check if the MQTTControl, WebIPCControl, and MQTTStatus module exists.

For TWCManager.py, I will put the main loop into a class that is another thread. When running TWCManager.py standalone __name__ == "__main__" will be True, so I'll use that in an if statement to startup the thread. Otherwise, if importing as a module, whatever is importing TWCManager as a module will be required to startup the thread.

Will work on this and share later this week.

[MikeBishop]

Certainly it would be nice to only need to install the dependencies if you plan to use those modules, though that's less of a pain now that we have the installer setup.

[AndySchroder]

Okay, so I did not realize that you added the modules_available list in TWCManager.py. If I comment out the lines for MQTTControl and MQTTStatus, these do not even attempt to load. However, should this choice just be moved to config.json? In config.json, there are enable/disable values for things. Why not just read that and not even try to load if disabled?

[ngardiner]

These are all just different approaches to the same outcome. Currently, a module which is not enabled is loaded at boot, and then code within that module makes the decision as to whether it will function based on the settings in the config file.

In an upcoming update, I plan to unload modules when they are disabled by removing them from the modules list, but that code will remain in the modules themselves. They will of course call a central function to do the deregistration but the decision to do so would be encapsulated within the module. The reason this code is encapsulated within the module is that there is no one universal way to enable or disable modules, and in some cases disabling a module may not make sense.

There's no criteria that says a module must have an enable/disable switch however disabling one is simply a matter of removing it from the list. Different evaluations for different modules may lead to deregistration - eg I may choose to deregister a module because it is enabled but none of the parameters critical to the functioning of the module were passed.

Rather than doing all of that evaluation within the main module, it can be encapsulated within the modules themselves.

[MikeBishop]

If the module namespace within the config file were standardized, we might skip loading modules that don't have a config element at all. Comment out anything that's not almost-always needed and let people uncomment what they need.

[ngardiner]

I would think it comes down to a trade-off. At the moment we have the ability for users to enable or disable modules where it makes sense, but without the need to explicitly enable/disable every module (such as the Policy module), and the PR submitted by @AndySchroder will also trap the non-existence of a module and not fail should it not exist, which means we will have quite robust handling of module loading.

What is missing is the small amount of logic per module which requires it which effectively says:

if self.config.get('enabled', 0) != 1:
  self.master.unload_module(me)

The alternative is to make this explicit for each module in the config file. You would save the code above by putting extra code into the main module, but the user would need to enable each module and there's no automation for situations where a module detects itself as being accidentally loaded or misconfigured and unloads itself, or where an Interface module realises that another one has already been loaded and unloads itself. It also means if we add any modules to the codebase in the future, every user needs to update their config file to account for it.

For me I just wouldn't see a strong enough benefit in standardising the behaviour for all modules vs automating it. I feel it would make the structure too rigid, making further modularization in the future a challenge.

Also, the behaviour above is exactly what the user experience would be if we went ahead with module auto-unloading. If the relevant config component was commented out by the user, the modules initialization routine would detect this and immediately unload. I don't doubt theres a small cost to this approach in load time but the concept of it is based around module autonomy - allowing the module to decide what's best, especially if we want to offer the ability for an author to contribute a modules and effectively make it self-encapsulated.

[AndySchroder]

How is it possible for a module to unload itself? What is the motivation for a module unloading itself? I would think you would just have the logic occur before the module is ever loaded and decide then if it is a good idea to load the module.

Either way, I don't have a problem leaving this issue open, as long as you merge the pull request that I've submitted. I think what I've added should still be in there even if there is also an option to intentionally not use modules that are already installed. That additional feature can be added later if there is motivation.

[AndySchroder]

Also, please see the commented I added here #96 . I think there may be a small error in my proposal.

[ngardiner]

How is it possible for a module to unload itself?

Technically it doesn't unload itself, it requests it. Unloading a module in python is a matter of removing all references to it, so the GC routine cleans it up. There should be 2 references to it, one in master.modules and one in sys.modules. Removing these references should cause it to be removed.

There is more to this as well. It is not necessarily just a matter of removing a module. The same logic can be applied to reloading a module too. For example, imagine the web interface has configurable options for a module, and on changing them, we want to restart a module to pick up the changes. This can be done by deleting the reference to a module and reloading the module. It's a nice powerful way of leveraging the fact that the code has been modularised.

I would think you would just have the logic occur before the module is ever loaded and decide then if it is a good idea to load the module.

Well in a perfect world this would be the solution. Unfortunately plenty of things about the current state are not perfect and not enough time exists to fix them all, but the discussion, and I intend it to be a discussion and not a dictate, is that if we do fix something, we should take the time to choose the best approach, because once we do it we are stuck with associated limitations going forward.

Either way, I don't have a problem leaving this issue open, as long as you merge the pull request that I've submitted.

:) I am happy with the proposal but just need you to make a few tweaks to the code which I have commented on #96. Should be ready to merge soon.

[AndySchroder]

Please see this comment and commit #96 (comment) . I have a proposal to resolve Part 2 of this issue, which is making TWCManager.py importable as a module.

[AndySchroder]

Today I've released my project, Distributed Charge, which needs TWCManager to be importable as a module.

http://andyschroder.com/DistributedCharge/

Hopefully this example will provide some context to where it may be useful to import TWCManager as a module. I think both projects can work and evolve in a very synergistic way, so I hope you'll consider merging my pull request.