napari / napari-plugin-engine

A fork of pluggy - plugin management package
https://napari-plugin-engine.readthedocs.io/en/latest/
MIT License
6 stars 8 forks source link

Add a napari-free way to leverage plugins in `npe`? #24

Open tlambert03 opened 3 years ago

tlambert03 commented 3 years ago

@manzt inquired whether it was possible to use reader plugins detected by napari_plugin_engine without having to import napari. It seems like an extremely reasonable and valuable thing to be able to do. For instance, a developer may invest some time to write napari reader plugins for a few formats, but currently they pretty much have to use them through napari. That's a shame, particularly since we've also made an effort to keep most of the "napari-specific" stuff out of the return value from the reader plugins (though the metadata dict is obviously a bit specific).

@jni, @sofroniewn what would you think of adding a separate module here that would provide some convenience functions like that? It is a little napari specific... but we would keep it isolated in a module, and we wouldn't even need to put the hook specifications in here.

example of a minimal napari-free function to use available reader plugins ```python from napari_plugin_engine import PluginManager pm = PluginManager(project_name='napari', discover_entry_point='napari.plugin') def plugin_reader(path: str, plugin_name: str = None): get_readers = pm.hook.napari_get_reader if plugin_name: try: reader = get_readers(path=path, _plugin=plugin_name) except KeyError: raise ValueError( f"No plugin named {plugin_name}. Valid names are {set(pm.plugins)}" ) else: # returns a list of functions that claim to be able to read path readers = get_readers(path=path) # here you have to decide whether to just go with the first one, # or "try/except" and return the first one that doesn't raise an # exception. let's just use the first one. if readers: reader = readers[0] else: raise ValueError(f"No plugin available to read path: {path}") # actually call the reader function (plugin may raise an error?) return reader(path) ```
sofroniewn commented 3 years ago

Goal seems very reasonable request to me, I'm supportive! I'm not quite sure what is different in that code snippet from how things are now, but I'd probably say press on with a PR and I'd be happy to review details then. I don't see any obvious downsides

tlambert03 commented 3 years ago

yeah... it's definitely possible that napari itself could just use this code too. It would essentially replace io.read_data_with_plugins

sofroniewn commented 3 years ago

Right, I see now. Sorry i sort of missed what was going on before, this is actually a more fundamental change then I had realized (and again I might not quite be getting it right now either), but right now the napari-plugin-engine doesn't know about any of our hookspecs, and now we'd be adding get_readers = pm.hook.napari_get_reader which is very much connected to one of hookspecs. I guess I'm actually more negative on this coupling. My hesitation is more than just the napari coupling, it's really about hookspec information making it into the plugin engine, which I think should be free of all hookspec info (correct me if that's not how it is now though!).

For instance, a developer may invest some time to write napari reader plugins for a few formats, but currently they pretty much have to use them through napari. That's a shame, particularly since we've also made an effort to keep most of the "napari-specific" stuff out of the return value from the reader plugins (though the metadata dict is obviously a bit specific).

I guess let's say they want to use them in their own tool I think they need to provide the code snippet that you provided.

If we one day had a repo with all our hookspecs but no napari, then it would make sense for these snippets to live there.

I'm not sure, but I see now that there is more going on here!

tlambert03 commented 3 years ago

napari-plugin-engine doesn't know about any of our hookspecs ... My hesitation is more than just the napari coupling, it's really about hookspec information making it into the plugin engine

This doesn't actually require us to move our hookspecs here. (the plugin manager can register implementations for "unregistered" hookspecs... which is what that example above does). But it does form an implicit connection that "somewhere out there, plugin developers are decorating functions for such-and-such hookspec over at napari". Which, is always going to be true?

so calling pm.hook.napari_get_reader really is just saying "hey pm ... did you happen to see any implementations for a hook spec named "napari_get_reader"? (Where pm here is declared as PluginManager(project_name='napari', discover_entry_point='napari.plugin') ... all strings)

However, I totally get it, and that's why I opened an issue instead of a PR :) I think the ultimate goal is very important though: we really ought to be rewarding developers who take time to put something in a certain format... and that reward should come in the form of them being able to use that code in any environment that has napari_plugin_engine (a small pure python dep), rather than needing Qt, GL stuff, etc...

sofroniewn commented 3 years ago

But it does form an implicit connection that "somewhere out there, plugin developers are decorating functions for such-and-such hookspec over at napari". Which, is always going to be true?

Yes, I think "implicit" is a good word here

However, I totally get it, and that's why I opened an issue instead of a PR :) I think the ultimate goal is very important though: we really ought to be rewarding developers who take time to put something in a certain format... and that reward should come in the form of them being able to use that code in any environment that has napari_plugin_engine (a small pure python dep), rather than needing Qt, GL stuff, etc...

Yeah, I like that - and maybe one day our hook specs are so stable that they actually live here. Or maybe one day we have three repos

We don't need to do this now, but curious how that decomposition sounds? Do we want to start work on that middle repo? Maybe not yet, but maybe soon?

These are just some rough ideas, I wonder what @jni thinks about all this too

tlambert03 commented 3 years ago

linking https://github.com/NHPatterson/wsireg/issues/6 as a real-world example of something that could benefit...

jni commented 3 years ago

@tlambert03 does this blur the line a bit with https://blog.danallan.com/posts/2020-03-07-reader-protocol/? ie do we want to aim to implement that protocol and then have napari use it? I think that's essentially @sofroniewn's proposal above, and I think it makes sense...

But yeah I get the goal and we should fix it one way or the other.

Also — @tlambert03 could the above snippet not live essentially anywhere, ie in its own repo? napari-readers?

tlambert03 commented 3 years ago

I wouldn't say it blurs the line. I'm fine implementing any protocol, honestly the only goal here is to let people like @manzt reuse the work they've done making something that takes a path and returns data... but without needing all the dependencies of napari. Don't have a strong opinion on exactly what the interface is. I certainly like dan's stuff and would be happy to go that way if that's what everyone wants.

yep. could live anywhere

sofroniewn commented 3 years ago

Ok, good discussion. In the interest of pragmatism and helping people, I'm ok adding plugin_reader to this repo as a quick fix for others to use.

I'd like us to keep in mind the idea of an image_analysis_plugin_interface or imaging_plugin_interface repo with the hook specs, and some of these convenience functions, but we can delay on that until we have a more clear vision.

Once that is released we could deprecate plugin_reader and migrate people to that new package.

I'd also be ok if @tlambert03 you wanted to start on that new repo now, but that might be more effort than we're ready for now.

How does this all sound?

jni commented 3 years ago

I wouldn't say it blurs the line.

Well, I guess what I'm trying to say is, we're going "all in" on saying "here's a protocol/group of protocols for getting data into napari, and by the way now it's for getting data into Python period". And so I'd like us to not get into the 15th-competing-standard situation.

But the progressive approach that @sofroniewn describes makes sense enough to me...

manzt commented 3 years ago

Just catching up. Thanks all! I hadn't realized napari-plugin-engine was more general and not "napari-specific". (In fact, I think I will add a custom entrypoint to some of my zarr-store packages, so that simple-zarr-server can launch a HTTP server by filepath 😄).

From my point of view it's not critical where plugin_reader lives (I'm even happy to just have it implemented in our project), but I agree with @tlambert03 that having convenience methods for some interfaces would be really nice. So in that regard, I am in favor of the suggestion from @sofroniewn

imagesc-bot commented 1 year ago

This issue has been mentioned on Image.sc Forum. There might be relevant details there:

https://forum.image.sc/t/napari-python-cell-profiler-batch-processing/88433/2