plugin management (writing properties to XML) should be lazy rather than eager.

[timobaumann]

At present, we initialize all plugin runtimes before starting a dialog model. It would be wise to only load those plugins that are actually needed for the model. This would resolve issues like https://github.com/dialogos-project/dialogos/issues/177 and boost startup times for people with many plugins installed.

[alexanderkoller]

That would indeed be cool.

It would require some rethinking of how plugins are loaded and initialized. For instance, we'd have to keep track of the plugins which were previously initialized, and we'd need to insert something like the DialogOS startup screen into the code that loads dialog models, so information about plugins being loaded can be displayed as needed.

[alexanderkoller]

Also, we need to partially initialize the plugin at DialogOS startup time so its nodes can be displayed in the node palette and added to the dialog.

[timobaumann]

Hi, I was thinking only about the PluginRuntimes, which is not necessarily the same as the actual plugin (but would be the thing involved in connecting to some outside API, I guess).

I've implemented that now (not beautifully) in the most recent commit. I'm currently using Graph#getNodes(). If someone were to implement search of reachable nodes that would be highly appreciated (whether BFS or DFS...).

Loading the plugin itself is yet another issue: we write all plugins that are loaded into the XML file and then try to load the plugin when reading the file. Thus, reading a file from someone who has more plugins installed than you do, leads to problems.

I don't know how plugins get into the XML initially.

[timobaumann]

can there be plugins that do something useful without defining their own node types?

[alexanderkoller]

I don't think plugins without their own node types are useful.

I quite like your solution of initializing the runtimes of only those plugins that have nodes in the graph. Perhaps there is a slightly cleaner way in which the default implementation of isRelevantForNodes can look at the nodes that are defined by that plugin. The node classes are registered in the constructor of the plugin class, iirc, so they are probably stored somewhere.

I'm a little worried about the reachable nodes, because it adds a layer of complexity to the whole thing. Would this make a big difference in practice? E.g. when you develop ROS dialogs, do you (or would you) delete in-edges of ROS nodes to speed up startup time? I find it hard to imagine that people would systematically "uncomment" parts of the dialog during development by deleting their in-edges.

[akoehn]

Could a plugin which only provides more classes to the classpath be useful? E.g. you want to use a berkeleyDB in your scrips and don't want to build this into its own node type?

OTOH, these could also just be thrown into the classpath and used without being encapsulated in a plugin.

[alexanderkoller]

Interesting point. Such functionality couldn't simply be thrown into the classpath, if the plugin has configuration that is set up in the DialogOS GUI and stored in the dialog file.

This seems to be so much at odds with the design philosophy of DialogOS, though, that I would consider simply ignoring that case until someone has an actual need for it. Also, Timo's current solution has a method that checks whether the plugin runtime should be initialized when the dialog is executed. A hypothetical plugin like the one your describe could simply overwrite that method with return true to declare itself as always relevant.

[akoehn]

Yes, I am not arguing that this is important functionality, it was just the only possible use-case that came to my mind. Also, it does not need any initialization so the lazy loading would still be valid.

[timobaumann]

the PluginRuntime initialization is lazy now (at the discretion of each plugin, or rather it's PluginSettings). We should still only write a plugin into the XML if it's relevant. Ideally: only write settings to XML that are not the default anyway -- although defaults could change over time...

[timobaumann]

property writing to XML is now lazy. The implementation currently uses isRelevantForNodes() (as implemented previiously). This means that:

  WARNING: This change can potentially lead to data loss: imagine you have stored many G2P rules. 
           If you then save the dialog model that does not contain any SphinxNode, the G2P rules will not
           be stored in the file (because the plugin assumes that it is not relevant for the given file).

We could potentially also implement a hasRelevantStorageInformation():boolean which could default to isRelevantForNodes. I've opted against this for the moment.

@alexanderkoller : Please make sure that Lego and Alexa plugins implement isRelevantForNodes in their PluginSettings. See e.g. dialogos/plugins/DialogOS_SphinxPlugin/src/main/java/edu/cmu/lti/dialogos/sphinx/plugin/Settings.java for how it's done. Thanks.

[alexanderkoller]

Shouldn't PluginSettings#isRelevantForNodes just always return true, so that all plugins that don't automatically disable themselves are initialized by default?

[alexanderkoller]

Or maybe it already does? If yes, then why would we need to make changes to other plugins?

[timobaumann]

The default implementation returns true, indeed.

this means that every plugin that does not override this behaviour:

(a) the plugin will be initialized (PluginRuntime) at every dialog startup which impacts performance if you have many plugins installed (but are only using few of them)

(b) the plugin will be written into the XML. As a result other users (which do not have all these plugins installed) will be unable to open your dialog model. In other words: You have LEGO plugin installed, but don't use it in a dialog.dos which you share with me. I don't have that plugin installed. As dialog.dos doesn't use the plugin, we'd expect everything to work smoothly. However, it only works smoothly if the LEGO plugin declares that it's not relevant for dialog.dos so that it's not written into the file.

I think we should not publish plugins as part of our distribution that are not reasonable wrt. isRelevantForNodes. At the same time, the present default makes life easier for external plugin developers.

[alexanderkoller]

(a) is only relevant in practice if initialization takes time, which is not the case for most plugins, I think.

(b) is a much more serious issue. Is this actually true? That would be terrible, and a really good reason to implement isRelevant methods for all plugins.

[alexanderkoller]

Actually, such a good reason that I wonder if the default implementation of isRelevant shouldn't do the nodes checking by default. The only reason why we wouldn't do this is so we don't lose data like the G2P you mentioned above - which could be handled by an SphinxPlugin#isRelevant implementation that returns true more often than the default implementation.

[timobaumann]

there's no mapping of what plugin defines what nodes. so only the nodes can check this. (Of course, this could be changed, but I don't see why.) Again, I really don't see the issue of implementing this in the plugins. It's mostly a one-liner.

[timobaumann]

as it happens, at present missing plugins are silently discarded. I'm about to change that because the downstream error that presently happens is that node types aren't found. It's impossible for users to figure out why their file doesn't work based on some node type being missing. whereas they might be lucky if we can point them to the fact that the might be missing a plugin.