Provide access to Viewer instance to all hook functions

[jwindhager]

🚀 Feature

Always pass the calling napari.Viewer instance to functions decorated with napari_hook_implementation

Motivation

In some cases, plugins may require access to napari's Viewer instance. For example, when opening a file that should not immediately be displayed as layer, but populate a plugin's dock widget instead (cf. #2201), the reader_function needs to have access to the plugin's dock widget registered with napari.

Pitch

Always pass the calling napari.Viewer instance to functions decorated with napari_hook_implementation

Alternatives

Obtain the viewer instance using Python's inspect module (ugly):

def reader_function(path):
    viewer = _get_viewer()
    # ...

def _get_viewer() -> Optional[Viewer]:
    import inspect
    frame = inspect.currentframe().f_back
    while frame:
        instance = frame.f_locals.get('self')
        if instance is not None and isinstance(instance, Viewer):
            return instance
        frame = frame.f_back
    return None

[sofroniewn]

Always pass the calling napari.Viewer instance to functions decorated with napari_hook_implementation

Hi @jwindhager, we've been quite intentional about not implementing this as much as possible as we've been trying to maintain a loose coupling between plugins as napari - for example an IO hookspec should really just have to think about IO and not depend on or know about the napari viewer. I think we should try and find another way to meet your needs here, that also doesn't involve using Python's inspect!

In some cases, plugins may require access to napari's Viewer instance. For example, when opening a file that should not immediately be displayed as layer, but populate a plugin's dock widget instead (cf. #2201), the reader_function needs to have access to the plugin's dock widget registered with napari.

For example here I'd rather think if there was a way that the dock widget was able to use the IO hookspec (which remained minimal) rather than the other way around.

Another use case that I'm particularly interested in supporting is that of people using functions from the napari_experimental_provide_function hookspec inside a dock widget. I think in particular for a lot of the things @haesleinhuepf is doing he could register functions with the provide_function hookspec, but would then access those functions (and potentially other functions registered by other plugins) within his dock_widget.

Maybe @jwindhager you can give a little more detail/ point to some code and we can try and get this approach working inside your dock widget. It might not be possible yet, we might need to make some other things available on the napari side to allow all this to work.

[jwindhager]

Hi @sofroniewn, thanks for the detailed considerations!

First, I want to say a big thank you for working on this amazing project!

Now, I fully support your intention of loose coupling between plugins. This, actually, was the main intention behind this issue. IMHO, not providing access to napari's Viewer instance does not help here. Following up on the (somewhat vaguely described - apologies!) use case above, here is a concrete example:

We recently developed napari-imc for reading and displaying file formats related to Imaging Mass Cytometry, a multiplexed imaging technique for biological tissue routinely employed in our lab that produces 2D images with up to ~50 channels. In the case of one of these proprietary :-( file formats, namely .mcd, one file contains different images from different sources:

• Panoramas: these are "conventional" optical images with a large FOV taken by a camera at low resolution
• Acquisitions: these are the aforementioned images originating from imaging mass cytometry (rastered laser ablation)
• Images in arbitrary file formats "uploaded" to .mcd files by the user, before/after ablation images, slide images, etc

All of these images share the same (machine) coordinate system, but not (necessarily) the same ROI, and acquisitions (at different locations) share the same channels. That's why I wrote a combined file reader/dock widget plugin, to be able to toggle-display images/channels as necessary. Here is a screenshot - I hope it will illustrate why a combined reader/dock widget is necessary in this case (in part this is also caused by napari not supporting the concept of named channels shared across layers/images as of now - should I open another feature request for this pretty big change?):

I hope this makes sense until now. Please have a look at /~https://github.com/BodenmillerGroup/napari-imc/blob/master/napari_imc/_reader.py to see how the reader hook is implemented at the moment.

Now, as you suggested already, one could - instead of implementing a napari reader hook - just add, let's say, an "open IMC file" button to the custom widget. This indeed sounded like a cleaner approach to me at first and is in fact what we had in earlier versions. However, this comes with a big caveat: other plugins cannot simply call viewer.open('/path/to/file.mcd') anymore and would have to directly interact with the napari-imc plugin instead - this would result in the exact opposite of loose coupling.

I'm open to any suggestions how to improve the design of this plugin. In general, however, I feel that the "open file" hook should not be restricted to "just" adding layers, to support loose coupling of plugins, and therefore should be provided access to the Viewer instance. Similar situations could arise for other hook specs.

[sofroniewn]

First, I want to say a big thank you for working on this amazing project!

Thanks that's nice of you to say!

Here is a screenshot - I hope it will illustrate why a combined reader/dock widget is necessary in this case

I love the screenshot, your tool looks incredible! Your needs remind me a little of some of the napari-omero plugin that @tlambert03 is working on too - it blurs the line between reader and dock widget plugin too

(in part this is also caused by napari not supporting the concept of named channels shared across layers/images as of now - should I open another feature request for this pretty big change?):

You might want to look at #970 and #1502 first - I know @tlambert03 is eager to pick this work back up again. Curious if it sounds like they might be what you want.

I hope this makes sense until now. Please have a look at /~https://github.com/BodenmillerGroup/napari-imc/blob/master/napari_imc/_reader.py to see how the reader hook is implemented at the moment.

I will do this and get back to you! More coming once I've had a chance to dive in. Thanks!!

[sofroniewn]

Ok, I've had a little bit more time to look into this now, and this is such a cool data type! I've not worked with Imaging Mass Cytometry data before, but it looks very fun! (Not sure if @jni has, but he might be interested in this issue too).

I hope it will illustrate why a combined reader/dock widget is necessary in this case (in part this is also caused by napari not supporting the concept of named channels shared across layers/images as of now - should I open another feature request for this pretty big change?):

It seems like this is really the crux of it. If we had layer groups (see #970) sorted out with a nice UI than the widgets that you provide in the top right hand corner of your screenshot would actually be built-in to napari and you might not even need your custom dock widget, or if you did it would be much more minimal and could focus just on your domain specific needs rather than the nesting/ grouping needs.

You could use things like layer visibility to make loading lazy if that was a concern too.

At that point viewer.open('/path/to/file.mcd', plugin='napari-imc') would be very usable. My thoughts are that we might want to push on #1502 with another great use case in mind and then revisit this? Does that seem reasonable. What does @tlambert03 think?

[jwindhager]

Thanks, it indeed looks quite similar to napari-omero! I guess that there is quite a need for a tree-like structure with shared channel properties in the multiplexed imaging community.

Also, thanks for pointing me to #970 and #1502, this would be incredible to have! In this case, as you wrote, the tree structure in the upper right part of my screenshot would not be required anymore, as it would be supported by napari directly. I also fully support @kevinyamauchi's proposal and just commented in #970. Depending on the result of the discussion over there and the associated pull request, this issue may be resolved for the described use case.

However, depending on how #970 develops, I think it might still be worthwhile considering providing access to the Viewer instance - possibly as private function argument? - to be prepared for future requests related to "file open" hook specs and others.

[jni]

@jwindhager very cool stuff as I mentioned in #970!

I think there is a bigger issue re providing the viewer which is that we generally don't want plugins messing with the viewer willy-nilly, and in particular when I drag a file and a new dock widget appears, or 70 such widgets, I want to be able to tell napari "shut down all plugins" or "shut down the imc plugin" and have that happen — which is not possible if plugins add their own dock widgets.

Ultimately, we do give dock widgets access to the viewer anyway, so they can still do this. But we want to come up with a model where dock widgets don't spawn other widgets, but rather ask napari to do this for them. OR, with a model where plugins provide build and teardown functions.

I wonder whether we could do something like this for IO widgets: have the hookspec return a reader_function or a reader_function and a dock widget class, which we will instantiate as with experimental_dock_widget when the file is read.

I don't know whether this actually solves the coupling issues, it's 1am here 😅, but this kind of semi-declarative approach makes more sense to me than giving reader functions access to the viewer so they can modify it. That seems like a different beast altogether.

We are very happy to keep iterating on these ideas though @jwindhager! Just fyi the US/Europe dev/community meeting is in a couple of hours, you might want to join in, check the #dev-meeting channel on https://napari.zulipchat.com! Everyone welcome, especially people with cool data to show! 😂

[tlambert03]

There's a lot to discuss here, so forgive the somewhat quick response, just wanted to get some thoughts out.

Thank you very much @jwindhager for your detailed and helpful comments! I concur, your application is cool and we certainly want to make sure you can do what you need to. I agree, it looks very similar to some of the stuff I'd like to do in napari-omero, so we've certainly got common goals 😄. My very brief two cents:

• as has mentioned by others, I fully agree that layergroups will be the key to much of this conundrum. It's actually not as far away as it might first appear. I will resuscitate [WIP] Layergroups (TJL) #1502 and submit a few incremental PRs in the coming days to get the ball rolling here again.
• I'm -1 on passing the Viewer to pure io hookspecs for the same reasons mentioned by @sofroniewn. (If we passed the viewer, we really don't need a "reader" hookspec at all, since it basically enables the user to do anything.)
• I'm even more -1 on the concept of temporarily modifying non-experimental hookspecs (like the IO plugins) as a stopgap while we build the "better" long-term solution. Once taken out of experimental, hookspecs kind of need to be the most stable part of our API, so they unfortunately demand laborious consideration and slow iteration.
• I agree with @jni that layergroups should be arbitrary groupings of layers based on whatever the user deems semantically relevant. My current branches are building layergroups as generically as possible... which would definitely allow grouping by channel – probably the most immediate application – but will also allow us flexibility going forward.

more soon, thanks again for your thoughts!

[jwindhager]

Hi both, thanks so much for your kind words @tlambert03 and @jni!

I wonder whether we could do something like this for IO widgets: have the hookspec return a reader_function or a reader_function and a dock widget class, which we will instantiate as with experimental_dock_widget when the file is read.

I thought about this again (a bit more awake), and arrived at the same conclusion: coming up with more declarative approach to this, ideally without exposing Qt instances, would be the cleanest solution. However, this in the end will only work if napari's UI is flexible enough to accommodate any type of image, now or in the future. E.g., in the described used case, the plugin would return a (yet to be defined) tree structure instead of the bare-bones raw image data array, which could then be appended to a tree as proposed in #970 (comment), provided both the tree structure and the channel list is supported out-of-the-box.

Just fyi the US/Europe dev/community meeting is in a couple of hours, you might want to join in, check the #dev-meeting channel on https://napari.zulipchat.com! Everyone welcome, especially people with cool data to show! joy

Thanks for the invite, I didn't know about this - happy to join!

I'm -1 on passing the Viewer to pure io hookspecs
I'm even more -1 on the concept of temporarily modifying non-experimental hookspecs

Thinking about this again, I fully agree! See my answer above

I agree with @jni that layergroups should be arbitrary groupings of layers based on whatever the user deems semantically relevant. My current branches are building layergroups as generically as possible... which would definitely allow grouping by channel – probably the most immediate application – but will also allow us flexibility going forward.

In my opinion, tree structures for images (screenshot, top right) and layer groups (screenshot, bottom right) are two orthogonal - yet essential - animals. Allowing for arbitrary groupings would definitely be a cool feature to see! This is also what I meant in #970 (comment) with "tree items grouped by one of their properties". Probably not the best way to describe this 😂

In any case, if you agree, I think this issue can be closed now in favor of #970 - I'll stick with the inspect approach linked earlier until tree structure/channel lists are supported OOTB. Happy to keep iterating, though! 😄