Improve loading of plugins

[astrofrog]

In #578 and #585, I tried to improve the loading of plugins so that the loading itself is done from the plugin directories, and the only thing outside is that GlueApplication calls load_all_plugins. I'm still not quite happy with it, so this issue is meant to be a reminder that we need to improve this a little.

In particular, for example the link helpers in #578 can get registered just by importing the plugins directory, whereas the ginga plugin has to be explicitly loaded, which doesn't seem very consistent.

[astrofrog]

<brainstoming>

At the moment, we have a bit of a mix of ways to load plugins:

• the spectrum tool and pv slicer are included in a static method of ImageWidget
• the ginga plugin requires load_ginga_plugin to be called, so the GlueApplication does that when starting up

Neither are ideal in my opinion. The first is not ideal because the main glue code should not have to know about plugins, so it's up to the PV slicer and Spectrum to declare themselves

The second is not ideal either because at the moment there is no way to tell glue to NOT load those plugins.

One solution I've been thinking about is to instead have a registry of plugins, which would be identified by a string giving the module name, like sphinx does with extensions. The user could then add/remove these in their own config.py before they are actually loaded.

However, this is still not perfect, because the question is, at what stage should the plugins be loaded? That is, the coordinate_helper plugin is needed for stuff in glue.core, but the ginga plugin is only needed for glue.qt and should not be loaded if only glue.core is loaded. Sow how do we figure out when to load what plugin? We could also say that plugins are only loaded when the application is started, so someone using glue.core only and wanting access to the coordinate helper plugin would need to load that plugin explicitly:

load_plugin('glue.plugins.coordinate_helpers')
from glue import core

</brainstoming>

Ok, so one option is:

• Create a new registry of plugins, where plugins are identified by strings (e.g. 'glue.plugins.coordinate_helpers', like what Sphinx does.
• Create a load_plugin function that given a string referring to a plugin, will 'load' it, whatever that means for each plugin.
• If the user wants to just use parts of glue (e.g. glue.core, or import just one widget), they have to load plugins explicitly, e.g.:

load_plugin('glue.plugins.coordinate_helpers')
from glue import core

• If the user launches the glue app, then this will automatically load all plugins in the plugin registry.

An alternative option I just thought of is:

• use the existing registries, but allow entries that are just a string of the form 'plugin:glue.plugins.coordinate_helpers'. Then when members is accessed, load all plugins in that specific registry. So plugin tools are only loaded when the tool registry is accessed. To make it cleaner we could also have all registries take a method add_plugin('glue.plugins.coordinate_helpers') so we don't need to use add and therefore don't need the plugin: thing to avoid confusion

This avoids the creation of a new registry, and allows loading as needed.

@ChrisBeaumont - sorry for the brain dump, but do you have any thoughts on this?

[astrofrog]

I'm going to try and whip up a PR based on my alternative suggestion since I think it would be easy, just to see.

[astrofrog]

I've attached pseudo code here to demonstrate what I think is my preferred option for now.

[ChrisBeaumont]

Agreed that this should be standardized somehow.

If at all possible we should prevent the user or plugin developer from having to worry about import logic. The registry definitely feels like the right place for all hooks to live, and ideally the registry API is a single hook (ie the decorator/add method).

I wonder if each registry group should define when it should be loaded?

[astrofrog]

@ChrisBeaumont - see attached code, where the plugins are added on a per-registry basis, and each registry loads its own plugins only when members is accessed. Is that what you meant by each registry group should define when it should be loaded?

[astrofrog]

This works nicely by the way - the ginga widget is only loaded at the point of dragging a dataset into the main window! The exporters are loaded sooner because that registry must be accessed sooner.

[astrofrog]

However, the plugins are registered before the user's config.py is parsed, which allows the user to remove any plugins if needed.

[astrofrog]

One final thing - from the plugin developer's point of view, they just need to define a load_plugin function that adds things to the registry as normal, and then also add an entry in register_plugins if the plugin is going in glue.plugins, otherwise when using the plugin they just need to register it (we can provide docs for this).

[ChrisBeaumont]

So is the idea that someone who has their own plugin would manually call the appropriate registry.add_plugin from their config.py?

[astrofrog]

In this approach, yes, or they could directly load it (see below)

[astrofrog]

Adding an item to a registry using add is the same as it was previously, and it just adding normal entries.

Registering a plugin add_plugin tells the registry that there is a plugin that needs to be loaded in order for the members attribute to be complete. The first time members is accessed, any plugins are loaded (see next item). In that sense, a plugin entry in the registry is a pointer to deferred registry entries.

Loading a plugin means actually adding the proper entries to a registry (note that when the registries load plugins, they remove the plugin entry, so they are loaded only once)

Registries can contain two kinds of things - normal members entries (in _members) and plugins (in _plugins). Plugins are always just strings pointing to a sub-package. The first time members is accessed, all plugins in _plugins are loaded, _plugins is emptied, and loading the plugins adds entries to _members.

To summarize what happens inside glue: as soon as glue is started up, we register all built-in plugins internally using add_plugin. Glue then loads the user's config.py file which can for example remove plugins from the registries (still need a proper mechanism for that that doesn't involve touching _plugins since it is private - maybe I can just make it public). They can also register their own plugins. Then, in glue, if you access qt_tools.members, it will then first load all plugins registered with qt_tools.

In fact, what I've ended up doing is very close to what sphinx does:

• Add plugins by name in:

https://github.com/glue-viz/glue/blob/master/doc/conf.py#L32

(equivalent to what I call registering the plugin)

• Each plugin defines a setup function that does the actual set-up and is called internally in Sphinx:

https://github.com/astropy/astropy-helpers/blob/master/astropy_helpers/sphinx/ext/changelog_links.py#L74

The difference is that in Sphinx, all plugins are loaded at the start. If we don't want that to happen, then we need to make them be loaded as needed, which is why plugins need to be registered on a per-registry basis.

However, the terminology is confusing (registering plugins with registries...) so an alternative API is to simply have registries have a plugins attribute which is a list, and users can append, insert, or remove form that list. So then one could do:

from glue.config import qt_tools
qt_tools.plugins.append('my.nice.qt_tool')

What do you think? The alternative would be to have a single list of plugins like sphinx, so e.g.:

from glue.config import plugins
plugins.append('my.nice.qt_tool')

but then it would be hard to determine when plugins should be loaded and we might end up importing qt straight up.

However, we could allow both - that is, have a general purpose plugins registry for when users don't care if it's loaded straight away, and then allow plugins to also be added on a per-registry basis. Then we get the best of both world.

[as a side note I think we could also in addition rename load_plugin to setup to match sphinx and make it less confusing]

What do you think?

[ChrisBeaumont]

In that sense, a plugin entry in the registry is a pointer to deferred registry entries.

This sounds like the essential distinction between a normal registry item and a plugin -- the plugin mechanism exists solely to add things to a registry in the future.

I think the names plugin and registry are confusing, if this is the main difference between them (to my casual ears plugin and registry sound like synonyms, so it's hard to keep the distinction straight). Maybe add_plugin should be called something else (lazy_add, maybe?). Then a "plugin" is more clearly just an alternative, more advanced way of adding things to a registry. What do you think?

I like using setup instead of load_plugin. One other alternative, instead of relying on a naming convention, the string could just be a path to the function to run: my.nice.qt_tool.setup.

Once we decide on an API, this feature should be added to the sphinx docs as well.

[astrofrog]

@ChrisBeaumont - I agree with all your points. It's possible to write plug-in functionality and load it immediately not using lazy loading, so I agree the terminology was wrong. I have renamed add_plugin to lazy_add and load_plugin to setup. I decided to stay with requiring the function to set it up should be setup (might as well settle on a standard, right?). I've now also added sections to the customization.rst file. Let me know what you think!

Note that currently I still need to make the spectrum tool and PV slicing tool use this. As a side note, should the registry of models to fit live in the spectrum tool plugin?

[ChrisBeaumont]

might as well call this something like lazy_members

[astrofrog]

Actually, should this then just be a public attribute and we get rid of lazy_add, so that users can append but also remove from lazy_members?

[ChrisBeaumont]

Oh sorry -- I meant to type _lazy_members. I don't have any opinions on whether it should be public or private though.

[ChrisBeaumont]

Much clearer to me know! looks good

[astrofrog]

Thanks for the review! I've now moved the PV slicer and Spectrum tool to use this too, so if it passes, I'll go ahead and merge :)